The PR for this is up now: https://github.com/apache/beam/pull/12481
Any volunteers to help review? We may want a separate reviewer for Python
and Java changes.
Brian
On Wed, Aug 5, 2020 at 9:00 AM Brian Hulette <bhule...@google.com> wrote:
> What I'm working on changes ExternalConfigurationPayload [1] to this:
>
> message ExternalConfigurationPayload {
> // A schema for use in beam:coder:row:v1
> Schema schema = 1;
>
> // A payload which can be decoded using beam:coder:row:v1 and the given
> schema.
> bytes payload = 2;
> }
>
> The calling SDK can infer a schema from a user configuration type (in
> Python we can make minor changes to SchemaBasedPayloadBuilder for this),
> and use it's implementation of beam:coder:row:v1 to encode an instance of
> that type to the payload.
>
> Similarly the expanding SDK can infer a schema from a user configuration
> type and map the encoded row to an instance of the user type, assuming the
> schemas are compatible.
>
> Brian
>
> [1]
> https://github.com/apache/beam/blob/86b8326b4ebc4e217585847102743cc1d1af367a/model/pipeline/src/main/proto/external_transforms.proto#L42
>
> On Wed, Aug 5, 2020 at 2:04 AM Maximilian Michels <m...@apache.org> wrote:
>
>> +1
>>
>> The format to store coders is not set in stone, it was a first version
>> to make external configuration work. Using the Coder message would be
>> better.
>>
>> As for using Schema to store the configuration, could somebody fill me
>> in how that would work?
>>
>> -Max
>>
>> On 04.08.20 02:01, Brian Hulette wrote:
>> > I've opened BEAM-10571 [1] for this, and I'm most of the way to an
>> > implementation now. Aiming to have it done before the 2.24.0 cut since
>> > it will be the last release with python 2 support.
>> >
>> > [1] https://issues.apache.org/jira/browse/BEAM-10571
>> >
>> > On Wed, Jul 15, 2020 at 9:03 AM Chamikara Jayalath <
>> chamik...@google.com
>> > <mailto:chamik...@google.com>> wrote:
>> >
>> >
>> >
>> > On Fri, Jul 10, 2020 at 4:47 PM Robert Bradshaw <
>> rober...@google.com
>> > <mailto:rober...@google.com>> wrote:
>> >
>> > On Fri, Jul 10, 2020 at 4:36 PM Brian Hulette
>> > <bhule...@google.com <mailto:bhule...@google.com>> wrote:
>> > >
>> > > Ah yes I'm +1 for that approach too - it would let us
>> > leverage all the schema-inference already in the Java SDK for
>> > translating configuration objects which would be great.
>> > > Things on the Python side would be trickier as schemas don't
>> > formally support all the types you can use in the PayloadBuilder
>> > implementations [1] yet, just NamedTuple. For now we could just
>> > make the PayloadBuilder implementations generate Rows without
>> > making that translation available for use in PCollections.
>> >
>> >
>> > This will be a good opportunity to add some sort of a minimal Python
>> > type to Beam schema mapping :)
>> >
>> >
>> > Yes, though eventually it might be nice to support all of these
>> > various types as schema'd PCollection elements as well.
>> >
>> > > Do we need to worry about update compatibility for
>> > ExternalConfigurationPayload?
>> >
>> > Technically, each URN defines their payload, and the fact that
>> we've
>> > settled on ExternalConfigurationPayload is a convention. On a
>> > practical note, we haven't declared these protos stable yet. (I
>> > would
>> > like to do so before we drop support for Python 2, as external
>> > transforms are a possible escape hatch and the first strong
>> > motivation
>> > to have external transforms that span Beam versions).
>> >
>> >
>> > +1
>> >
>> >
>> > > [1]
>> >
>> https://github.com/apache/beam/blob/master/sdks/python/apache_beam/transforms/external.py
>> > >
>> > > On Fri, Jul 10, 2020 at 4:23 PM Robert Bradshaw
>> > <rober...@google.com <mailto:rober...@google.com>> wrote:
>> > >>
>> > >> I would be in favor of just using a schema to store the
>> entire
>> > >> configuration. The reason we went with what we have to day
>> > is that we
>> > >> didn't have cross language schemas yet.
>> > >>
>> > >> On Fri, Jul 10, 2020 at 12:24 PM Brian Hulette
>> > <bhule...@google.com <mailto:bhule...@google.com>> wrote:
>> > >> >
>> > >> > Hi everyone,
>> > >> > I noticed that currently the ExternalConfigurationPayload
>> > uses a list of coder URNs to represent the coder that was used
>> > to serialize each configuration field [1]. This seems acceptable
>> > at first blush, but there's one notable issue: it has no place
>> > to store a payload for the coder. Most standard coders don't use
>> > a payload so it's not a problem, but row coder does use a
>> > payload to store it's schema, which means it can't be used in an
>> > ExternalConfigurationPayload today.
>> > >> >
>> > >> > Is there a reason not to just use the Coder message [2] in
>> > ExternalConfigurationPayload instead of a list of coder URNs?
>> > That would work with row coder, and it would also make it easier
>> > to re-use logic for translating Pipeline protos.
>> > >> >
>> > >> > I'd be happy to make this change, but I wanted to ask on
>> > dev@ in case there's something I'm missing here.
>> > >> >
>> > >> > Brian
>> > >> >
>> > >> > [1]
>> >
>> https://github.com/apache/beam/blob/c54a0b7f49f2eb4a15df115205e2fa455116ccbe/model/pipeline/src/main/proto/external_transforms.proto#L34-L35
>> > >> > [2]
>> >
>> https://github.com/apache/beam/blob/c54a0b7f49f2eb4a15df115205e2fa455116ccbe/model/pipeline/src/main/proto/beam_runner_api.proto#L542-L555
>> >
>>
>
