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(python): make S3 cache arch specific #5196

Merged
merged 6 commits into from
Feb 3, 2025
Merged

Conversation

pyranota
Copy link
Collaborator

@pyranota pyranota commented Feb 3, 2025

Current schema:
image

New schema:
image


Important

Make S3 cache paths architecture-specific and adjust logging level in uv_pip_compile.

  • Behavior:
    • Introduces TARGET constant in global_cache.rs to make S3 cache paths architecture-specific.
    • Updates paths in build_tar_and_push and pull_from_tar to include TARGET.
  • Logging:
    • Changes logging level from error to debug in uv_pip_compile in python_executor.rs.

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

Current schema:

S3-Bucket
├── python_311
│   ├── wheel==1.0
│   └── wheel2==1.0
└── python_312
    ├── wheel==1.0
    └── wheel2==1.0

New schema:

S3-Bucket
├── linux_aarch64
│   └── ...
└── linux_x86_64
    ├── python_311
    │   ├── wheel==1.0
    │   └── wheel2==1.0
    └── python_312
        ├── wheel==1.0
        └── wheel2==1.0
Copy link

cloudflare-workers-and-pages bot commented Feb 3, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: dfc7f12
Status: ✅  Deploy successful!
Preview URL: https://a618a76e.windmill.pages.dev
Branch Preview URL: https://py-multiarch-s3-cache.windmill.pages.dev

View logs

@pyranota pyranota marked this pull request as ready for review February 3, 2025 17:26
@pyranota pyranota requested a review from rubenfiszel as a code owner February 3, 2025 17:26
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 dfc7f12 in 1 minute and 50 seconds

More details
  • Looked at 45 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. backend/windmill-worker/src/global_cache.rs:62
  • Draft comment:
    Consider using named parameters for clarity in the S3 path. The format string "/tar/{TARGET}/{}/{folder_name}.tar" relies on auto-captured identifiers (TARGET and folder_name) along with a positional placeholder. Explicitly naming all parameters (e.g. using "{target}", "{python}", "{folder_name}") can improve readability and reduce potential bugs.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    While named parameters can improve readability, this case is fairly straightforward. The format string is short and the meaning is clear from context. The positional parameter comes from an inline if expression, making it natural to use a positional parameter. The code is consistent in both places it appears. No real risk of bugs since the format is simple.
    The comment does identify a real coding practice issue. Named parameters can make code more maintainable in the long run.
    However, this is a minor stylistic suggestion that doesn't materially improve the code quality or reduce bug risk in this specific case. The current format is clear and consistent.
    Delete this comment as it suggests a minor stylistic change that doesn't meaningfully improve the code quality or reduce bugs in this specific case.
2. backend/windmill-worker/src/python_executor.rs:635
  • Draft comment:
    Changing log level from 'error' to 'debug' for uv args is a good improvement to reduce noise in production logs.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
3. backend/windmill-worker/src/global_cache.rs:62
  • Draft comment:
    Using {TARGET} in the S3 key path now makes caching arch-specific. Ensure that TARGET is computed correctly across all platforms (consider including OS if needed).
  • Reason this comment was not posted:
    Comment did not seem useful.
4. backend/windmill-worker/src/python_executor.rs:635
  • Draft comment:
    Changed uv args logging from error to debug to reduce noise. This is appropriate if the log is not indicating an error.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_FFCIZzVPtmEzkybM


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

@pyranota pyranota marked this pull request as draft February 3, 2025 17:32
@pyranota pyranota marked this pull request as ready for review February 3, 2025 17:41
@rubenfiszel rubenfiszel merged commit 0e80775 into main Feb 3, 2025
7 of 8 checks passed
@rubenfiszel rubenfiszel deleted the py-multiarch-s3-cache branch February 3, 2025 18:31
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 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