[BEAM-8165]Change default docker images name #9487

Hannah-Jiang · 2019-09-05T20:31:36Z

As we decided to add _sdk to image name, this PR is changing default image name for Python, Java and Go images.

Publishing docker images as part of release is tackled with following three PRs.

Change default image name. (current PR, [BEAM-8165]Change default docker images name #9487)
Add staging and publishing scripts for docker images to release procedure. ([BEAM-8105] Docker images release scripts #9506 )
Updating release_guide.md ([BEAM-8105] update release guide with docker images #9510)

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

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

Pre-Commit Tests Status (on master branch)

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

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

Hannah-Jiang · 2019-09-05T20:58:49Z

Run Python Dataflow ValidatesContainer

Hannah-Jiang · 2019-09-06T19:20:14Z

Run Python Dataflow ValidatesContainer

Hannah-Jiang · 2019-09-06T21:28:48Z

Run Python PreCommit

Hannah-Jiang · 2019-09-06T21:54:39Z

R: @aaltay
Can you please review or find someone else to review it? Thanks.

aaltay

LGTM modulo one open comment.

aaltay · 2019-09-07T01:50:46Z

sdks/python/apache_beam/runners/portability/portable_runner.py

-               '{version_suffix}:latest'.format(
-                   user=os.environ['USER'],
-                   version_suffix=version_suffix))
+      image = ('apachebeam/python{version_suffix}_sdk:latest'.format(


We may need to give people a way to run with their locally built containers for testing purposes, or for using custom containers.

We could reuse worker_harness_container_image flag, introduce a new flag. Or perhaps something else to do this in an easy way.

@tweise might have suggestions.

I have mixed feelings about this. When working with master, wouldn't I expect the locally built image to be used? What does it mean to refer to hub for something that wasn't released.

Perhaps do that depending on whether current version is dev/snapshot or a release?

And thanks for the ping @aaltay !

To build on Thomas's idea, we could do something similar to what dataflowrunner does today (

beam/sdks/python/apache_beam/runners/dataflow/internal/apiclient.py

Line 946 in 9043b3e

def _get_required_container_version(use_fnapi):

)

If this is a released version on release branch (i.e. no 'dev' in version') default to image in hub. (release manager will have to build this container as part of the release anyway.)

If it is a dev version --> this is a question. Dataflowrunner's solution is to have a fixed image that is occasionally updated as needed or if provided a flag use that as an override. (Here we might just simplify and require a locally built version. This occasionally built image is convenient but is error-prone and a hassle.)

Thanks @aaltay and @tweise for comments.

--environment_config option can be used to pass customized images when use Portable runner. The default image is used when no docker image is specified.

My understanding is, from users perspective, they don't need to worry about creating docker images at local when they write pipelines, so it's ok to pull it from remote. From developers perspective, developers who are working on sdks are expected to know how portable runner is working, so as how docker is used. If they want to test their developing sdks, they should build an image at local and use it. The image name can be same as the default one, then the local image is used instead of pulling it from remote, or can pass it with --environment_config option if it has different name.

I can add more comments here to explain how to pass customized images.

What scenarios am I missing?

When the Beam version is not a development version (a release), then by default use the image corresponding to that release. Not latest, because that could be anything and may not be compatible with the release version? If the Python Beam version is 2.16, then it should not pick up something that could be 2.17, for example.

If the version of Beam is a development version, continue to use the locally built image as before.

But if a locally built container would automatically be picked up, then the distinction won't be necessary. Is that already the case with these changes?

Ah, I see, it’s possible that Beam version is not up to date.
It will check local first, if the image is not available, them pull it from remote.

Hannah-Jiang · 2019-09-10T16:14:57Z

Run Python Dataflow ValidatesContainer

Hannah-Jiang · 2019-09-10T16:25:44Z

@aaltay , I fixed to use version as tag when pull default image. For quick workaround, I hardcoded version for docker tasks. Created a ticket for later improvement. https://issues.apache.org/jira/browse/BEAM-8192

tweise · 2019-09-10T16:32:06Z

The version is available as property project.version

aaltay · 2019-09-10T16:33:01Z

sdks/python/container/py2/build.gradle

@@ -51,8 +52,13 @@ task copyLauncherDependencies(type: Copy) {
  }
 }

+def sdk_version = '2.16.0'


This is probably not a good idea. The reason is:

This needs to match the https://github.com/apache/beam/blob/master/sdks/python/apache_beam/version.py - So it needs to be 2.16.0.dev here

Release process needs to be updated to change these versions along with other versions. Release process automatically changes the version in the release branch to remove the ".dev" part and changes master branch to the next version.

If we have to do this, at least is it possible to put this in a single gradle file?

Another idea, we can execute or parse https://github.com/apache/beam/blob/master/sdks/python/apache_beam/version.py and extract version from here. That should be possible with gradle.

@yifanzou could you help with this?
/cc @markflyhigh as the release manager.

It would also be good to retain the distinction between -SNAPSHOT/.dev version and release version in the container tag. It can lead to confusing results when a locally built container from snapshot source overrides the image of a release.

+1 on not adding another version in this gradle file. Like Ahmet mentioned, we define the version number in the version.py, and it would be a problem to make the version# in different places being constant. Gradle script uses groovy, which has power to interact with files, https://discuss.gradle.org/t/read-project-version-number-dynamically-from-a-file/22607. Hopefully this helps.

https://docs.gradle.org/current/dsl/org.gradle.api.resources.TextResource.html

Thanks for reviewing and comments.
I wanted to make sdk_version consistent with the one from version.py, forgot to change it back after testing. Thanks for catching it.

After evaluating many options, I think introducing a new variable sdk_version to gradle.properties is the easiest way for now. This version will be consistent to the one from version.py, so we need to update it when we release it. It would be better to read it from version.py and assign to sdk_version, however, as far as I know, gradle.properties is not supposed to read variables from another file. If it's possible, I am happy to try.

I made a commit with this change, can you please review and make comments?

Thanks,
Hannah

aaltay · 2019-09-10T16:48:02Z

project.version does not match Python version in master branch. It will be 2.16.0-SNAPSHOT vs 2.16.0.dev. I am guessing we can still use it and with a simple string raplace change '-SNAPSHOT' to '.dev' for python.

Hannah-Jiang · 2019-09-10T16:50:47Z

The version is available as property project.version

I investigated it, this would be the best way to go with, but this version is always {version}-SNAPSHOT, both for released and developing version. How do we know which tag we should use between {version}, {version}.dev and {version}-SNAPSHOT?

tweise · 2019-09-10T16:56:05Z

It is indeed very odd that the version is set to -SNAPSHOT in release tags: https://github.com/apache/beam/blob/v2.15.0/gradle.properties#L26

tweise · 2019-09-10T17:00:03Z

Versions changes are applied partially: c071613

Hannah-Jiang · 2019-09-10T20:37:36Z

project.version does not match Python version in master branch. It will be 2.16.0-SNAPSHOT vs 2.16.0.dev. I am guessing we can still use it and with a simple string raplace change '-SNAPSHOT' to '.dev' for python.

I also investigated this option during weekend and there is a scenarios we cannot support with string replacement.
For released versions, it would be {version}-SNAPSHOT at gradle.properties, but {version} at version.py. So if a user build an image from released version, it would be tagged with {version}.dev, however, an image with {version} tag will be used as a default image, which would make difficult to investigate for users who are not familiar with how tagging works with Beam.
This is the same issue as I mentioned previously, given {version}-SNAPSHOT, we cannot tell if it's developing version or released version. I also tried to change {version}-SNAPSHOT to {version} for released versions, and {version}.dev for developing versions, but 2.16 cutoff date is so close and the impact with the change would be wide, so I decided to introduce a new variable for now and make the change later.

We have the version defined at several different locations and suffixes are not consistent, which should be improved. I changed scope of the Jira ticket to include this work.

aaltay · 2019-09-11T02:26:53Z

OK. I am fine with this change. I will only call 'sdk_version', 'python_sdk_version', since that is only applicable to python. And let's plan to clean it after 2.16. - LGTM.

@tweise - What do you think?

Hannah-Jiang · 2019-09-11T03:59:09Z

OK. I am fine with this change. I will only call 'sdk_version', 'python_sdk_version', since that is only applicable to python. And let's plan to clean it after 2.16. - LGTM.

Thanks, I addressed your comment.

tweise · 2019-09-11T12:45:41Z

I think it is reasonable to go with this for 2.16 and work on a long term solution in master as follow-up.

Hannah-Jiang · 2019-09-11T14:48:15Z

Run Java PreCommit

Hannah-Jiang · 2019-09-11T14:48:52Z

Run CommunityMetrics PreCommit

Hannah-Jiang · 2019-09-11T14:50:11Z

Run Python Dataflow ValidatesContainer

Hannah-Jiang · 2019-09-11T16:33:31Z

Can we merge it?

Hannah-Jiang · 2019-09-11T16:55:43Z

Thank you @aaltay .

ibzib · 2019-09-18T17:55:31Z

@Hannah-Jiang Could you update https://github.com/apache/beam/blob/master/sdks/CONTAINERS.md to reflect these changes?

Hannah-Jiang · 2019-09-18T18:16:29Z

@Hannah-Jiang Could you update https://github.com/apache/beam/blob/master/sdks/CONTAINERS.md to reflect these changes?

We are already working on it, a tech writer is on it and it will be released as part of 2.16.

Hannah-Jiang changed the title ~~[WIP] Add docker images to release~~ [WIP] Change default docker images name Sep 5, 2019

Hannah-Jiang changed the title ~~[WIP] Change default docker images name~~ [WIP] [BEAM-6185]Change default docker images name Sep 6, 2019

[BEAM-8105] change default docker image name

473377e

Hannah-Jiang force-pushed the add_containers_to_release branch from 294f466 to 473377e Compare September 6, 2019 19:19

Hannah-Jiang mentioned this pull request Sep 6, 2019

[BEAM-8105] Docker images release scripts #9506

Merged

3 tasks

Hannah-Jiang changed the title ~~[WIP] [BEAM-6185]Change default docker images name~~ [BEAM-6185]Change default docker images name Sep 6, 2019

Hannah-Jiang mentioned this pull request Sep 6, 2019

[BEAM-8105] update release guide with docker images #9510

Merged

3 tasks

aaltay reviewed Sep 7, 2019

View reviewed changes

Hannah-Jiang changed the title ~~[BEAM-6185]Change default docker images name~~ [BEAM-8165]Change default docker images name Sep 9, 2019

Hannah-Jiang force-pushed the add_containers_to_release branch 9 times, most recently from e1f9f7a to e86f1bf Compare September 10, 2019 16:13

aaltay reviewed Sep 10, 2019

View reviewed changes

Hannah-Jiang force-pushed the add_containers_to_release branch from e86f1bf to 482017f Compare September 10, 2019 20:14

Hannah-Jiang force-pushed the add_containers_to_release branch 2 times, most recently from cb9fb90 to 273b4c7 Compare September 10, 2019 20:53

[BEAM-8165] read version to set tag for default docker image

53f069d

Hannah-Jiang force-pushed the add_containers_to_release branch from 273b4c7 to 53f069d Compare September 11, 2019 03:58

aaltay approved these changes Sep 11, 2019

View reviewed changes

aaltay merged commit 8790822 into apache:master Sep 11, 2019

kamilwu mentioned this pull request Sep 12, 2019

[BEAM-8178] Fix broken Load Tests running on Flink #9521

Merged

3 tasks

ibzib mentioned this pull request Sep 19, 2019

[BEAM-8164][website] Correct document for building the python SDK ha… #9536

Merged

[BEAM-8165]Change default docker images name #9487

[BEAM-8165]Change default docker images name #9487

Conversation

Hannah-Jiang commented Sep 5, 2019 • edited Loading

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Hannah-Jiang commented Sep 5, 2019

Hannah-Jiang commented Sep 6, 2019

Hannah-Jiang commented Sep 6, 2019

Hannah-Jiang commented Sep 6, 2019

aaltay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hannah-Jiang commented Sep 10, 2019

Hannah-Jiang commented Sep 10, 2019

tweise commented Sep 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifanzou Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaltay commented Sep 10, 2019

Hannah-Jiang commented Sep 10, 2019

tweise commented Sep 10, 2019

tweise commented Sep 10, 2019

Hannah-Jiang commented Sep 10, 2019

aaltay commented Sep 11, 2019

Hannah-Jiang commented Sep 11, 2019 • edited Loading

tweise commented Sep 11, 2019

Hannah-Jiang commented Sep 11, 2019

Hannah-Jiang commented Sep 11, 2019

Hannah-Jiang commented Sep 11, 2019

Hannah-Jiang commented Sep 11, 2019

Hannah-Jiang commented Sep 11, 2019

ibzib commented Sep 18, 2019

Hannah-Jiang commented Sep 18, 2019 • edited Loading

Hannah-Jiang commented Sep 5, 2019 •

edited

Loading

yifanzou Sep 10, 2019 •

edited

Loading

Hannah-Jiang commented Sep 11, 2019 •

edited

Loading

Hannah-Jiang commented Sep 18, 2019 •

edited

Loading