-
Notifications
You must be signed in to change notification settings - Fork 82
chore(clp-package): Migrate fully to uv + hatchling (resolves #1396); Upgrade dependencies. #1405
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: main
Are you sure you want to change the base?
Conversation
…#1396); Require Python >= 3.10 (resolves y-scope#1380); Upgrade dependencies.
WalkthroughMultiple Python components switch build tooling from Poetry to Hatch/uv, several pyproject dependency specifiers and authors metadata are updated, taskfile tasks change from Changes
Sequence Diagram(s)(omitted — changes are import/configuration-only and do not introduce new runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings
📚 Learning: 2025-10-13T03:24:35.074Z
Applied to files:
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Are we planning to include uv locks? If so, shall we update tasks for uv lock file checking? #1381
yes
i had to write a script to run |
Sure, just a heads up |
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.
this empty file has to be added. otherwise, uv build
had trouble finding this as a valid Python package.
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.
All the changed Python source files in this PR are just the results of running task lint:fix-py
.
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.
Yeah I think these changes make sense: before this PR, there's no project name set in the pyproject.toml
. Now the imports are separated into "third-party" dependency and the project imports.
as discussed offline with @kirkrodrigues , the Python 3.9 -> 3.10 requirement change will be done in a separate PR. |
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.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
components/clp-mcp-server/uv.lock
is excluded by!**/*.lock
components/clp-package-utils/uv.lock
is excluded by!**/*.lock
components/clp-py-utils/uv.lock
is excluded by!**/*.lock
components/job-orchestration/uv.lock
is excluded by!**/*.lock
integration-tests/uv.lock
is excluded by!**/*.lock
📒 Files selected for processing (3)
components/clp-package-utils/pyproject.toml
(1 hunks)components/clp-py-utils/pyproject.toml
(1 hunks)components/job-orchestration/pyproject.toml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
authors = [{name = "YScope Inc.", email = "dev@yscope.com"}] | ||
readme = "README.md" | ||
|
||
[tool.poetry.dependencies] | ||
python = ">= 3.9" | ||
Brotli = "^1.1.0" | ||
msgpack = "^1.0.7" | ||
psutil = "^7.1.0" | ||
pydantic = "^2.11.9" | ||
pymongo = "^4.7.2" | ||
PyYAML = "^6.0.1" | ||
requires-python = ">=3.9" | ||
dependencies = [ | ||
"Brotli>=1.1.0", | ||
"msgpack>=1.1.2", | ||
"psutil>=7.1.0", | ||
"pydantic>=2.12.0", | ||
"pymongo>=4.15.3", | ||
"PyYAML>=6.0.3", | ||
] |
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.
Align requires-python
with the 3.10 floor.
The PR mandate is to move every package to Python ≥3.10, yet Line 7 still advertises support down to 3.9. This leaves the wheel installable on 3.9, diverging from the documented requirement and increasing support risk. Please bump the constraint here (and ensure matching metadata elsewhere) to enforce the new floor.
-requires-python = ">=3.9"
+requires-python = ">=3.10"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
authors = [{name = "YScope Inc.", email = "dev@yscope.com"}] | |
readme = "README.md" | |
[tool.poetry.dependencies] | |
python = ">= 3.9" | |
Brotli = "^1.1.0" | |
msgpack = "^1.0.7" | |
psutil = "^7.1.0" | |
pydantic = "^2.11.9" | |
pymongo = "^4.7.2" | |
PyYAML = "^6.0.1" | |
requires-python = ">=3.9" | |
dependencies = [ | |
"Brotli>=1.1.0", | |
"msgpack>=1.1.2", | |
"psutil>=7.1.0", | |
"pydantic>=2.12.0", | |
"pymongo>=4.15.3", | |
"PyYAML>=6.0.3", | |
] | |
authors = [{name = "YScope Inc.", email = "dev@yscope.com"}] | |
readme = "README.md" | |
requires-python = ">=3.10" | |
dependencies = [ | |
"Brotli>=1.1.0", | |
"msgpack>=1.1.2", | |
"psutil>=7.1.0", | |
"pydantic>=2.12.0", | |
"pymongo>=4.15.3", | |
"PyYAML>=6.0.3", | |
] |
🤖 Prompt for AI Agents
In components/clp-package-utils/pyproject.toml around lines 5 to 15, update the
requires-python constraint from ">=3.9" to ">=3.10" to enforce the new Python
3.10 floor; then search the repository for other packaging metadata (other
pyproject.toml, setup.cfg, setup.py, or wheel classifiers) and CI matrix entries
and update any remaining references to Python 3.9 so they consistently reflect
>=3.10.
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.
It was decided that we should do the Python version requirement change in a separate PR to reduce review effort
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.
@junhaoliao, understood! I'll note that the Python 3.10 migration is being handled separately.
Would you like me to create a GitHub issue to track this follow-up work? I can include a reference back to this PR and the discussion.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.053Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.
authors = [{name = "YScope Inc.", email = "dev@yscope.com"}] | ||
readme = "README.md" | ||
|
||
[tool.poetry.dependencies] | ||
python = ">= 3.9" | ||
boto3 = "^1.35.81" | ||
# mariadb version must be compatible with libmariadev installed in runtime env. | ||
# See https://mariadb.com/docs/server/connect/programming-languages/python/install/#Dependencies | ||
mariadb = "~1.0.11" | ||
mysql-connector-python = "^8.2.0" | ||
pydantic = "^2.11.9" | ||
python-dotenv = "^1.0.1" | ||
python-Levenshtein = "~0.22" | ||
sqlalchemy = "~2.0" | ||
PyYAML = "^6.0.1" | ||
result = "^0.17.0" | ||
StrEnum = "^0.4.15" | ||
requires-python = ">=3.9" | ||
dependencies = [ | ||
"boto3>=1.40.49", | ||
# mariadb version must be compatible with libmariadev installed in runtime env. | ||
# See https://mariadb.com/docs/server/connect/programming-languages/python/install/#Dependencies | ||
"mariadb>=1.0.11,<1.1.dev0", | ||
"mysql-connector-python>=9.4.0", | ||
"pydantic>=2.12.0", | ||
"python-dotenv>=1.1.1", | ||
"python-Levenshtein>=0.27.1", | ||
"sqlalchemy>=2.0.43", | ||
"PyYAML>=6.0.3", | ||
"result>=0.17.0", | ||
"StrEnum>=0.4.15", | ||
] |
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.
Enforce the Python 3.10 minimum here as well.
Line 7 continues to advertise support for Python 3.9, so users on 3.9 will receive this build despite the intent to drop that runtime. Please update the requirement to reflect the new ≥3.10 baseline.
-requires-python = ">=3.9"
+requires-python = ">=3.10"
🤖 Prompt for AI Agents
In components/clp-py-utils/pyproject.toml around lines 5 to 21, the
requires-python field still states ">=3.9" while the project now enforces a
Python ≥3.10 baseline; update the requires-python value to ">=3.10" (and if
present elsewhere in the file or package metadata, make the same change) so
published builds correctly require Python 3.10 or newer.
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.
It was decided that we should do the Python version requirement change in a separate PR to reduce review effort
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.
@junhaoliao, understood! That makes sense to separate the Python version requirement change into its own PR.
Would you like me to create a follow-up issue to track updating the requires-python
field to ">=3.10"
across all Python packages in the repository?
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.
authors = [{name = "YScope Inc.", email = "dev@yscope.com"}] | ||
readme = "README.md" | ||
|
||
[tool.poetry.dependencies] | ||
python = ">= 3.9,<4.0" | ||
Brotli = "^1.1.0" | ||
celery = {extras = ["redis"], version = "^5.5.3"} | ||
# mariadb version must be compatible with libmariadev installed in runtime env. | ||
# See https://mariadb.com/docs/server/connect/programming-languages/python/install/#Dependencies | ||
mariadb = "~1.0.11" | ||
msgpack = "^1.0.7" | ||
mysql-connector-python = "^8.2.0" | ||
pika = "^1.3.2" | ||
pydantic = "^2.11.9" | ||
pymongo = "^4.7.2" | ||
PyYAML = "^6.0.1" | ||
yscope-spider-py="0.1.0" | ||
requires-python = ">=3.9,<4.0" | ||
dependencies = [ | ||
"Brotli>=1.1.0", | ||
"celery[redis]>=5.5.3", | ||
# mariadb version must be compatible with libmariadev installed in runtime env. | ||
# See https://mariadb.com/docs/server/connect/programming-languages/python/install/#Dependencies | ||
"mariadb>=1.0.11,<1.1.dev0", | ||
"msgpack>=1.1.2", | ||
"mysql-connector-python>=9.4.0", | ||
"pika>=1.3.2", | ||
"pydantic>=2.12.0", | ||
"pymongo>=4.15.3", | ||
"PyYAML>=6.0.3", | ||
"yscope-spider-py==0.1.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.
Update the Python compatibility floor.
This pyproject still declares Python ≥3.9, contradicting the PR goal of raising the requirement to ≥3.10. Without updating the marker, installers will keep pulling the package on 3.9 runtimes. Please tighten the constraint accordingly (keeping the <4.0
upper bound if that is still intended).
-requires-python = ">=3.9,<4.0"
+requires-python = ">=3.10,<4.0"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
authors = [{name = "YScope Inc.", email = "dev@yscope.com"}] | |
readme = "README.md" | |
[tool.poetry.dependencies] | |
python = ">= 3.9,<4.0" | |
Brotli = "^1.1.0" | |
celery = {extras = ["redis"], version = "^5.5.3"} | |
# mariadb version must be compatible with libmariadev installed in runtime env. | |
# See https://mariadb.com/docs/server/connect/programming-languages/python/install/#Dependencies | |
mariadb = "~1.0.11" | |
msgpack = "^1.0.7" | |
mysql-connector-python = "^8.2.0" | |
pika = "^1.3.2" | |
pydantic = "^2.11.9" | |
pymongo = "^4.7.2" | |
PyYAML = "^6.0.1" | |
yscope-spider-py="0.1.0" | |
requires-python = ">=3.9,<4.0" | |
dependencies = [ | |
"Brotli>=1.1.0", | |
"celery[redis]>=5.5.3", | |
# mariadb version must be compatible with libmariadev installed in runtime env. | |
# See https://mariadb.com/docs/server/connect/programming-languages/python/install/#Dependencies | |
"mariadb>=1.0.11,<1.1.dev0", | |
"msgpack>=1.1.2", | |
"mysql-connector-python>=9.4.0", | |
"pika>=1.3.2", | |
"pydantic>=2.12.0", | |
"pymongo>=4.15.3", | |
"PyYAML>=6.0.3", | |
"yscope-spider-py==0.1.0", | |
] | |
authors = [{name = "YScope Inc.", email = "dev@yscope.com"}] | |
readme = "README.md" | |
requires-python = ">=3.10,<4.0" | |
dependencies = [ | |
"Brotli>=1.1.0", | |
"celery[redis]>=5.5.3", | |
# mariadb version must be compatible with libmariadev installed in runtime env. | |
# See https://mariadb.com/docs/server/connect/programming-languages/python/install/#Dependencies | |
"mariadb>=1.0.11,<1.1.dev0", | |
"msgpack>=1.1.2", | |
"mysql-connector-python>=9.4.0", | |
"pika>=1.3.2", | |
"pydantic>=2.12.0", | |
"pymongo>=4.15.3", | |
"PyYAML>=6.0.3", | |
"yscope-spider-py==0.1.0", | |
] |
🤖 Prompt for AI Agents
In components/job-orchestration/pyproject.toml around lines 5 to 21, the
requires-python marker still allows Python 3.9 but the PR intends to raise the
minimum to 3.10; update the requires-python entry to ">=3.10,<4.0" so installers
won't accept Python 3.9 runtimes while preserving the existing upper bound.
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.
It was decided that we should do the Python version requirement change in a separate PR to reduce review effort. Therefore, the requires-python
value was unchanged
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.
@junhaoliao, understood! Thanks for clarifying that the Python version bump will be handled in a separate PR. That makes sense for keeping this review focused on the migration to uv + hatchling. I'll skip this suggestion accordingly.
…n pyproject.toml and uv.lock.
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.
Overall lgtm. Only some minor fixes.
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.
Shall we defer this?
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
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.
Yeah I think these changes make sense: before this PR, there's no project name set in the pyproject.toml
. Now the imports are separated into "third-party" dependency and the project imports.
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.
Several general comments:
- Each
pyproject.toml
should follow
https://github.com/y-scope/yscope-dev-utils/tree/main/exports/lint-configs/python - If we really want to make each component into a formal python package, then we should add a
src
layer. The example setup includes:
- the integration tests component (even though it doesn't have actual source files)
- log-archival-bench repo (this one doesn't have test files yet)
- ystdlib-py
- We should probably use the same python version everywhere? idk if mixing up
3.9
and3.10
makes sense. Or we just defer changing python req in this PR like you said. - The linting process needs to be updated: https://github.com/junhaoliao/clp/blob/cf28cae05f47e32c18ce00cb385ce540cbe9ad43/taskfiles/lint.yaml#L780
But that would probably pollute the scope of this PR. Just need to be aware of this code section that's pending updates.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
Thanks for the review. All the points you mentioned are worth doing, but some of them will make the current PR even larger and more difficult to review. Do you mind if we defer some of the items? (feel free to ask the rabbit to create any issue so we don't forget) please see below for my responses:
Do you mean the linting configs should be ported from https://github.com/y-scope/yscope-dev-utils/blob/main/exports/ystdlib-py/pyproject.toml ? I'm aware that they are needed to enable the rules but I intentionally left those out to avoid bloating the PR, which mainly aims to migrate from poetry to uv.
Do you mind if we merge the current PR first, and then open a separate, simple PR to move the sources into the
It was @kirkrodrigues 's proposal to defer the Python 3.9 -> 3.10 upgrade to reduce review effort. As a result, the changes to upgrade the min required version were reverted in 6fe93a1 and 3ef632f. I believe the Python wheels for That said, we should have updated the User Quick Start Guide to say we officially only support Python 3.10 (instead of "3.9 or higher) when we were removing the Jammy Focal image because the libraries are only built for Python 3.10. @coderabbitai create an issue for the incorrect doc
Right, i think we should defer the linter config updates. |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 42 @junhaoliao, I've created issue #1438 to track the documentation update for Python 3.10 support in the User Quick Start Guide. Your approach to defer the following items makes perfect sense to keep this PR focused on the core migration from poetry to uv + hatchling:
This staged approach will make the reviews more manageable and reduce the risk of conflicts with other in-flight PRs. The current PR already accomplishes the primary objective of migrating the build system and dependency management tooling. 🧠 Learnings used
|
Description
clp-package-utils
,clp-py-utils
,job-orchestration
:clp-package-utils
,clp-py-utils
,job-orchestration
,integration-tests
:Checklist
breaking change.
Validation performed
build/clp-package/etc/clp-config.yml
.sbin/start-clp.sh
sbin/compress.sh ~/samples/hive-24hr
Summary by CodeRabbit
Chores
Tests
Bug Fixes
Style