Thanks Kenn and Lukasz. How about we add a hidden option to `PipelineOptions` and set depending on `args == null` in `PipelinOptionsFactory` ? Then, we can get in `PipelineOptionsValidator#validate` and there is no need to change the interface.
@Hidden boolean getFromCLI(); void setFromCLI(boolean fromCLI); Besides, for programmers, do you think we need to print `public abstract java.lang.String org.apache.beam.sdk.options.PipelineOptionsValidatorTest$Required.getObject()` or just `getObject()` ? On Tue, Jun 27, 2017 at 6:24 AM Lukasz Cwik <[email protected]> wrote: > Sounds good to me, thus the change as currently proposed in PR/3438 needs > some work to do this split. > > On Mon, Jun 26, 2017 at 3:13 PM, Kenneth Knowles <[email protected]> > wrote: > > > I do think it is the responsibility of the CLI to own this; we definitely > > shouldn't rewrite PipelineOptions validation entirely, or programmatic > uses > > will have needless noise or bad messages. I think that when someone calls > > fromArgs() they have sufficiently indicated that they are a CLI code path > > and we can do the right thing. > > > > Separate from that, it might be handy to expose such a utility for users > > that build their command line options via some other mechanism. > > > > On Mon, Jun 26, 2017 at 3:07 PM, Lukasz Cwik <[email protected]> > > wrote: > > > > > In this case, we specifically asked in code to validate the options: > > > https://github.com/apache/beam/blob/1ea1de4aa9d32e3c5a596ccd7d84af > > > f1cc2a7428/examples/java/src/main/java/org/apache/beam/ > > > examples/WordCount.java#L173 > > > > > > It seems like we could know whether PipelineOptions validation is > > happening > > > from the PipelineOptionsFactory.fromArgs() and/or have a special CLI > > > option > > > like --validate (similar to --help) that would provide names based upon > > > arguments. > > > > > > In general, the PipelineOptionsValidator / .withValidation() method > > should > > > provide information about what method has not been set when not used > > from a > > > CLI. > > > > > > Or we make sure both are listed to not have this dichotomy based upon > how > > > PipelineOptions are used. > > > > > > On Mon, Jun 26, 2017 at 10:10 AM, Kenneth Knowles > <[email protected] > > > > > > wrote: > > > > > > > We should decouple CLI parsing validation from programmatic > > > PipelineOptions > > > > validation. > > > > > > > > On Mon, Jun 26, 2017 at 9:56 AM, Lukasz Cwik > <[email protected] > > > > > > > wrote: > > > > > > > > > This change assumes that all users are CLI users (compared to the > > > > existing > > > > > code which assumed that all users were programmatic users). Should > we > > > > word > > > > > the message so its useful for both CLI and users who set the > options > > > > > programmatically? > > > > > > > > > > On Mon, Jun 26, 2017 at 6:00 AM, Jean-Baptiste Onofré < > > [email protected] > > > > > > > > > wrote: > > > > > > > > > > > Thanks Manu ! > > > > > > > > > > > > I will review it. > > > > > > > > > > > > Regards > > > > > > JB > > > > > > > > > > > > > > > > > > On 06/26/2017 02:59 PM, Manu Zhang wrote: > > > > > > > > > > > >> Thanks Kenn and JB, I just filed > > > > > >> https://issues.apache.org/jira/browse/BEAM-2514 and PR > > > > > >> https://github.com/apache/beam/pull/3438 > > > > > >> Please help to review. > > > > > >> > > > > > >> Thanks, > > > > > >> Manu > > > > > >> > > > > > >> On Mon, Jun 26, 2017 at 12:20 PM Jean-Baptiste Onofré < > > > > [email protected]> > > > > > >> wrote: > > > > > >> > > > > > >> Hi Manu, > > > > > >>> > > > > > >>> Agree, it makes sense. > > > > > >>> > > > > > >>> Regards > > > > > >>> JB > > > > > >>> > > > > > >>> On 06/25/2017 12:57 PM, Manu Zhang wrote: > > > > > >>> > > > > > >>>> Hi all, > > > > > >>>> > > > > > >>>> Currently, if a required option is missing for a Beam > pipeline, > > > the > > > > > >>>> error > > > > > >>>> message is like > > > > > >>>> > > > > > >>>> > > > > > >>>>> Exception in thread "main" > java.lang.IllegalArgumentException: > > > > > Missing > > > > > >>>>> required value for [public abstract java.lang.String > > > > > >>>>> org.apache.beam.examples.WordCount$WordCountOptions. > > getOutput(), > > > > > "Path > > > > > >>>>> > > > > > >>>> of > > > > > >>> > > > > > >>>> the file to write to"]. > > > > > >>>>> > > > > > >>>> > > > > > >>>> > > > > > >>>> This is quite long but doesn't give any hint to users about > the > > > > > required > > > > > >>>> option. Instead, I'm thinking about something like > > > > > >>>> > > > > > >>>> Exception in thread "main" java.lang.IllegalArgumentException: > > > > > Missing > > > > > >>>> > > > > > >>>>> required option [--output, "Path of the file to write to"]. > > > > > >>>>> > > > > > >>>> > > > > > >>>> > > > > > >>>> It can be achieved by adding a method that *returns the option > > > from > > > > a > > > > > >>>> method* to ProxyInvocationHandler > > > > > >>>> < > > > > > >>>> > > > > > >>> https://github.com/apache/beam/blob/master/sdks/java/core/ > > > > > >>> src/main/java/org/apache/beam/sdk/options/ > > > > ProxyInvocationHandler.java > > > > > >>> > > > > > >>>> > > > > > >>>> class. > > > > > >>>> > > > > > >>>> public String getOption(Method method) { > > > > > >>>> return gettersToPropertyNames.get(method.getName()); > > > > > >>>> } > > > > > >>>> > > > > > >>>> > > > > > >>>> This may look general for ProxyInvocationHandler but you get > the > > > > idea. > > > > > >>>> > > > > > >>>> WDYT? > > > > > >>>> > > > > > >>>> Thanks, > > > > > >>>> Manu > > > > > >>>> > > > > > >>>> > > > > > >>> -- > > > > > >>> Jean-Baptiste Onofré > > > > > >>> [email protected] > > > > > >>> http://blog.nanthrax.net > > > > > >>> Talend - http://www.talend.com > > > > > >>> > > > > > >>> > > > > > >> > > > > > > -- > > > > > > Jean-Baptiste Onofré > > > > > > [email protected] > > > > > > http://blog.nanthrax.net > > > > > > Talend - http://www.talend.com > > > > > > > > > > > > > > > > > > > > >
