-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Switch from Poetry to uv #3024
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
Open
wintonzheng
wants to merge
1
commit into
main
Choose a base branch
from
codex/migrate-from-poetry-to-uv
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Switch from Poetry to uv #3024
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Important
Looks good to me! 👍
Reviewed everything up to 0a79ed1 in 2 minutes and 10 seconds. Click for details.
- Reviewed
266
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
10
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/ci.yml:48
- Draft comment:
Cache key now hashes both 'pyproject.toml' and 'poetry.lock'. Since the switch to uv is underway, consider updating or removing references to 'poetry.lock' if uv uses a different lock file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment identifies a real inconsistency - we're switching from Poetry to uv but still using poetry.lock in the cache key. However, I don't see strong evidence that uv uses a different lock file named uv.lock. The suggestion to use uv.lock seems speculative without confirmation of uv's lock file naming convention. I might be too hesitant - if we're removing Poetry entirely, the poetry.lock reference will definitely be wrong. But the suggested fix assumes facts about uv that aren't verified. While the comment identifies a real issue, making assumptions about uv's lock file format could lead to broken CI. Better to let the author research the correct uv caching approach. The comment should be deleted because it makes speculative assumptions about uv's lock file format without clear evidence.
2. .github/workflows/ci.yml:51
- Draft comment:
The dependency installation command 'uv pip install -e "[dev]"' is run conditionally and then again unconditionally. This duplication might be redundant—consider consolidating them to avoid unnecessary re-installation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This pattern is actually common and intentional in dependency management. The first conditional install happens only on cache miss and sets up the full environment. The second unconditional install ensures any new dependencies are installed even if the cache hit. This is not redundant - it's a standard pattern to handle both cache hit and miss cases correctly. Maybe there's a more efficient way to handle both cache scenarios that I'm not aware of? The comment author might have experience with a better pattern. While there might be other patterns, this is a standard and well-understood approach. The apparent duplication serves a specific purpose in dependency management with caching. The comment should be deleted because it suggests removing a pattern that is actually correct and necessary for proper dependency management with caching.
3. .github/workflows/ci.yml:80
- Draft comment:
Using 'source .venv/bin/activate' assumes the virtual environment exists. Ensure that a .venv is reliably created (e.g. via 'uv venv') before these activation steps are executed. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. .github/workflows/codeflash.yaml:40
- Draft comment:
Consider using a uv-specific execution command (if available, e.g. 'uv run codeflash') rather than manually sourcing the virtual environment, to streamline and standardize the execution process. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment is about changed code (switching from poetry to uv), I need to consider: 1) Is this a clear improvement? 2) Do we know for sure 'uv run' exists and works? 3) The current approach using source and activate is a very standard, well-understood pattern. 4) The comment is somewhat speculative with "if available". I might be overlooking potential benefits of uv-specific commands that could make the workflow more robust. The current solution might have hidden issues with virtual environment activation. The standard venv activation pattern is well-tested and works reliably across environments. The speculative nature of the comment ("if available") suggests uncertainty about the alternative. Delete the comment. The suggested change is speculative, and the current solution uses a standard, reliable pattern for virtual environment activation.
5. .github/workflows/sdk-release.yml:58
- Draft comment:
The cache key again includes 'poetry.lock' along with 'pyproject.toml'. Revisit if uv produces a different lock file, to ensure the dependency cache remains fresh. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid point - we're using poetry.lock in the cache key but switched to uv. However, this is more of a speculative "maybe this could be an issue" type comment. We don't know if uv actually uses a different lock file format or if this will cause any real problems. The cache will simply miss and rebuild if needed. The comment identifies a potential inconsistency that could affect build efficiency. If uv uses a different lock file, the cache key won't properly invalidate. While technically correct, this is a speculative comment about a potential future issue. The worst case is just a cache miss, which isn't a critical problem. This comment should be deleted as it's speculative and doesn't identify a clear, immediate problem that needs fixing.
6. .github/workflows/sdk-release.yml:72
- Draft comment:
Installing twine inline using 'uv pip install twine && twine upload ...' could obscure errors during install. Consider separating the installation of twine from the upload command to improve clarity and error handling. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment suggests a valid practice, the risk is minimal in this context. The twine installation is very simple and unlikely to fail silently. If it fails, the whole step will fail clearly. GitHub Actions shows clear output for each command in the chain. The current approach is concise and common in CI scripts. I might be underestimating the value of explicit error handling in a release workflow. A failed release due to unclear errors could be problematic. The simplicity of installing a single package (twine) via uv makes the risk very low, and GitHub Actions provides good visibility into command execution regardless. The comment should be deleted as it suggests a change that would add complexity without meaningful benefit in this specific context.
7. Dockerfile:3
- Draft comment:
The Dockerfile installs uv globally and uses it to install the package. If isolation is desired, consider creating a virtual environment using 'uv venv' before installing dependencies. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Using Docker already provides container isolation. While virtual environments are good practice in development, they add less value inside a container where the entire environment is already isolated. The comment seems to be suggesting a best practice that isn't as relevant in this containerized context. Maybe there are specific isolation requirements we don't know about. Virtual environments could provide an extra layer of dependency isolation even within containers. The container itself provides strong isolation, and adding a virtual environment would increase complexity without providing significant benefits in this context. Delete the comment as it suggests additional complexity that isn't necessary given the containerized environment.
8. run_skyvern.sh:14
- Draft comment:
The script conditionally runs 'uv venv' if .venv is missing. Ensure that 'uv venv' reliably creates the virtual environment for consistent local development setups. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that 'uv venv' reliably creates the virtual environment. This falls under the rule of not asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
9. README.md:395
- Draft comment:
The README now uses 'uv pip install -e .' which aligns with the new tool. Verify that all documentation and examples reflect the removal of Poetry for consistency. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
10. skyvern/webeye/README.md:23
- Draft comment:
The WebHuman README updates prerequisites and installation to use uv. Ensure that any additional uv-specific guidance is included for new users. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_pZBFsbNpsPXu3dBZ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
uv
for local scriptsDescription
This pull request migrates the project's Python package management from Poetry to uv across all environments and workflows. The change affects CI/CD pipelines, Docker builds, local development scripts, and documentation. Key modifications include:
.github/workflows/ci.yml
,.github/workflows/codeflash.yaml
, and.github/workflows/sdk-release.yml
to useuv
instead of Poetry for dependency management and virtual environment creationDockerfile
by removing the multi-stage Poetry-based requirements generation and directly usinguv pip install
run_skyvern.sh
to create and activate virtual environments usinguv venv
and install dependencies withuv pip install
uv
instead of Poetry for installation instructionsThe migration maintains the same dependency caching strategy but updates cache keys to include both
pyproject.toml
andpoetry.lock
files. All workflow steps that previously usedpoetry run
commands now activate the virtual environment manually and run commands directly.Testing
pre-commit run --all-files
(fails: pre-commit not installed)pytest
(fails: missing dependencies)Changes that Break Backward Compatibility
This change may break backward compatibility for developers who have existing Poetry-based local development setups. Developers will need to:
uv
instead of Poetryuv pip install -e .
instead ofpoetry install
source .venv/bin/activate
instead of usingpoetry run
Documentation
Updated documentation includes:
README.md
: Changed installation command frompip install -e .
touv pip install -e .
skyvern/webeye/README.md
: Updated prerequisites to mentionuv
instead of Poetry and changed installation instructionshttps://chatgpt.com/codex/tasks/task_b_6881b0bcd644832d9b7d03ba7191469f
Created with Palmier
Important
Switch from Poetry to uv for dependency management and virtual environment setup across the project.
uv
in.github/workflows/ci.yml
,.github/workflows/codeflash.yaml
, and.github/workflows/sdk-release.yml
.uv
.uv
for installing dependencies.uv
and dependencies.run_skyvern.sh
to useuv
for virtual environment and dependency management.README.md
andskyvern/webeye/README.md
to documentuv
usage for installation and setup.pre-commit
andpytest
fail due to missing dependencies and pre-commit not being installed.This description was created by
for 0a79ed1. You can customize this summary. It will automatically update as commits are pushed.