-
Notifications
You must be signed in to change notification settings - Fork 4.5k
BEAM-9626: Move out optional packages from core dependencies - 20259 #35297
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
Conversation
merging from upstream
|
PTAL @damccorm I also feel that we should update the CHANGES.md file to mention this change. |
|
Assigning reviewers: R: @liferoad for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
CHANGES.md
Outdated
| * X behavior was changed ([#X](https://github.com/apache/beam/issues/X)). | ||
| * X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).======= | ||
| * Yapf version upgraded to 0.43.0 for formatting (Python) ([#34801](https://github.com/apache/beam/pull/34801/)). | ||
| * Extra packages moved out of `install_required` to individual extra sections (`hdfs`, `mongodb`, `redis`) as part of [#35297](https://github.com/apache/beam/pull/35297) |
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.
Could we mention that the others have been moved to separate (existing) extras as well?
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.
Good point.
| # 3. Exclude protobuf 4 versions that leak memory, see: | ||
| # https://github.com/apache/beam/issues/28246 | ||
| 'protobuf>=3.20.3,<6.0.0.dev0,!=4.0.*,!=4.21.*,!=4.22.0,!=4.23.*,!=4.24.*', # pylint: disable=line-too-long | ||
| 'pydot>=1.2.0,<2', |
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.
Looks like some of these are causing failures because the appropriate extras aren't installed when running the test - could you take a look? (or feel free to revert/simplify to a smaller set of dependencies)
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.
I see you started on this, I think its probably just adding interactive/yaml to a few tox suites
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.
yes, I am not done with this change yet. I will update the tox suites soon.
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.
@damccorm what is the easiest way to find the mapping of test environment and the tox.ini section? This could help me to easily identify the broken environment and fix it. Otherwise, it will take longer time.
Also, I realized that jsonschema is part of core and can't be moved to extra section of yaml.
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.
would this line (Task :sdks:python:test-suites:tox:py312:testPython312 ) in the log good enough to identify the environment? or would you choose a different strategy to identify the environment?
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.
would this line (Task :sdks:python:test-suites:tox:py312:testPython312 ) in the log good enough to identify the environment? or would you choose a different strategy to identify the environment?
Yeah, that's what I usually use, and then you can look at the gradle task. An alternative at the failed test log, you can see something like:
________________________ YamlMappingTest.test_validate _________________________
[gw1] linux -- Python 3.9.22 /runner/_work/beam/beam/sdks/python/test-suites/tox/py39/build/srcs/sdks/python/target/.tox-py39-cloud/py39-cloud/bin/python
...
Tox will create a folder with the task name (in this case py39-cloud) as part of the path name. So in this case, the task is py39-cloud -
Line 99 in 9d75411
| [testenv:py{39,310,311,312,313}-cloud] |
You'll also see this task show up elsewhere in the logs (e.g. tox-py39-cloud shows up a bunch of places). An llm probably could figure it out too, but I haven't tried it yet :)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #35297 +/- ##
==========================================
Coverage 54.50% 54.51%
Complexity 1559 1559
==========================================
Files 1035 1036 +1
Lines 161595 161757 +162
Branches 1139 1139
==========================================
+ Hits 88084 88175 +91
- Misses 71380 71451 +71
Partials 2131 2131
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Reminder, please take a look at this pr: @liferoad |
|
waiting on author |
|
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 [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
More information could be found here:
BEAM-9626
#20259
Once this change is merged, the Pymongo could be installed with this command:
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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.mdwith 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 or the workflows README to see a list of phrases to trigger workflows.