Skip to content
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

feat: Replace pip-compile with uv #4460

Merged
merged 25 commits into from
Oct 3, 2024
Merged

feat: Replace pip-compile with uv #4460

merged 25 commits into from
Oct 3, 2024

Conversation

pyranota
Copy link
Collaborator

@pyranota pyranota commented Sep 30, 2024

Makes more idiomatic and is much faster on cold starts.
Useful if starting fresh instance or/and adding new dependency to python


Important

Replace pip-compile with uv for Python dependency management, update Docker and Nix configurations, and refactor TypeScript and Python annotations.

  • Dependency Management:
    • Replace pip-compile with uv in python_executor.rs and ansible_executor.rs for Python dependency resolution.
    • Add get_annotation_python() in worker.rs to handle no_uv annotation for Python scripts.
    • Update uv_pip_compile() to handle no_uv flag and fallback to pip-compile if necessary.
  • Docker and Environment:
    • Install uv in Dockerfile, DockerfileSlim, and DockerfileSlimEe.
    • Add uv to shell.nix for Nix environment setup.
  • Code Refactoring:
    • Rename get_annotation() to get_annotation_ts() in worker.rs and update references in bun_executor.rs, worker_lockfiles.rs, and scripts.rs.
    • Update handle_python_deps() and handle_ansible_python_deps() to use uv_pip_compile().
  • Miscellaneous:
    • Update RUST_LOG to "debug" in shell.nix for development purposes.

This description was created by Ellipsis for 621e5a2. It will automatically update as commits are pushed.

- Replace pip-compile with uv packages
- Pin rust version
- Add var to trigger windmill print more info in stdout
Some of the flags are included by default in UV and can be safely removed:
- --resolver=backtracking
- --no-emit-index-url

Also uv does not support `--pip-args` and suggests to directly pass args to uv.
Copy link

cloudflare-workers-and-pages bot commented Sep 30, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8e48066
Status:⚡️  Build in progress...

View logs

@pyranota
Copy link
Collaborator Author

pyranota commented Sep 30, 2024

Currently testing change. Small note:
uv handles extra index differently compared to pip-compile, package will be checked against extra index first and if not found fallback to main index, quite the opposite for pip-compile where main index will be checked first and then extra.

Edit: Resolved by using --index-strategy unsafe-best-match. Read more - https://docs.astral.sh/uv/pip/compatibility/#packages-that-exist-on-multiple-indexes

@pyranota pyranota marked this pull request as ready for review October 2, 2024 10:44
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 80fbdd2 in 34 seconds

More details
  • Looked at 588 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. docker/DockerfileSlim:18
  • Draft comment:
    pip-tools is still being installed, which is unnecessary if uv is replacing pip-compile. Consider removing this line.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. docker/DockerfileSlimEe:18
  • Draft comment:
    pip-tools is still being installed, which is unnecessary if uv is replacing pip-compile. Consider removing this line.
  • Reason this comment was not posted:
    Marked as duplicate.
3. shell.nix:31
  • Draft comment:
    pip-tools is still being included, which is unnecessary if uv is replacing pip-compile. Consider removing this line.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_kUt9IB3lRJVitQsx


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

error[E0658]: attributes on expressions are experimental
   --> windmill-worker/src/python_executor.rs:144:5
    |
144 |     #[cfg(feature = "enterprise")]
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #15701 <rust-lang/rust#15701> for more information
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 175528e in 22 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. backend/windmill-worker/src/python_executor.rs:146
  • Draft comment:
    Using unwrap here can cause a panic if the expected capture group is not present. Consider handling this case more gracefully to avoid potential runtime errors.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_Ac1Y9G8sFlzsPKkk


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Will force recalculation of lockfile
And block uv from using cached values
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 4f06bfe in 31 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. backend/windmill-worker/src/python_executor.rs:261
  • Draft comment:
    The added arguments --python-preference only-system and --no-python-downloads ensure that uv does not manage Python installations, which aligns with the PR intent.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The added arguments to the uv command in the uv_pip_compile function are intended to prevent uv from managing Python installations. This is consistent with the PR description and comments.

Workflow ID: wflow_aaeBOQd0dDUKyrRH


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on abee3d7 in 17 seconds

More details
  • Looked at 14 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/DockerfileBackendTests:47
  • Draft comment:
    Consider verifying the integrity of the uv installation script before executing it to prevent potential security risks.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_1pQj35qzDhf2xDeZ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@pyranota
Copy link
Collaborator Author

pyranota commented Oct 2, 2024

@rubenfiszel Can't pass checks because environment of tests is managed by repository and I cant alter it to include uv

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 04aac60 in 43 seconds

More details
  • Looked at 41 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_NlqBFn1F4ei1slY7


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 621e5a2 in 19 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. backend/windmill-worker/src/python_executor.rs:152
  • Draft comment:
    Redundant log message. Consider removing this line as the fallback to pip-compile is already logged on line 180.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The comment on line 152 is redundant since the same information is already logged on line 180. It's better to keep the logging consistent and avoid redundancy.

Workflow ID: wflow_3aCdEJ1ooo8gn7GV


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel merged commit b54c9ee into main Oct 3, 2024
5 of 6 checks passed
@rubenfiszel rubenfiszel deleted the python-uv branch October 3, 2024 14:00
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants