Repository navigation
Update xgboost dependency, execute in CI - #40694
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR updates the xgboost and datatable dependency bounds to xgboost>=2.0.0,<4 and datatable>=1.1.0,<2, re-enables XGBoost unit tests in CI (p311_ml_test, p312_ml_test, and postCommitPyDep tox tasks), and fixes an AttributeError on inference_calls when models are reloaded from disk during pipeline execution.
🔍 General Feedback
- The dependency updates, test fixture fix, and CI configuration in
setup.py,tox.ini, andbuild.gradleare clean and well-aligned. - There are a few minor typos and a placeholder PR link in
CHANGES.mdthat should be cleaned up before merging.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40694 +/- ##
==========================================
+ Coverage 80.04% 80.06% +0.01%
==========================================
Files 490 490
Lines 82916 82955 +39
==========================================
+ Hits 66373 66417 +44
+ Misses 16543 16538 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Assigning reviewers: R: @tvalentyn 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). |
| # in tox.ini. | ||
| xgboost_dependency = [ | ||
| # xgboost<2.0 wheels fail `pip check` on recent pip ("not supported on this | ||
| # platform"), see https://github.com/apache/beam/issues/31285. |
There was a problem hiding this comment.
consider pinning a specific comment if this is needed.
There was a problem hiding this comment.
Cleaned up the extra verbosity on the comment
| # apache_beam.ml.inference.xgboost_inference imports datatable directly. | ||
| # 1.1.0 is the first release with Python 3.10+ wheels; no release has | ||
| # wheels for Python 3.13+. | ||
| 'datatable>=1.1.0,<2', |
There was a problem hiding this comment.
Given how outdated datatable is, i think the XGBoost MH variation using datatable is no longer relevant; I think we should not make it a mandatory dep for XGBoost users, given there are other MHs that don't need it, and perhaps we should outright remove it due to lack of datatable releases.
There was a problem hiding this comment.
Datatable hasn't had a release since December of 2023, so I do think it's fair to consider it abandoned. I don't think we really have a good metric of how many users we have for the datatable variation of the model hander; perhaps it would be reasonable to mark the datatable handler as deprecated for this coming beam release, then remove it in the next? That would leave 2.78.0 as a functional release with updated xgboost + datatable support, then we remove it in 2.79.0
There was a problem hiding this comment.
Went ahead and marked the handler as deprecated, marked it for removal in the 2.79.0 release
| * (Python) `Regex.find_all` with `group=Regex.ALL` now returns every group, as `(match, group1, group2, ...)`, instead of only group 1, and with `outputEmpty=False` it drops a match only when the whole match is empty, matching the `group=0` behavior ([#40399](https://github.com/apache/beam/pull/40399)). | ||
| * (Python) `Regex.all_matches` now returns every group of the regex, instead of failing with a `TypeError` when the regex has no groups and dropping nested or unmatched trailing groups ([#40562](https://github.com/apache/beam/issues/40562)). | ||
| * (Python) `--dataflow_endpoint` now defaults to `''` instead of `https://dataflow.googleapis.com`. When unset, the Google Cloud client library resolves the Dataflow endpoint and honors `GOOGLE_CLOUD_UNIVERSE_DOMAIN`, so Dataflow jobs can be submitted in non-default universes. Code that reads `GoogleCloudOptions.dataflow_endpoint` should handle an empty value ([#40569](https://github.com/apache/beam/issues/40569)). | ||
| * (Python) The `xgboost` extras now install `xgboost>=2.0.0,<4` and `datatable>=1.1.0,<2`, moving both the lower and upper bounds for both. The previous minimum `xgboost` and `datatable` versions (`1.6.0` and `1.0.0` respectively) are no longer allowed ([#40694](https://github.com/apache/beam/issues/40694)). |
There was a problem hiding this comment.
we should update xgboost inference docstring which currently says:
**Supported Versions:** RunInference APIs in Apache Beam have been tested
with XGBoost 1.6.0 and 1.7.0
tvalentyn
left a comment
tvalentyn
left a comment
There was a problem hiding this comment.
Let's file an issue that targets the next release to follow up on the deprecation.
Thanks!
|
Dependency postcommit breakages are numpy/pandas (fixed in #40704), pytorch (fix in-flight) and transformers (not yet started) |
Filed as #40705 and put on the 2.79.0 milestone |
2 participants
Update the xgboost dependencies (xgboost and datatables,) fix a broken test case that had rotted while xgboost wasn't executing in CI.