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: remove pip fallback option for python and ansible #5186

Merged
merged 9 commits into from
Feb 19, 2025

Conversation

pyranota
Copy link
Collaborator

@pyranota pyranota commented Jan 31, 2025

pip was deprecated since 1.425.0 (2024-11-15)


Important

Remove deprecated pip fallback options for Python and Ansible, cleaning up related code and configurations.

  • Behavior:
    • Remove pip fallback logic in uv_pip_compile() and spawn_uv_install() in python_executor.rs.
    • Remove pip related constants in worker.rs and worker_lockfiles.rs.
    • Delete download.py.pip.config.proto and download_deps.py.pip.sh.
  • Configuration:
    • Remove LOCK_CACHE_DIR and PIP_CACHE_DIR from main.rs and worker.rs.
    • Remove no_uv related flags from PythonAnnotations in worker.rs.
  • Misc:
    • Update handle_ansible_python_deps() in ansible_executor.rs to remove pip fallback.
    • Remove USE_PIP_COMPILE and USE_PIP_INSTALL from python_executor.rs and worker_lockfiles.rs.

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

BREAKING CHANGE: pip was deprecated since 1.425.0 (2024-11-15)
@pyranota pyranota changed the title refactor!: Remove pip fallback option for python and ansible feat: Remove pip fallback option for python and ansible Jan 31, 2025
Copy link

cloudflare-workers-and-pages bot commented Jan 31, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: d4d0d19
Status: ✅  Deploy successful!
Preview URL: https://3837f364.windmill.pages.dev
Branch Preview URL: https://py-remove-pip.windmill.pages.dev

View logs

@pyranota pyranota changed the title feat: Remove pip fallback option for python and ansible feat: remove pip fallback option for python and ansible Jan 31, 2025
@rubenfiszel rubenfiszel force-pushed the main branch 3 times, most recently from 2af98cd to 90ba65a Compare February 5, 2025 15:32
@rubenfiszel rubenfiszel force-pushed the main branch 3 times, most recently from 30f0807 to 9c57565 Compare February 14, 2025 03:38
@rubenfiszel rubenfiszel force-pushed the main branch 8 times, most recently from 3a05600 to 3b6585a Compare February 18, 2025 00:54
@pyranota pyranota marked this pull request as ready for review February 18, 2025 18:00
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 d4d0d19 in 1 minute and 28 seconds

More details
  • Looked at 912 lines of code in 10 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. backend/windmill-worker/src/worker_lockfiles.rs:159
  • Draft comment:
    In 'try_normalize', calling to_str() later (e.g. in parse_bun_relative_imports) may panic on non-UTF8 paths. Consider handling non-UTF8 filenames more gracefully.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-worker/src/worker_lockfiles.rs:186
  • Draft comment:
    In 'parse_bun_relative_imports', if 'try_normalize' returns None, an error is logged but no fallback is provided. Consider returning an informative error or handling the None case.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-worker/src/worker_lockfiles.rs:113
  • Draft comment:
    Review the SQL in 'clear_dependency_map_for_item'; using '($4::text IS NULL OR importer_node_id = $4::text)' may benefit from clearer parameter handling or comments explaining the logic.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
4. backend/windmill-worker/src/worker_lockfiles.rs:1155
  • Draft comment:
    The 'lock_modules' function is heavily recursive and complex. Consider refactoring or adding more inline documentation to improve maintainability and reduce cognitive load.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. backend/windmill-worker/src/worker_lockfiles.rs:1205
  • Draft comment:
    In 'reduce_flow', the pattern matching on FlowModuleValue uses an 'unreachable!' branch. Consider adding explicit error messages to handle unexpected cases gracefully.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/windmill-worker/src/worker_lockfiles.rs:295
  • Draft comment:
    After updating the 'script' table in 'handle_dependency_job', cache invalidation is done using 'cache::script::invalidate'. Consider checking and logging errors from cache invalidation to ensure consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. backend/windmill-worker/src/python_executor.rs:428
  • Draft comment:
    In the 'uv_pip_compile' function the pip fallback branch is now removed so that dependency resolution always uses the 'uv' tool. Make sure tests & documentation are updated to reflect that pip (deprecated since 1.425.0) is no longer used.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. backend/windmill-worker/src/ansible_executor.rs:77
  • Draft comment:
    The Ansible dependencies handling now calls 'uv_pip_compile' directly, removing the pip fallback option. Verify that the change aligns with expected behavior for resolving Python dependencies in Ansible jobs.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_DPj9g5ul1j7prM19


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

@rubenfiszel rubenfiszel merged commit 50bc4ca into main Feb 19, 2025
7 checks passed
@rubenfiszel rubenfiszel deleted the py-remove-pip branch February 19, 2025 15:33
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2025
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