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

fix: Fix python refetching wheels from S3 #5133

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

pyranota
Copy link
Collaborator

@pyranota pyranota commented Jan 24, 2025

Important

Adds creation of .valid.windmill file in handle_python_reqs() to indicate successful Python wheel installation, preventing unnecessary reinstalls.

  • Behavior:
    • In handle_python_reqs() in python_executor.rs, a file .valid.windmill is created after successful installation of Python wheels to indicate validity.
    • If file creation fails, logs an error with workspace and job IDs.
  • Error Handling:
    • Logs error to Sentry if .valid.windmill file creation fails, including workspace and job IDs.

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

@pyranota pyranota marked this pull request as ready for review January 24, 2025 22:34
@pyranota pyranota requested a review from rubenfiszel as a code owner January 24, 2025 22:34
Copy link

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: b5cd14a
Status: ✅  Deploy successful!
Preview URL: https://b84176a9.windmill.pages.dev
Branch Preview URL: https://fix-py-refetching-from-s3.windmill.pages.dev

View logs

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 b5cd14a in 53 seconds

More details
  • Looked at 23 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:2170
  • Draft comment:
    Avoid using newline characters within the format string in tracing::error! to prevent formatting issues. Consider using multiple arguments instead.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment points out a real issue - using newlines in format strings can make logs harder to parse and less consistent. However, this is a relatively minor code quality issue that doesn't affect functionality. The suggestion to use multiple arguments is also not clearly better since it would make the code more verbose.
    The comment may be too pedantic - while it points out a real issue, the current format is readable and works fine. The suggested change could actually make the code less readable.
    While the comment identifies a real logging best practice, the benefit of changing this is minimal and could reduce readability. The current format clearly expresses the error message's intent.
    Delete this comment since it suggests a change that would provide minimal benefit while potentially making the code less readable.

Workflow ID: wflow_5m9YCvHZUh65EOnx


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

@rubenfiszel rubenfiszel changed the title Fix: Fix python refetching wheels from S3 fix: Fix python refetching wheels from S3 Jan 25, 2025
@rubenfiszel rubenfiszel merged commit 2f1804b into main Jan 25, 2025
7 checks passed
@rubenfiszel rubenfiszel deleted the fix-py-refetching-from-s3 branch January 25, 2025 11:38
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 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