-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Support google-cloud-spanner v3 and fixes broken unit tests #23365
Conversation
Run SpannerIO Read 2GB Performance Test Python |
Run Python 3.7 PostCommit |
Codecov Report
@@ Coverage Diff @@
## master #23365 +/- ##
==========================================
+ Coverage 73.40% 73.42% +0.01%
==========================================
Files 718 718
Lines 95620 95652 +32
==========================================
+ Hits 70194 70230 +36
+ Misses 24115 24111 -4
Partials 1311 1311
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Assigning reviewers. If you would like to opt out of this review, comment R: @yeandy for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Run SpannerIO Read 2GB Performance Test Python |
Run Python 3.7 PostCommit |
Run SpannerIO Read 2GB Performance Test Python |
Run Python 3.7 PostCommit |
* Fix spannerio read unit test actually not assert anything * Loose upperbound of spanner client library
e11aa43
to
9395a96
Compare
Run SpannerIO Read 2GB Performance Test Python |
Run Python 3.7 PostCommit |
Looks like not appropriate teardown/setup for tests |
Run Python 3.7 PostCommit |
Run SpannerIO Read 2GB Performance Test Python |
Performance test passed: https://ci-beam.apache.org/job/beam_PerformanceTests_SpannerIO_Read_2GB_Python_PR/8/ R: @johnjcasey |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
sdks/python/setup.py
Outdated
@@ -289,7 +289,8 @@ def get_portability_package_data(): | |||
'google-cloud-bigquery-storage>=2.6.3,<2.14', | |||
'google-cloud-core>=0.28.1,<3', | |||
'google-cloud-bigtable>=0.31.1,<2', | |||
'google-cloud-spanner>=1.13.0,<2', | |||
# google-cloud-spanner 2.x causes dependency parsing backoff | |||
'google-cloud-spanner>=1.13.0,!=2,<=3.21.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
We shouldn't use <= in the upper bound, it precludes us from asking someone to issue a patch release that could still be picked up by the constraints.
So we can use<3.22.0
or<4
. -
I think this is server-side dependency only, so using
<4
should be safe as we do with other GCP dependencies at this time. We'll decide soon whether we want to cap upper bounds tighter for all IO. -
Can you please run PostCommit integration tests with
google-cloud-spanner>=3.0.0
requirement if you haven't ? Unless we specify that requirement, postcommit tests will not pick up the new dependency. We can still merge the PRas is
with both V1 and V3 bounds. It's also fine to switch the lower bound to V3, as going forward we won't be testing V1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1,2) Yes we should set 'google-cloud-spanner<4' as other cloud dependencies
(3) I ran this when debugging (got some error and fixed it). From logging it shows version 3.21.0 is picked by pypi:
15:20:37 Collecting google-cloud-spanner!=2,<=3.21.0,>=1.13.0
15:20:37 Using cached google_cloud_spanner-3.21.0-py2.py3-none-any.whl (291 kB)
also
installed: apache-beam @ file:///app/sdks/python/target/.tox/.tmp/package/1/apache-beam-2.43.0.dev0.zip,cachetools==4.2.4,...,google-cloud-recommendations-ai==0.7.1,google-cloud-spanner==3.21.0,...
jenkins already cached v3.21.0 and is using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, change applied.
cc: @BjornPrime |
Run Python 3.7 PostCommit |
Run SpannerIO Read 2GB Performance Test Python |
Integration test and performance test passed with cloud-spanner v3.22.0. Could revert back the last commit |
This reverts commit 5b42e71.
The native python spannerio was proposed to be deprecated by Q4 but not happens. Still need some maintenance.
Fixes #20529
Fixes #21198
Fix spannerio read unit test actually not assert anything
Loose upperbound of spanner client library
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:
R: @username
).addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.