Skip to content

[BEAM-8348] set job_name in portable_runner.py job request#9789

Closed
ibzib wants to merge 2 commits intoapache:masterfrom
ibzib:job_name
Closed

[BEAM-8348] set job_name in portable_runner.py job request#9789
ibzib wants to merge 2 commits intoapache:masterfrom
ibzib:job_name

Conversation

@ibzib
Copy link

@ibzib ibzib commented Oct 14, 2019

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@ibzib ibzib requested review from angoenka and tweise October 14, 2019 20:26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GoogleCloudOptions doesn't seem appropriate for this. Should we relocate the option?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was initially my preferred approach, but I don't see a way to do it without breaking all the existing Dataflow pipelines that set a job name. See #9724

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to replicate the option and deprecate it in GoogleCloudOptions?

FWIW this option also exists in the Flink runner and the job_name is already reflected in the pipeline (see #9752 (comment)), so what is the effect of this change going to be?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PipelineOptions does not allow the same option to belong to multiple subclasses.

This PR changes job_name in the job request, not the pipeline options. e.g. here https://github.com/apache/beam/blob/master/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/jobsubmission/InMemoryJobService.java#L124

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, what is the user facing impact?

Regarding PipelineOptions, can it not be changed to allow for the co-existence of job_name in both classes for a transition period? @robertwb

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could add an explicit @property in GoogleCloudOptions that delegates to the one in PipelineOptions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea. Unfortunately, it looks like the override of __settatr__ would take precedence over the property's setter. I'm not too familiar with how all these Python internals work, though, so I could be missing something obvious?

def __setattr__(self, name, value):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. Perhaps override _visible_option_list?

@ibzib ibzib changed the title [BEAM-8348] set job_name in portable_runner.py [BEAM-8348] set job_name in portable_runner.py job request Oct 15, 2019
@robertwb
Copy link
Contributor

robertwb commented Nov 6, 2019

Any update on this?

@ibzib
Copy link
Author

ibzib commented Nov 6, 2019

I haven't made any progress on this. It's not a terribly important feature.

@stale
Copy link

stale bot commented Jan 5, 2020

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label Jan 5, 2020
@ibzib ibzib closed this Jan 11, 2020
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants