Repository navigation
Update xgboost dependency, execute in CI #40694
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
Changes from all commits
989751d
e4dd145
e5d6834
c081dda
d7e44b1
8455404
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| { | ||
| "comment": "Modify this file in a trivial way to cause this test suite to run", | ||
| "modification": 4 | ||
| "modification": 5 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| { | ||
| "comment": "Modify this file in a trivial way to cause this test suite to run", | ||
| "revision": 22 | ||
| "revision": 23 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,5 +15,5 @@ | |
| # limitations under the License. | ||
| # | ||
|
|
||
| datatable==1.0.0 | ||
| xgboost>=1.6.0 | ||
| datatable>=1.1.0,<2 | ||
| xgboost>=2.0.0,<4 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,6 +166,17 @@ def cythonize(*args, **kwargs): | |
|
|
||
| milvus_dependency = ['pymilvus>=2.5.10,<3.0.0'] | ||
| qdrant_dependency = ['qdrant-client>=1.15.0'] | ||
| # NOTE: When updating these bounds, also update the bounds in | ||
| # apache_beam/ml/inference/xgboost_tests_requirements.txt, which is used by the | ||
| # XGBoost RunInference integration tests, and the py311-xgboost-* environments | ||
| # in tox.ini. | ||
| xgboost_dependency = [ | ||
| 'xgboost>=2.0.0,<4', | ||
| # 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', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Went ahead and marked the handler as deprecated, marked it for removal in the 2.79.0 release |
||
| ] | ||
|
|
||
| # google-adk / OpenTelemetry require protobuf>=5; tensorflow-transform in | ||
| # ml_test is pinned to versions that require protobuf<5 on Python 3.10. Those | ||
|
|
@@ -608,13 +619,12 @@ def get_portability_package_data(): | |
| 'p310_ml_test': [ | ||
| 'datatable', | ||
| ] + ml_base + qdrant_dependency, | ||
| 'p311_ml_test': [ | ||
| 'datatable', | ||
| ] + ml_base + qdrant_dependency, | ||
| # xgboost_dependency includes datatable, which only has wheels for | ||
| # Python <=3.12, so it is limited to the 3.11 and 3.12 test extras. | ||
| 'p311_ml_test': ml_base + xgboost_dependency + qdrant_dependency, | ||
| 'p312_ml_test': [ | ||
| 'datatable', | ||
| 'typesafe-sdk>=0.7.1,<0.8', | ||
| ] + ml_base + qdrant_dependency, | ||
| ] + ml_base + xgboost_dependency + qdrant_dependency, | ||
| # maintainer: milvus tests only run with this extension. Make sure it | ||
| # is covered by docker-in-docker test when changing py version | ||
| 'p313_ml_test': ml_base_core + milvus_dependency + qdrant_dependency, | ||
|
|
@@ -682,7 +692,7 @@ def get_portability_package_data(): | |
| 'transformers==4.25.1', # tensorflow transient dep | ||
| 'absl-py>=0.12.0' | ||
| ], | ||
| 'xgboost': ['xgboost>=1.6.0,<2.1.3', 'datatable==1.0.0'], | ||
| 'xgboost': xgboost_dependency, | ||
| 'tensorflow-hub': ['tensorflow-hub>=0.14.0,<0.16.0'], | ||
| 'milvus': milvus_dependency, | ||
| 'qdrant': qdrant_dependency, | ||
|
|
||
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 should update xgboost inference docstring which currently says:
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.
Updated