-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Pin google-adk to 1.17.0 for YAML/Xlang precommit dependency resolution #38090
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -162,10 +162,16 @@ def cythonize(*args, **kwargs): | |
|
|
||
| milvus_dependency = ['pymilvus>=2.5.10,<3.0.0'] | ||
|
|
||
| ml_base = [ | ||
| # google-adk / OpenTelemetry require protobuf>=5; tensorflow-transform in | ||
| # ml_test is pinned to versions that require protobuf<5 on Python 3.10. Those | ||
| # cannot be installed together, so ADK deps stay out of ml_test (use ml_base). | ||
| ml_base_core = [ | ||
| 'embeddings>=0.0.4', # 0.0.3 crashes setuptools | ||
| 'google-adk', | ||
| 'onnxruntime', | ||
| # onnx 1.12–1.13 cap protobuf in ways that trigger huge backtracking with | ||
| # Beam[gcp]+ml_test; pip can fall back to onnx 1.11 sdist which needs cmake. | ||
| # 1.14.1+ matches tf2onnx>=1.16 and ships manylinux wheels for py3.10. | ||
| 'onnx>=1.14.1,<2', | ||
| 'langchain', | ||
| 'sentence-transformers>=2.2.2', | ||
| 'skl2onnx', | ||
|
|
@@ -174,11 +180,24 @@ def cythonize(*args, **kwargs): | |
| # tensorflow transitive dep, lower versions not compatible with Python3.10+ | ||
| 'absl-py>=0.12.0', | ||
| 'tensorflow-hub', | ||
| 'tf2onnx', | ||
| 'torch', | ||
| 'transformers', | ||
| ] | ||
|
|
||
| ml_adk_dependency = [ | ||
| 'google-adk==1.28.1', | ||
| # proto-plus<1.24 caps protobuf<5; opentelemetry-proto (via ADK) needs | ||
| # protobuf>=5. Scoped here so the main dependency list stays broader. | ||
| 'proto-plus>=1.26.1,<2', | ||
| 'opentelemetry-api==1.37.0', | ||
| 'opentelemetry-sdk==1.37.0', | ||
| 'opentelemetry-exporter-otlp-proto-http==1.37.0', | ||
|
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. Is there a reason we need to pin these opentelemetry dependencies as well?
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. Yes as ADK fixes OTel versions, unpinned exporters were upgrading and conflicting with ADK’s SDK constraint, so i pined the trio to match
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. and pip failed with something like google-adk 1.17.0 wants opentelemetry-sdk exactly 1.37.x, but pip pulled opentelemetry-exporter-otlp-proto-http 1.40.x, which requires opentelemetry-sdk~=1.40
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. Do you know where the opentelemetry requirement was coming from that installed 1.40.x? I would've expected that to be handled by pip
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. Pip was handling it, it ended in ResolutionImpossible / a hard conflict, not a wrong install and the 1.40.x side was coming from opentelemetry-exporter-otlp-proto-http (e.g. 1.40.0 wants opentelemetry-sdk~=1.40). google-adk meanwhile pins opentelemetry-sdk to 1.37.x, so theres no single version of the SDK that satisfies both so pip can’t merge those, it just fails unless we pin the exporter (and api) to the same minor as ADK’s SDK so nothing pulls 1.40.x into the graph
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. Right, but what requires
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. I had to pin opentelemetry-exporter-otlp-proto-http because otherwise pip kept grabbing a newer 1.40.x build that wants opentelemetry-sdk~=1.40, while ADK only lines up with 1.37.x so resolution blew up unless i locked the exporter to the same minor as the SDK @damccorm |
||
| # protobuf>=5 (ADK/OTel); tf2onnx 1.16.x pins protobuf~=3.20 only. | ||
| 'tf2onnx>=1.17.0,<1.18', | ||
| ] | ||
|
|
||
| ml_base = ml_base_core + ml_adk_dependency | ||
|
|
||
|
|
||
| def find_by_ext(root_dir, ext): | ||
| for root, _, files in os.walk(root_dir): | ||
|
|
@@ -392,6 +411,9 @@ def get_portability_package_data(): | |
| 'packaging>=22.0', | ||
| 'pillow>=12.1.1,<13', | ||
| 'pymongo>=3.8.0,<5.0.0', | ||
| # ADK / OpenTelemetry need proto-plus>=1.26.1 (protobuf>=5); that | ||
| # floor is declared on ml_adk_dependency only so core installs stay | ||
| # compatible with older proto-plus. | ||
| 'proto-plus>=1.7.1,<2', | ||
| # 1. Use a tighter upper bound in protobuf dependency to make sure | ||
| # the minor version at job submission | ||
|
|
@@ -449,7 +471,8 @@ def get_portability_package_data(): | |
| 'parameterized>=0.7.1,<0.10.0', | ||
| 'pyhamcrest>=1.9,!=1.10.0,<3.0.0', | ||
| 'requests_mock>=1.7,<2.0', | ||
| 'tenacity>=8.0.0,<9', | ||
| # google-adk 1.28+ requires tenacity>=9,<10 (conflicts with <9). | ||
| 'tenacity>=8.0.0,<10', | ||
| 'pytest>=7.1.2,<9.0', | ||
| 'pytest-xdist>=2.5.0,<4', | ||
| 'pytest-timeout>=2.1.0,<3', | ||
|
|
@@ -547,14 +570,14 @@ def get_portability_package_data(): | |
| # TFT->TFX-BSL require pandas 1.x, which is not compatible | ||
| # with numpy 2.x | ||
|
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. Why don't we need this anymore?
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. if you mean protobuf<4, i dropped that cap because with ADK/OTel in the same extra it fought protobuf>=5. ml_test no longer pulls ADK, so that specific cap isnt what unblocks resolution anymore, we can revisit a narrower cap separately if [gcp]+[ml_test] still allows it. |
||
| 'numpy<2', | ||
| # To help with dependency resolution in test suite. Revise once | ||
| # https://github.com/apache/beam/issues/37854 is fixed | ||
| 'protobuf<4; python_version<"3.11"' | ||
| # Comment out xgboost as it is breaking presubmit python ml | ||
| # tests due to tag check introduced since pip 24.2 | ||
| # https://github.com/apache/beam/issues/31285 | ||
| # 'xgboost<2.0', # https://github.com/apache/beam/issues/31252 | ||
| ] + ml_base, | ||
| # tft needs protobuf<5; tf2onnx 1.17+ allows protobuf 5 on the | ||
| # ADK-only path. | ||
| 'tf2onnx>=1.16.1,<1.17', | ||
| ] + ml_base_core, | ||
| 'p310_ml_test': [ | ||
| 'datatable', | ||
| ] + ml_base, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.