Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADAM-1480] Add switch to disable the fast concat method. #1479

Merged

Conversation

@fnothaft
Copy link
Member

@fnothaft fnothaft commented Apr 7, 2017

Resolves #1478.

@coveralls
Copy link

@coveralls coveralls commented Apr 7, 2017

Coverage Status

Coverage increased (+0.08%) to 81.716% when pulling 9f4c33b on fnothaft:issues/1478-concat-fallback into 93b32c6 on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Apr 7, 2017

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1936/
Test PASSed.

@@ -56,6 +56,10 @@ class ADAM2VcfArgs extends Args4jBase with ParquetArgs {
@Args4jOption(required = false, name = "-single", usage = "Save as a single VCF file.")
var single: Boolean = false

@Args4jOption(required = false, name = "-disable_fast_concat",
usage = "Disables the parallel file concatenation engine.")
var disableFastConcat: Boolean = false

This comment has been minimized.

@heuermh

heuermh Apr 10, 2017
Member

Not a fan of double-negatives; is there a better way to do args4j flags that default to true?

* If asSingleFile is true and deferMerging is false, disables the use of the
* fast file concatenation engine.
*/
var disableFastConcat: Boolean

This comment has been minimized.

@heuermh

heuermh Apr 10, 2017
Member

Per this, #1295, and #1438, perhaps we should brainstorm on how to clean up the save methods and ensure consistency. One or more enums, better SaveArgs classes, a context map, etc.

@heuermh
Copy link
Member

@heuermh heuermh commented Apr 10, 2017

I'd be willing to merge this without changes. It adds more pressure towards a future refactoring of save methods for consistency though.

@fnothaft
Copy link
Member Author

@fnothaft fnothaft commented Apr 17, 2017

@heuermh with #1487 in the pipe, shall we merge this as is?

@fnothaft fnothaft force-pushed the fnothaft:issues/1478-concat-fallback branch from 9f4c33b to fe0b50b Apr 22, 2017
@fnothaft
Copy link
Member Author

@fnothaft fnothaft commented Apr 22, 2017

Ping.

@coveralls
Copy link

@coveralls coveralls commented Apr 22, 2017

Coverage Status

Coverage decreased (-0.2%) to 81.787% when pulling fe0b50b on fnothaft:issues/1478-concat-fallback into 93d17b4 on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Apr 22, 2017

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1957/
Test PASSed.

@heuermh
Copy link
Member

@heuermh heuermh commented Apr 24, 2017

Not a fan of double-negatives; is there a better way to do args4j flags that default to true?

What I'm thinking is something like this
http://stackoverflow.com/questions/15008758/parsing-boolean-values-with-argparse/31347222#31347222

where instead of -disable arguments that default to false, we have positive and negative arguments in a mutally exclusive group with the positive one being defaulted to true. That way the field can be named in the positive and code reads better. I'm not quite sure how to set up such with Args4j though.

@fnothaft
Copy link
Member Author

@fnothaft fnothaft commented Apr 24, 2017

I don't really see the -disable "negative" name as an issue, and I don't have the bandwidth to track down how to do what you're talking about with Args4j. Can we merge this as is?

@heuermh
Copy link
Member

@heuermh heuermh commented Apr 24, 2017

Args4j can simulate mutually exclusive groups with forbids, however I don't see a way of combining two --feature and --no-feature style options into a single value as I describe above.

An alternative would be to use ExplicitBooleanOptionHandler and default to true, i.e.

@Args4jOption
  (
    required = false,
    name = "-fast_concat",
    usage = "Enable the parallel file concatenation engine. Defaults to true.",
    handler = classOf[ExplicitBooleanOptionHandler]
  )
var fastConcat: Boolean = true

Then these would be true

$ adam-submit -- adam2vcf -single foo.adam foo.vcf
$ adam-submit -- adam2vcf -single -fast_concat true foo.adam foo.vcf

and this would be false

$ adam-submit -- adam2vcf -single -fast_concat false foo.adam foo.vcf

and this would fail

$ adam-submit -- adam2vcf -single -fast_concat foo.adam foo.vcf
"foo.adam" is not a legal boolean value
 VCF    : The VCF file to convert
 ADAM   : Location to write ADAM Variant data
...

Would that be an improvement?

@heuermh
Copy link
Member

@heuermh heuermh commented Apr 24, 2017

Created #1503 for further discussion

@heuermh heuermh merged commit 1fb57a3 into bigdatagenomics:master Apr 24, 2017
1 of 3 checks passed
1 of 3 checks passed
codacy/pr Not so good... This pull request quality could be better.
Details
coverage/coveralls Coverage decreased (-0.2%) to 81.787%
Details
default Merged build finished.
Details
@heuermh
Copy link
Member

@heuermh heuermh commented Apr 24, 2017

Thank you, @fnothaft!

@fnothaft fnothaft deleted the fnothaft:issues/1478-concat-fallback branch Jun 22, 2017
@heuermh heuermh added this to the 0.23.0 milestone Dec 7, 2017
@heuermh heuermh added this to Completed in Release 0.23.0 Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.