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: Handle pip install by uv #4517

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

feat: Handle pip install by uv #4517

wants to merge 27 commits into from

Conversation

pyranota
Copy link
Collaborator

@pyranota pyranota commented Oct 10, 2024

Replace python pip install with uv pip install


Important

Replace python pip install with uv pip install, adding fallback mechanisms and updating cache management.

  • Behavior:
    • Replace python pip install with uv pip install in download_deps.py.sh and python_executor.rs.
    • Fallback to pip if USE_PIP_INSTALL is set or no_uv_install is true.
  • Configuration:
    • Add download.py.pip.config.proto and download_deps.py.pip.sh for fallback to pip.
    • Update docker-image.yml to trigger on python-uv branch.
  • Cache Management:
    • Introduce PY311_CACHE_DIR in worker.rs and global_cache.rs for uv installations.
    • Modify build_tar_and_push and pull_from_tar to handle uv cache.
  • Annotations:
    • Add no_uv_install and no_uv_compile to PythonAnnotations in worker.rs.

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

Dirty and untested, but already something working
Copy link

cloudflare-workers-and-pages bot commented Oct 10, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: eff3e1c
Status: ✅  Deploy successful!
Preview URL: https://9d166688.windmill.pages.dev
Branch Preview URL: https://python-uv.windmill.pages.dev

View logs

@pyranota
Copy link
Collaborator Author

we have to be careful not to merge this PR before we reverse CI/CD changes

@pyranota pyranota marked this pull request as ready for review October 23, 2024 17:58
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 0a1b134 in 1 minute and 26 seconds

More details
  • Looked at 677 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. backend/windmill-worker/nsjail/download_deps.py.pip.sh:1
  • Draft comment:
    Add a shebang line to specify the script interpreter.
#!/bin/sh
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The download_deps.py.pip.sh script is missing a shebang line. This can cause issues when the script is executed in environments that rely on the shebang to determine the interpreter.
2. backend/windmill-worker/nsjail/download_deps.py.pip.sh:24
  • Draft comment:
    Avoid using eval to execute commands as it can be a security risk. Consider using exec or running the command directly.
exec $CMD
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment addresses a potential security risk with using 'eval'. The suggestion to use 'exec' is a common practice to avoid such risks. However, 'exec' changes the script's behavior by not returning control to the script after execution, which might not be the intended behavior. The comment is about a change made in the diff, specifically the use of 'eval'.
    The comment assumes that replacing 'eval' with 'exec' is always a better choice, but it doesn't consider the change in script behavior. The author might have intended to use 'eval' for a specific reason.
    While 'exec' changes the behavior, the security risk associated with 'eval' is significant enough to warrant the suggestion. The author can decide if the behavior change is acceptable.
    Keep the comment as it highlights a valid security concern with using 'eval' and provides an actionable suggestion to mitigate it.
3. backend/windmill-worker/nsjail/download_deps.py.sh:37
  • Draft comment:
    Avoid using eval to execute commands as it can be a security risk. Consider using exec or running the command directly.
exec $CMD
  • Reason this comment was not posted:
    Marked as duplicate.
4. backend/windmill-worker/src/ansible_executor.rs:119
  • Draft comment:
    Consider using the appropriate logic to determine the value of no_uv_install instead of hardcoding false.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is suggesting a potential improvement by not hardcoding a value. This is a valid suggestion as it could improve code flexibility and maintainability. The comment is directly related to the change made in the diff, which is the addition of the false argument.
    The comment does not provide specific guidance on what logic should be used, which might make it less actionable. However, it does highlight a potential area for improvement.
    While the comment could be more specific, it still points out a hardcoded value that could be improved, which is a valid code quality suggestion.
    The comment is valid as it points out a hardcoded value that could be improved by using logic to determine its value. It should be kept.
5. backend/windmill-worker/src/global_cache.rs:32
  • Draft comment:
    Consider refactoring the logic for determining the prefix into a helper function to improve code maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The build_tar_and_push function in global_cache.rs uses a conditional to determine the prefix for the tar path. This logic is repeated in multiple places and could be refactored into a helper function to improve maintainability.
6. backend/windmill-worker/src/global_cache.rs:99
  • Draft comment:
    Consider refactoring the logic for determining the tar path into a helper function to improve code maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The pull_from_tar function in global_cache.rs uses a conditional to determine the tar path. This logic is repeated in multiple places and could be refactored into a helper function to improve maintainability.

Workflow ID: wflow_Dt3hrydoH4g28DWs


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

@pyranota
Copy link
Collaborator Author

Ready for review, but not ready to merge (Windows testing missing)

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 7b72b0c in 41 seconds

More details
  • Looked at 64 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. backend/windmill-worker/src/python_executor.rs:1151
  • Draft comment:
    Using unwrap_or_else is safer than unwrap as it provides a default value in case of an error. However, consider logging the error for better debugging. This applies to other instances of unwrap in the code as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses unwrap in multiple places, which can cause the program to panic if the value is None. This is not a safe practice, especially in production code. It's better to handle these cases gracefully.
2. backend/windmill-worker/src/python_executor.rs:1265
  • Draft comment:
    Using unwrap_or_else is safer than unwrap as it provides a default value in case of an error. However, consider logging the error for better debugging. This applies to other instances of unwrap in the code as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses unwrap in multiple places, which can cause the program to panic if the value is None. This is not a safe practice, especially in production code. It's better to handle these cases gracefully.
3. backend/windmill-worker/src/python_executor.rs:1275
  • Draft comment:
    Using unwrap_or_else is safer than unwrap as it provides a default value in case of an error. However, consider logging the error for better debugging. This applies to other instances of unwrap in the code as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses unwrap in multiple places, which can cause the program to panic if the value is None. This is not a safe practice, especially in production code. It's better to handle these cases gracefully.
4. backend/windmill-worker/src/python_executor.rs:1291
  • Draft comment:
    Using unwrap_or_else is safer than unwrap as it provides a default value in case of an error. However, consider logging the error for better debugging. This applies to other instances of unwrap in the code as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses unwrap in multiple places, which can cause the program to panic if the value is None. This is not a safe practice, especially in production code. It's better to handle these cases gracefully.
5. backend/windmill-worker/src/python_executor.rs:1313
  • Draft comment:
    Using unwrap_or_else is safer than unwrap as it provides a default value in case of an error. However, consider logging the error for better debugging. This applies to other instances of unwrap in the code as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses unwrap in multiple places, which can cause the program to panic if the value is None. This is not a safe practice, especially in production code. It's better to handle these cases gracefully.

Workflow ID: wflow_tJyLaeHTlFzZ1pJX


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants