Major changes to the command line parsing system #135

lbergelson · 2015-01-21T20:25:30Z

using jopt-simple to do the actual parsing. Right now it's just been dropped in to handle decomposing the arguments into key->value pairs.

supports short (-h) and long (--help) styles
supports boolean flags with or without an argument
options are now case sensitive

argument collections can be specified with @ArgumentCollection
NestedOptions removed -> replaced by ArgumentCollection

Option.fullName() is now respected if specified, if it isn't it defaults to use the field name
Overrideable has been removed from Option
Arguments must now be uniquely specified (no arguments targeting multiple fields any more) and no redefining existing ones.
The ability to override a specified option is a useful one, so it might need to be reimplemented at some point.
Collection defaults are now replaced rather than appended too

also opportunistically fixed a broken test in CreateSequenceDictionaryTestusing jopt-simple to do the actual parsing. Right now it's just been dropped in to handle decomposing the arguments into key->value pairs. In future it could also handle more of the argument checking and help formatting.

supports short (-h) and long (--help) styles
supports boolean flags with or without an argument
options are now case sensitive

argument collections can be specified with @ArgumentCollection
NestedOptions removed -> replaced by ArgumentCollection

Option.fullName() is now respected if specified, if it isn't it defaults to use the field name
Overrideable has been removed from Option
Arguments must now be uniquely specified (no arguments targeting multiple fields any more) and no redefining existing ones.
The ability to override a specified option is a useful one, so it might need to be reimplemented at some point.
Collection defaults are now replaced rather than appended too

also opportunistically fixed a broken test in CreateSequenceDictionaryTest

lbergelson · 2015-01-21T20:29:02Z

jopt-simple docs are here http://pholser.github.io/jopt-simple/

This should close #81, #111 and #67

coveralls · 2015-01-21T20:32:51Z

Coverage decreased (-0.29%) to 63.41% when pulling eb1a39e3e6e5454f59be1d76e8ae9c9ab8dcbbba on lb_unix_style_options into 93aad64 on master.

mattsooknah · 2015-01-21T22:21:12Z

src/main/java/org/broadinstitute/hellbender/cmdline/CommandLineParser.java

+                try {
+                    createOptionDefinitions(field.get(callerOptions));
+                } catch (final IllegalAccessException e) {
+                    throw new GATKException.CommandLineParserInternalException("BUG: NestedArguments must be public must be public.", e);


Minor typo here.

Ooh, thanks.

mattsooknah · 2015-01-21T22:39:22Z

I believe Option.overridable() is needed for some CRAM stuff - @jacarey would know more. That's not to say that it needs to be implemented now, just FYI.

lbergelson · 2015-01-21T22:45:33Z

It seemed like it was for child parsers to be able to override the master
parsers decision when things are specified in multiple places. I
disallowed specifying the same option in multiple places though for now, so
I didn't think it was necessary. We can reinstate it if necessary.

mattsooknah · 2015-01-21T23:18:39Z

src/main/java/org/broadinstitute/hellbender/utils/test/ArgumentsBuilder.java

+        ArgumentsBuilder args = new ArgumentsBuilder(new Object[]{"Option=" + new File("/path/to"), "OtherOption=" + -1});
+        Assert.assertEquals(args.getArgsArray(), new String[]{"--Option","/path/to", "--OtherOption", "-1"});
+    }
+


I'd say either create a separate class with the actual @Tests, or move this whole class to src/test

oops, i had meant to do that

On Wed, Jan 21, 2015 at 6:18 PM, Matt Sooknah [email protected]
wrote:

In
src/main/java/org/broadinstitute/hellbender/utils/test/ArgumentsBuilder.java
#135 (comment)
:

}

@test

public void testOneBigString(){

ArgumentsBuilder args = new ArgumentsBuilder();

args.add(" Value=1 Value=2 Value=3 Value= 4 ");

Assert.assertEquals(args.getArgsList().toArray(), new String[]{"--Value", "1", "--Value", "2",

"--Value", "3","--Value", "4"});

}

@test

public void testFromArray(){

ArgumentsBuilder args = new ArgumentsBuilder(new Object[]{"Option=" + new File("/path/to"), "OtherOption=" + -1});

Assert.assertEquals(args.getArgsArray(), new String[]{"--Option","/path/to", "--OtherOption", "-1"});

}

I'd say either create a separate class with the actual @tests, or move
this whole class to src/test

—
Reply to this email directly or view it on GitHub
https://github.com/broadinstitute/hellbender/pull/135/files#r23343682.

coveralls · 2015-01-22T14:56:36Z

Coverage decreased (-0.2%) to 63.5% when pulling 2b3620e42107d0090f3fc270fdfca14ec7d682c7 on lb_unix_style_options into 93aad64 on master.

akiezun · 2015-01-22T15:17:01Z

src/main/java/org/broadinstitute/hellbender/cmdline/ArgumentCollection.java

-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+import java.lang.annotation.*;


we should standardize on the style of imports or we'll have those edits all the time. Issue #138

akiezun · 2015-01-22T15:43:47Z

looks good, small comments. CommandlineLineParser is a pretty complex beast and I've not reviewed it fully but it's fine for now from me. @droazen should weigh in on this pull req too before you merge.

coveralls · 2015-01-22T17:53:14Z

Coverage decreased (-0.21%) to 63.5% when pulling e48a1360708acbb82b1179a5897c4039b5f067f6 on lb_unix_style_options into 93aad64 on master.

droazen · 2015-01-22T18:12:02Z

src/main/java/org/broadinstitute/hellbender/cmdline/CommandLineParser.java

    }

    // This is the object that the caller has provided that contains annotations,
    // and into which the values will be assigned.
    private final Object callerOptions;

-    // For child CommandLineParser, this contains the prefix for the option names, which is needed for generating
-    // the command line.  For non-nested, this is the empty string.
-    private final String prefix;


good riddance to this garbage :)

vdauwera · 2015-01-22T18:36:13Z

Not sure if this is the right place for me to jump in (so redirect me if not), but question: is there any plan to make it possible to define min/max values for numeric argument values? This is something we got to only in a very basic way in GATK but would be useful to enable in order to save users from themselves. This would involve two levels:

hard min/max values that correspond to limits beyond which values could cause errors/program failures; violation should throw a User Exception;
recommended min/max values that correspond to limits beyond which values do not make sense for a given analysis functionality for standard use cases; violation should log a WARN entry.

droazen · 2015-01-22T18:36:42Z

src/main/java/org/broadinstitute/hellbender/cmdline/CommandLineParser.java

+        for (String stringValue: values) {
+            final Object value;
+            try {
+                if (stringValue.equals("null")) {


Should be a named constant rather than hardcoded literal.

droazen · 2015-01-22T18:52:38Z

Review complete. Looks great, just a few relatively minor comments.

mattsooknah · 2015-01-22T18:56:40Z

@vdauwera General arg checking can be done in customCommandLineValidation, which returns zero or more error messages and prints them to stderr. I suppose log / warn messages could also be put in there.

But it might be nice to have a more specific process for checking values of numeric args.

droazen · 2015-01-22T19:03:17Z

@vdauwera If you want this (or any other) feature from the old GATK to make it into hellbender, feel free to create a ticket in our github issue tracker -- we can always close as WONTFIX if we deem it impractical or incompatible with the new codebase. I don't think adding minValue/maxValue attributes to @option would be a problem, though.

vdauwera · 2015-01-22T19:48:45Z

Thanks guys -- will create tickets from now on.

coveralls · 2015-01-23T15:26:50Z

Coverage decreased (-0.21%) to 63.5% when pulling eb6062f11e8c37cf1ab512913dfac04e573bd38b on lb_unix_style_options into 9195042 on master.

-using jopt-simple to do the actual parsing -supports short (-h) and long (--help) styles -supports boolean flags with or without an argument -options are now case sensitive -argument collections can be specified with @ArgumentCollection -Option(fullname) is now respected if specified, if it isn't it defaults to use the field name -several Option values are no longer used, overrideable -NestedOptions removed -> replaced by ArgumentCollection -Arguments must now be uniquely specified (no arguments targetting multiple fields any more) -Collection defaults are now replaced rather than appended too -opportunistically fixed a broken test in CreateSequenceDictionaryTest

lbergelson · 2015-01-23T15:58:19Z

I think I've addressed all the comments. Going to merge on travis success.

coveralls · 2015-01-23T16:04:58Z

Coverage decreased (-0.21%) to 63.5% when pulling 8a93d43 on lb_unix_style_options into 9195042 on master.

Major changes to the command line parsing system Closes #67, #81, #111

* revises explore to work with Interpretation * deletes lingering pdb.set_trace() * Interpretation enums * Interpretation for string and continuous * Tested continuous * Nitpicks * adds parenthesis * Nate's requested changes plus others: 1. If tensor is scaler (`if tm.shape[0] == 1`), isolates the value inside the numpy array via `tensor = tensor.item`. If not, retain the value as an array (e.g. categorical labels). 2. In the DataFrame, casts the `fpath`, `generator`, and any tm with that has `Interpretation.Language` into "string" dtypes (see https://pandas.pydata.org/pandas-docs/stable/user_guide/text.html). This may confer some speed benefit in the future compared to the default `object` dtype. 3. If dropping NaN rows from dataframe (how we obtain intersect of tensor maps) results in an empty dataframe, skips calculating summary statistics. Previous attempts to do that threw errors. * removes import pdb Co-authored-by: paolodi <[email protected]>

mattsooknah reviewed Jan 21, 2015
View reviewed changes

lbergelson force-pushed the lb_unix_style_options branch from 2f67422 to 2b3620e Compare January 22, 2015 14:45

akiezun reviewed Jan 22, 2015
View reviewed changes

droazen reviewed Jan 22, 2015
View reviewed changes

lbergelson force-pushed the lb_unix_style_options branch from e48a136 to eb6062f Compare January 23, 2015 15:18

lbergelson force-pushed the lb_unix_style_options branch from eb6062f to 8a93d43 Compare January 23, 2015 15:53

lbergelson added a commit that referenced this pull request Jan 23, 2015

Merge pull request #135 from broadinstitute/lb_unix_style_options

844f9cd

Major changes to the command line parsing system Closes #67, #81, #111

lbergelson merged commit 844f9cd into master Jan 23, 2015

lbergelson deleted the lb_unix_style_options branch January 23, 2015 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major changes to the command line parsing system #135

Major changes to the command line parsing system #135

lbergelson commented Jan 21, 2015

lbergelson commented Jan 21, 2015

coveralls commented Jan 21, 2015

mattsooknah Jan 21, 2015

lbergelson Jan 21, 2015

mattsooknah commented Jan 21, 2015

lbergelson commented Jan 21, 2015

mattsooknah Jan 21, 2015

lbergelson Jan 21, 2015

coveralls commented Jan 22, 2015

akiezun Jan 22, 2015

akiezun commented Jan 22, 2015

coveralls commented Jan 22, 2015

droazen Jan 22, 2015

vdauwera commented Jan 22, 2015

droazen Jan 22, 2015

droazen commented Jan 22, 2015

mattsooknah commented Jan 22, 2015

droazen commented Jan 22, 2015

vdauwera commented Jan 22, 2015

coveralls commented Jan 23, 2015

lbergelson commented Jan 23, 2015

coveralls commented Jan 23, 2015

Major changes to the command line parsing system #135

Major changes to the command line parsing system #135

Conversation

lbergelson commented Jan 21, 2015

lbergelson commented Jan 21, 2015

coveralls commented Jan 21, 2015

mattsooknah Jan 21, 2015

Choose a reason for hiding this comment

lbergelson Jan 21, 2015

Choose a reason for hiding this comment

mattsooknah commented Jan 21, 2015

lbergelson commented Jan 21, 2015

mattsooknah Jan 21, 2015

Choose a reason for hiding this comment

lbergelson Jan 21, 2015

Choose a reason for hiding this comment

coveralls commented Jan 22, 2015

akiezun Jan 22, 2015

Choose a reason for hiding this comment

akiezun commented Jan 22, 2015

coveralls commented Jan 22, 2015

droazen Jan 22, 2015

Choose a reason for hiding this comment

vdauwera commented Jan 22, 2015

droazen Jan 22, 2015

Choose a reason for hiding this comment

droazen commented Jan 22, 2015

mattsooknah commented Jan 22, 2015

droazen commented Jan 22, 2015

vdauwera commented Jan 22, 2015

coveralls commented Jan 23, 2015

lbergelson commented Jan 23, 2015

coveralls commented Jan 23, 2015