In a sense, all runners (with perhaps the exception of the direct runner) are proxies for actual runners. In that sense, I think it makes just as much sense to say "I want the portable runner with job endpoint X" as to say "I want the flink runner with master Y." Saying "I want the Portable Runner" without specifying an endpoint should, however, be undefined (though we could argue that the direct runner would be a reasonable default).
On Thu, Apr 30, 2020 at 11:49 AM Ismaël Mejía <[email protected]> wrote: > Thomas has a point on the PortableRunner name, I was super confused > because of the `PortableRunner` not being a runner, I don't know if > too late but maybe it is still worth to give it a better name. > > On Thu, Apr 30, 2020 at 8:41 PM Thomas Weise <[email protected]> wrote: > > > > +1 for removing the default runner. It has always been the Beam user > expectation that a runner needs to be selected. > > > > "PortableRunner" isn't a runner (despite its name) - it's a proxy to a > runner that the user specifies via job_endpoint. > > > > Thanks for cleaning this up! > > > > On Thu, Apr 30, 2020 at 10:11 AM Kyle Weaver <[email protected]> > wrote: > >> > >> I'll bite :) Thanks for the feedback everyone! > >> > >> On Thu, Apr 30, 2020 at 1:01 PM Robert Bradshaw <[email protected]> > wrote: > >>> > >>> I filed https://issues.apache.org/jira/browse/BEAM-9860. Any takers? > >>> > >>> On Thu, Apr 30, 2020 at 5:49 AM Ismaël Mejía <[email protected]> > wrote: > >>>> > >>>> +1 for A there are zero reasons to have a default runner set by > >>>> default, being explicit is better as Robert suggests and it resolves > >>>> the confusion that the user reported. > >>>> > >>>> On Wed, Apr 29, 2020 at 10:05 PM Robert Bradshaw <[email protected]> > wrote: > >>>> > > >>>> > +1, I was actually thinking about this just the other day. > PortableRunner should require job_endpoint to be set, and we can have a > nice error message directing the explicit use of FlinkRunner for the old > behavior. > >>>> > > >>>> > On Wed, Apr 29, 2020 at 11:50 AM Kyle Weaver <[email protected]> > wrote: > >>>> >> > >>>> >> > Could the error message suggest switching to FlinkRunner (and/or > other runners that start a job server for you)? Then it seems like the > breakage would only be a minor annoyance. > >>>> >> > >>>> >> Definitely. > >>>> >> > >>>> >> On Wed, Apr 29, 2020 at 2:49 PM Brian Hulette <[email protected]> > wrote: > >>>> >>> > >>>> >>> Could the error message suggest switching to FlinkRunner (and/or > other runners that start a job server for you)? Then it seems like the > breakage would only be a minor annoyance. > >>>> >>> > >>>> >>> Brian > >>>> >>> > >>>> >>> On Wed, Apr 29, 2020 at 11:32 AM Kyle Weaver <[email protected]> > wrote: > >>>> >>>> > >>>> >>>> Hi all, > >>>> >>>> > >>>> >>>> Currently, when running a pipeline that has the options > runner=PortableRunner and job_endpoint unset, the Python SDK spins up a > Dockerized Flink job server [1]. This is problematic because the > PortableRunner can be used by any portable runner. So for example, a Spark > runner user was recently baffled when their job ran successfully but > printed a bunch of Flink log messages. > >>>> >>>> > >>>> >>>> There are not too many uses of this default behavior to my > knowledge, at least within Beam itself. The only example I could find was > in the portableWordCount tests, which is mostly the same as > portableWordCountFlinkRunner tests [2]. The default behavior is entirely > superseded by the FlinkRunner class, which provides better encapsulation. > >>>> >>>> > >>>> >>>> I also noticed that DockerizedJobServer is only used by [3]. In > FlinkRunner, we pull the job server from Maven if necessary and call Java > directly. In general, I think there are already quite enough knobs in the > portability framework, so we should remove it unless there is reason to > prefer running the job server with Docker instead of calling Java directly. > >>>> >>>> > >>>> >>>> There are a couple options: > >>>> >>>> > >>>> >>>> A) Remove the default behavior and require job_endpoint to > always be set when using PortableRunner. This would be a breaking change. > >>>> >>>> B) Keep the current behavior, but warn when the user sets > runner=PortableRunner without job_endpoint. This is easy to miss, but it's > better than nothing. > >>>> >>>> > >>>> >>>> What do you think? > >>>> >>>> > >>>> >>>> [1] > https://github.com/apache/beam/blob/33c73739cec8bc6a7c8319efa41eda7a2540bce1/sdks/python/apache_beam/runners/portability/job_server.py#L184 > >>>> >>>> [2] > https://github.com/apache/beam/blob/b3596b89dbc002c686bdaa7853074e757a81b6fb/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L1983-L2048 > >>>> >>>> [3] > https://github.com/apache/beam/blob/33c73739cec8bc6a7c8319efa41eda7a2540bce1/sdks/python/apache_beam/runners/portability/job_server.py#L163 >
