Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SDK harness container for FnAPI jobs when worker_harness_containe… #3434

Closed
wants to merge 15 commits into from

Conversation

tvalentyn
Copy link
Contributor

…r_image is not specified. By default the image tag corresponds to the version of the release, same as for legacy images.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify.
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

…r_image is not specified. By default the image tag corresponds to the version of the release, same as for legacy images.
@tvalentyn
Copy link
Contributor Author

R: @aaltay

@aaltay
Copy link
Member

aaltay commented Jun 23, 2017

R: @charlesccychen

get_required_container_version is the right place to update.

@charlesccychen
Copy link
Contributor

Can you please update dependency.py as Ahmet suggested? Thanks!

…r_image is not specified. By default the image tag corresponds to the version of the release, same as for legacy images.
@tvalentyn
Copy link
Contributor Author

Thanks, I moved the logic to dependency.py. I think get_required_container_version is currently meant to return just the version(tag) of the image, so I left it as is and added another function.
PTAL

@aaltay
Copy link
Member

aaltay commented Jun 28, 2017

@tvalentyn There is a lint error. Also could you please rebase your change?

…r_image is not specified. By default the image tag corresponds to the version of the release, same as for legacy images.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 70.879% when pulling 3ffab61 on tvalentyn:default_container_name into 0b19fb4 on apache:master.

@tvalentyn
Copy link
Contributor Author

@aaltay Thanks for heads-up. PTAL.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 70.896% when pulling 6360318 on tvalentyn:default_container_name into 0b19fb4 on apache:master.

@aaltay
Copy link
Member

aaltay commented Jun 28, 2017

Tests are failing. Could you look at that?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 70.874% when pulling 4232943 on tvalentyn:default_container_name into 16f8000 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 70.877% when pulling 4232943 on tvalentyn:default_container_name into 16f8000 on apache:master.

@tvalentyn
Copy link
Contributor Author

Thanks, my bad. PTAL

Copy link
Contributor

@charlesccychen charlesccychen left a comment

Choose a reason for hiding this comment

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

Can you also please squash the messy commit tree here so that it is one commit on top of master?

"""For internal use only; no backwards-compatibility guarantees.

Returns the Google Cloud Dataflow container version for remote execution.
Args:
job_type: string, BEAM job type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow this style guide for structured pydocs when specifying argument / return types: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html (see the other possible style guides here: https://stackoverflow.com/questions/34331088/how-to-comment-parameters-for-pydoc)

Returns:
string, Google Cloud Dataflow container image for remote execution.
"""
if job_type == 'FNAPI_BATCH' or job_type == 'FNAPI_STREAMING':
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add TODO to refactor job types into an enum-like class.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 70.874% when pulling e6115f0 on tvalentyn:default_container_name into 16f8000 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 70.884% when pulling e6115f0 on tvalentyn:default_container_name into 16f8000 on apache:master.

@tvalentyn
Copy link
Contributor Author

Let's drop this in favor of #3468

@tvalentyn tvalentyn closed this Jun 29, 2017
@tvalentyn tvalentyn deleted the default_container_name branch October 23, 2019 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants