On Mon, Oct 15, 2018 at 1:17 PM Robert Bradshaw <rober...@google.com> wrote:
> On Mon, Oct 15, 2018 at 7:50 PM Lukasz Cwik <lc...@google.com> wrote:
> >
> > I agree with the sentiment for better error checking.
> >
> > We can try to make it such that the SDK can "fetch" the set of options
> that the runner supports by making a call to the Job API. The API could
> return a list of option names (descriptions for --help purposes and also
> potentially the expected format) which would remove the worry around
> "unknown" options. Yes I understand to be able to make the Job API call, we
> may need to parse some options from the args parameters first and then
> parse the unknown options after they are fetched.
>
> This is an interesting idea, but seems it could get quite complicated.
> E.g. for delegating runners, one would first read the options to
> determine which runner to fetch the options from, which would then
> return a set of options that possibly depends on the values of some of
> its options...
>
> > Alternatively, we can choose an explicit format upfront.
> > To expand on the exact format for --runner_option=..., here are some
> different ideas:
> > 1) Specified multiple times, each one is an explicit flag
> > --runner_option=--blah=bar --runner_option=--foo=baz1
> --runner_option=--foo=baz2
>
> I'm -1 on this format. We should move away from the idea that options
> == flags (as that doesn't compose well with other libraries that do
> their own flags parsing). The ability to parse a set of flags into
> options is just a convenience that an author may (or may not) choose
> to use (e.g. when running pipelines a long-lived process like a
> service or a notebook, the command line flags are almost certainly not
> the right interface).
>
> > 2) specified multiple times, we drop the explicit flag
> > --runner_option=blah=bar --runner_option=foo=baz1
> --runner_option=foo=baz2
>
> This or (4) is my preference.
>
> > 3) we use a string which the runner can choose to interpret however they
> want (JSON/XML shown below)
> > --runner_option='{"blah": "bar", "foo": ["baz1", "baz2"]}'
> >
> --runner_option='<options><blah>bar</blah><foo>baz1</foo><foo>baz2</foo></options>'
>
> This would make validation hard. Also, I think it makes sense for some
> runner options to be "shared" (parallelism") by convention, so letting
> it be a free-form string wouldn't allow different runners to inspect
> different bits.
>
> We should consider if we should use urns for namespacing, and
> assigning semantic meaning to strings, here.
>
> > 4) we use a string which must be a specific format such as JSON (allows
> the SDK to do simple validation):
> > --runner_option='{"blah": "bar", "foo": ["baz1", "baz2"]}'
>
> I like this in that at least some validation can be performed, and
> expectations of how to format richer types. On the other hand it gets
> a bit verbose, given that most (I'd imagine) options will be simple.
> As with normal options,
>
> --option1=value1 --option2=value2
>
> is shorthand for {"option1": value1, "option2": value2}.
>
> I lean to 4 the most. With 2, you run into issues of what does
--runner_option=foo=["a", "b"] --runner_option=foo=["c", "d"] mean?
Is it an error or list of lists or concatenated. Similar issues for map
types represented via JSON object {...}
> > I would strongly suggest that we go with the "fetch" approach, since
> this makes the set of options discoverable and helps users find errors much
> earlier in their pipeline.
>
> This seems like an advanced feature that SDKs may want to support, but
> I wouldn't want to require this complexity for bootstrapping an SDK.
>
> SDKs that are starting off wouldn't need to "fetch" options, they could
choose to not support runner options or they could choose to pass all
options through to the runner blindly. Fetching the options only provides
the SDK the ability to provide error checking upfront and useful error/help
messages.
> Regarding always keeping runner options separate, +1, though I'm not
> sure the line is always clear.
>
>
> > On Mon, Oct 15, 2018 at 8:04 AM Robert Bradshaw <rober...@google.com>
> wrote:
> >>
> >> On Mon, Oct 15, 2018 at 3:58 PM Maximilian Michels <m...@apache.org>
> wrote:
> >> >
> >> > I agree that the current approach breaks the pipeline options contract
> >> > because "unknown" options get parsed in the same way as options which
> >> > have been defined by the user.
> >>
> >> FWIW, I think we're already breaking this "contract." Unknown options
> >> are silently ignored; with this change we just change how we record
> >> them. It still feels a bit hacky though.
> >>
> >> > I'm not sure the `experiments` flag works for us. AFAIK it only allows
> >> > true/false flags. We want to pass all types of pipeline options to the
> >> > Runner.
> >>
> >> Experiments is an arbitrary set of strings, which can be of the form
> >> "param=value" if that's useful. (Dataflow does this.) There is, again,
> >> no namespacing on the param names, but we could user urns or impose
> >> some other structure here.
> >>
> >> > How to solve this?
> >> >
> >> > 1) Add all options of all Runners to each SDK
> >> > We added some of the FlinkRunner options to the Python SDK but
> realized
> >> > syncing is rather cumbersome in the long term. However, we want the
> most
> >> > important options to be validated on the client side.
> >>
> >> I don't think this is sustainable in the long run. However, thinking
> >> about this, in the worse case validation happens after construction
> >> but before execution (as with much of our other validation) so it
> >> isn't that bad.
> >>
> >> > 2) Pass "unknown" options via a separate list in the Proto which can
> >> > only be accessed internally by the Runners. This still allows passing
> >> > arbitrary options but we wouldn't leak unknown options and display
> them
> >> > as top-level options.
> >>
> >> I think there needs to be a way for the user to communicate values
> >> directly to the runner regardless of the SDK. My preference would be
> >> to make this explicit, e.g. (repeated) --runner_option=..., rather
> >> than scooping up all unknown flags at command line parsing time.
> >> Perhaps an SDK that is aware of some runners could choose to lift
> >> these as top-level options, but still pass them as runner options.
> >>
> >> > On 13.10.18 02:34, Charles Chen wrote:
> >> > > The current release branch
> >> > > (https://github.com/apache/beam/commits/release-2.8.0) was cut
> after the
> >> > > revert went in. Sent out https://github.com/apache/beam/pull/6683
> as a
> >> > > revert of the revert. Regarding your comment above, I can help out
> with
> >> > > the design / PR reviews for common Python code as you suggest.
> >> > >
> >> > > On Fri, Oct 12, 2018 at 4:48 PM Thomas Weise <t...@apache.org
> >> > > <mailto:t...@apache.org>> wrote:
> >> > >
> >> > > Thanks, will tag you and looking forward to feedback so we can
> >> > > ensure that changes work for everyone.
> >> > >
> >> > > Looking at the PR, I see agreement from Max to revert the
> change on
> >> > > the release branch, but not in master. Would you mind to
> restore it
> >> > > in master?
> >> > >
> >> > > Thanks
> >> > >
> >> > > On Fri, Oct 12, 2018 at 4:40 PM Ahmet Altay <al...@google.com
> >> > > <mailto:al...@google.com>> wrote:
> >> > >
> >> > >
> >> > >
> >> > > On Fri, Oct 12, 2018 at 11:31 AM, Charles Chen <
> c...@google.com
> >> > > <mailto:c...@google.com>> wrote:
> >> > >
> >> > > What I mean is that a user may find that it works for
> them
> >> > > to pass "--myarg blah" and access it as "options.myarg"
> >> > > without explicitly defining a "my_arg" flag due to the
> added
> >> > > logic. This is not the intended behavior and we may
> want to
> >> > > change this implementation detail in the future.
> However,
> >> > > having this logic in a released version makes it hard to
> >> > > change this behavior since users may erroneously depend
> on
> >> > > this undocumented behavior. Instead, we should
> namespace /
> >> > > scope this so that it is obvious that this is meant for
> >> > > runner (and not Beam user) consumption.
> >> > >
> >> > > On Fri, Oct 12, 2018 at 10:48 AM Thomas Weise
> >> > > <t...@apache.org <mailto:t...@apache.org>> wrote:
> >> > >
> >> > > Can you please elaborate more what practical
> problems
> >> > > this introduces for users?
> >> > >
> >> > > I can see that this change allows a user to specify
> a
> >> > > runner specific option, which in the future may
> change
> >> > > because we decide to scope differently. If this only
> >> > > affects users of the portable Flink runner (like
> us),
> >> > > then no need to revert, because at this early stage
> we
> >> > > prefer something that works over being blocked.
> >> > >
> >> > > It would also be really great if some of the core
> Python
> >> > > SDK developers could help out with the design
> aspects
> >> > > and PR reviews of changes that affect common Python
> >> > > code. Anyone who specifically wants to be tagged on
> >> > > relevant JIRAs and PRs?
> >> > >
> >> > >
> >> > > I would be happy to be tagged, and I can also help with
> >> > > including other relevant folks whenever possible. In
> general I
> >> > > think Robert, Charles, myself are good candidates.
> >> > >
> >> > >
> >> > > Thanks
> >> > >
> >> > >
> >> > > On Fri, Oct 12, 2018 at 10:20 AM Ahmet Altay
> >> > > <al...@google.com <mailto:al...@google.com>> wrote:
> >> > >
> >> > >
> >> > >
> >> > > On Fri, Oct 12, 2018 at 10:11 AM, Charles Chen
> >> > > <c...@google.com <mailto:c...@google.com>> wrote:
> >> > >
> >> > > For context, I made comments on
> >> > > https://github.com/apache/beam/pull/6600
> noting
> >> > > that the changes being made were not good
> for
> >> > > Beam backwards-compatibility. The change
> as is
> >> > > allows users to use pipeline options without
> >> > > explicitly defining them, which is not the
> type
> >> > > of usage we would like to encourage since we
> >> > > prefer to be explicit whenever possible. If
> >> > > users write pipelines with this sort of
> pattern,
> >> > > they will potentially encounter pain when
> >> > > upgrading to a later version since this is
> an
> >> > > implementation detail and not an officially
> >> > > supported pattern. I agree with the
> comments
> >> > > above that this is ultimately a scoping
> issue.
> >> > > I would not have a problem with these
> changes if
> >> > > they were explicitly scoped under either a
> >> > > runner or unparsed options namespace.
> >> > >
> >> > > As a second note, since the 2.8.0 release is
> >> > > being cut right now, because of these
> >> > > backwards-compatibility concerns, I would
> >> > > suggest reverting these changes, at least
> until
> >> > > 2.8.0 is cut, so we can have a discussion
> here
> >> > > before committing to and releasing any
> API-level
> >> > > changes.
> >> > >
> >> > >
> >> > > +1 I would like to revert the changes in order
> not
> >> > > rush this into the release. Once this discussion
> >> > > results in an agreement changes can be brought
> back.
> >> > >
> >> > >
> >> > > On Fri, Oct 12, 2018 at 9:26 AM Henning
> Rohde
> >> > > <hero...@google.com <mailto:
> hero...@google.com>>
> >> > > wrote:
> >> > >
> >> > > Agree that pipeline options lack some
> >> > > mechanism for scoping. It is also not
> always
> >> > > possible distinguish options meant to be
> >> > > consumed at pipeline construction time,
> by
> >> > > the runner, by the SDK harness, by the
> user
> >> > > code or any combination -- and this
> causes
> >> > > confusion every now and then.
> >> > >
> >> > > For Dataflow, we have been using
> >> > > "experiments" for arbitrary
> runner-specific
> >> > > options. It's simply a string list
> pipeline
> >> > > option that all SDKs support and, for
> Go at
> >> > > least, is sent to portable runners.
> Flink
> >> > > can do the same in the short term to
> move
> >> > > forward.
> >> > >
> >> > > Henning
> >> > >
> >> > >
> >> > > On Fri, Oct 12, 2018 at 8:50 AM Thomas
> Weise
> >> > > <t...@apache.org <mailto:t...@apache.org>>
> wrote:
> >> > >
> >> > > [moving to the list]
> >> > >
> >> > > The requirement driving this part
> of the
> >> > > change was to allow a user to
> specify
> >> > > pipeline options that a runner
> supports
> >> > > without having to declare those in
> each
> >> > > language SDK.
> >> > >
> >> > > In the specific scenario, we have
> >> > > options that the Flink runner
> supports
> >> > > (and can validate), that are not
> >> > > enumerated in the Python SDK.
> >> > >
> >> > > I think we have a bigger problem
> scoping
> >> > > pipeline options. For example, the
> >> > > runner options are dumped into the
> SDK
> >> > > worker. There is also a possibility
> of
> >> > > name collisions. So I think this
> would
> >> > > benefit from broader feedback.
> >> > >
> >> > > Thanks,
> >> > > Thomas
> >> > >
> >> > >
> >> > > ---------- Forwarded message
> ---------
> >> > > From: *Charles Chen*
> >> > > <notificati...@github.com
> >> > > <mailto:notificati...@github.com>>
> >> > > Date: Fri, Oct 12, 2018 at 8:36 AM
> >> > > Subject: Re: [apache/beam]
> [BEAM-5442]
> >> > > Store duplicate unknown options in a
> >> > > list argument (#6600)
> >> > > To: apache/beam <
> b...@noreply.github.com
> >> > > <mailto:b...@noreply.github.com>>
> >> > > Cc: Thomas Weise <
> thomas.we...@gmail.com
> >> > > <mailto:thomas.we...@gmail.com>>,
> >> > > Mention <ment...@noreply.github.com
> >> > > <mailto:ment...@noreply.github.com
> >>
> >> > >
> >> > >
> >> > > CC: @tweise <
> https://github.com/tweise>
> >> > >
> >> > > —
> >> > > You are receiving this because you
> were
> >> > > mentioned.
> >> > > Reply to this email directly, view
> it on
> >> > > GitHub
> >> > > <
> https://github.com/apache/beam/pull/6600#issuecomment-429367754>,
> >> > > or mute the thread
> >> > > <
> https://github.com/notifications/unsubscribe-auth/AAQGDwwt15R85eq9pySUisyxq2HYz-Vyks5ukLcLgaJpZM4XMo-T
> >.
> >> > >
> >> > >
> >> > >
>
