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

[BEAM-12411] Update Tensorflow to version 2.5.0 & grpcio to 1.34.0 #14888

Merged
merged 3 commits into from
Jun 10, 2021

Conversation

aaltay
Copy link
Member

@aaltay aaltay commented May 26, 2021

Please add a meaningful description for your change here


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.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

ValidatesRunner compliance status (on master branch)

Lang ULR Dataflow Flink Samza Spark Twister2
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
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 ---
XLang Build Status Build Status Build Status --- Build Status ---

Examples testing status on various runners

Lang ULR Dataflow Flink Samza Spark Twister2
Go --- --- --- --- --- --- ---
Java --- Build Status
Build Status
Build Status
--- --- --- --- ---
Python --- --- --- --- --- --- ---
XLang --- --- --- --- --- --- ---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go Java Python
Build Status Build Status Build Status
Build Status
Build Status

Pre-Commit Tests Status (on master branch)

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

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

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@aaltay aaltay requested a review from iemejia May 26, 2021 16:22
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #14888 (81808d1) into master (79c1932) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14888      +/-   ##
==========================================
- Coverage   83.78%   83.75%   -0.04%     
==========================================
  Files         434      436       +2     
  Lines       58260    58491     +231     
==========================================
+ Hits        48816    48991     +175     
- Misses       9444     9500      +56     
Impacted Files Coverage Δ
...eam/runners/interactive/caching/streaming_cache.py
...s/python/apache_beam/typehints/sharded_key_type.py
.../srcs/sdks/python/apache_beam/coders/coder_impl.py
...m/testing/benchmarks/nexmark/models/auction_bid.py
...n/apache_beam/runners/dataflow/dataflow_metrics.py
...y38/build/srcs/sdks/python/apache_beam/io/utils.py
...he_beam/testing/benchmarks/nexmark/nexmark_perf.py
.../python/apache_beam/io/gcp/resource_identifiers.py
...s/sdks/python/apache_beam/examples/avro_bitcoin.py
...e_beam/io/gcp/internal/clients/storage/__init__.py
... and 860 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79c1932...81808d1. Read the comment docs.

@aaltay
Copy link
Member Author

aaltay commented May 26, 2021

Run Portable_Python PreCommit

@aaltay
Copy link
Member Author

aaltay commented May 26, 2021

Run PythonDocker PreCommit

@iemejia iemejia changed the title Update tensorflow to the latest version [BEAM-12411] Update Tensorflow to version 2.5.0 May 26, 2021
@iemejia
Copy link
Member

iemejia commented May 26, 2021

@aaltay It seems this requires to update also the grpcio version as a requisite too.

@aaltay
Copy link
Member Author

aaltay commented May 26, 2021

@aaltay It seems this requires to update also the grpcio version as a requisite too.

You are right. Updated that too.

@aaltay aaltay changed the title [BEAM-12411] Update Tensorflow to version 2.5.0 [BEAM-12411] Update Tensorflow to version 2.5.0 & grpcio to 1.34.0 May 26, 2021
@aaltay
Copy link
Member Author

aaltay commented May 26, 2021

Failing with:

13:55:01 RuntimeError: Could not retrieve licences for packages ['keras-nightly'] in Python3.8 environment. 
13:55:01  These licenses were not able to be pulled automatically.

Not sure how this change cause us to depend on keras-nightly. (@yifanmai - Do you have any idea?)

@iemejia - Changing to 2.4.2 might be easier but that is not a released version yet.

@yifanmai
Copy link
Contributor

That is strange because I'd expect only tf-nightly to depend on keras-nightly.

@aaltay
Copy link
Member Author

aaltay commented Jun 4, 2021

Run Python PreCommit

@aaltay
Copy link
Member Author

aaltay commented Jun 4, 2021

That is strange because I'd expect only tf-nightly to depend on keras-nightly.

It turns out tensorflow==2.5.0 depends on keras-nightly ~=2.5.0.dev. I can reproduce it in a clean virtual environment. I will file a bug.

It was added here: tensorflow/tensorflow@d171d94

@iemejia
Copy link
Member

iemejia commented Jun 8, 2021

That is strange because I'd expect only tf-nightly to depend on keras-nightly.

It turns out tensorflow==2.5.0 depends on keras-nightly ~=2.5.0.dev. I can reproduce it in a clean virtual environment. I will file a bug.

It was added here: tensorflow/tensorflow@d171d94

Can you please link the tensorflow issue for future reference. In the Java side we do not allow to include any SNAPSHOT dependency because of the lack of reproducibility. Is this ok to do on the python side?

@aaltay
Copy link
Member Author

aaltay commented Jun 8, 2021

Run PythonDocker PreCommit

@aaltay
Copy link
Member Author

aaltay commented Jun 8, 2021

That is strange because I'd expect only tf-nightly to depend on keras-nightly.

It turns out tensorflow==2.5.0 depends on keras-nightly ~=2.5.0.dev. I can reproduce it in a clean virtual environment. I will file a bug.
It was added here: tensorflow/tensorflow@d171d94

Can you please link the tensorflow issue for future reference. In the Java side we do not allow to include any SNAPSHOT dependency because of the lack of reproducibility. Is this ok to do on the python side?

I did not file a bug on their public issue tracker. The summary of the response was this was not intentional and it will be fixed in the next tensorflow release. If it helps, I can ask for a public bug for that. I do not think this is ok on python, and it was a bug.

On the license side, I think we are ok. Because even the tool was failing to parse, there is a license and apparently we have a file on our repo to manually track licenses for packages if the tooling we use fails.

@aaltay
Copy link
Member Author

aaltay commented Jun 9, 2021

Run PythonDocker PreCommit

@aaltay
Copy link
Member Author

aaltay commented Jun 9, 2021

@iemejia - tests are passing now. Let me know if this looks good.

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM, It is ok if the tensorflow team is already aware of it.

@iemejia iemejia merged commit 9cd7479 into master Jun 10, 2021
@iemejia
Copy link
Member

iemejia commented Jun 10, 2021

Thanks @aaltay . Can you please take care of cherry-picking this one for 2.31.0 since it is a security related upgrade.

aaltay pushed a commit to aaltay/beam that referenced this pull request Jun 11, 2021
@aaltay aaltay deleted the aaltay-patch-1 branch June 11, 2021 00:21
aaltay added a commit that referenced this pull request Jun 11, 2021
[BEAM-12411] Cherry-pick #14888: Update Tensorflow to version 2.5.0 & grpcio to 1.34.0
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.

3 participants