fix(python): assign PATCH version to python runtime only when needed

[pyranota]

Important

Modify Python version resolution to assign PATCH version only when necessary, with updates to tests and logic in python_versions.rs and worker.rs.

• Behavior:
  • Modify resolve() in python_versions.rs to assign PATCH version only if version specifiers or gravitational version include PATCH.
  • Update test_requirements_python, test_extra_requirements_python, test_extra_requirements_python2, and test_pins_python in worker.rs to reflect new behavior.
• Tests:
  • Add tests test_python_resolution_8 to test_python_resolution_16 in worker.rs to verify new version resolution logic.
• Misc:
  • Minor formatting changes in python_versions.rs.

^{This description was created by for a9d5639. You can customize this summary. It will automatically update as commits are pushed.}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	4af8023
Status:	✅  Deploy successful!
Preview URL:	https://9dc2acd6.windmill.pages.dev
Branch Preview URL:	https://fix-multipython-v2-specific.windmill.pages.dev

View logs

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 1a9d878 in 3 minutes and 45 seconds. Click for details.

• Reviewed 307 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 8 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/src/python_versions.rs:156

• Draft comment:
  Minor: The added parentheses in the filter closure (vs) are redundant and do not change functionality.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct that the parentheses are redundant, this is an extremely minor style issue that doesn't affect code functionality or readability in any meaningful way. The rules state not to make comments that are obvious or unimportant. This seems to fall into that category. The parentheses could potentially be there for a specific reason that's not obvious from this context, like making the code more consistent with other similar patterns in the codebase. Even if the parentheses are part of a consistent style pattern, removing them is such a minor change that it's not worth a PR comment. This kind of minor style issue would be better handled by an automated formatter. This comment should be deleted as it addresses an extremely minor style issue that doesn't meaningfully impact the code.

2. backend/windmill-worker/src/python_versions.rs:161

• Draft comment:
  Using 'result.clone()' instead of 'valid[0].clone()' in the 'if select_latest' branch makes the intent clearer and is functionally equivalent.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

3. backend/windmill-worker/src/python_versions.rs:193

• Draft comment:
  Good use of destructuring to extract MAJOR and MINOR from the version. Ensure that every version’s release has at least two elements (MAJOR and MINOR) since the error message relies on that assumption.
• 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.

4. backend/windmill-worker/src/python_versions.rs:217

• Draft comment:
  The update of 'newest_in_minor' when a new minor version is encountered assumes that the available versions are in descending order. If custom_versions are provided unsorted, this logic might not behave as expected.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% Looking at the code, the assumption about sorted versions is actually documented in the code comment "Since we go top to down we can assume". The versions come from either custom_versions or PyV::list_available_python_versions(). Looking at list_available_python_versions(), it explicitly sorts versions with .sorted() before returning. So this assumption is handled by the implementation. The comment identifies a real potential issue with unsorted custom versions. However, is this actually a problem in practice given the sorting in list_available_python_versions()? The sorting is guaranteed by list_available_python_versions() which is the main source of versions. For custom_versions, the assumption is part of the function's contract and is documented in the code comment. The comment should be deleted since the sorting assumption is already documented in the code comment and is handled properly by the implementation.

5. backend/windmill-worker/src/python_versions.rs:223

• Draft comment:
  In the gravity matching branch, the conditional check using 'patch_vs' and checking if gv has a PATCH digit correctly determines whether to return the full version or a truncated (MAJOR.MINOR only) version. The use of pattern matching to extract release numbers is clean.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, praising the use of pattern matching and the logic in the code. It doesn't provide any actionable feedback or suggestions for improvement.

6. backend/windmill-worker/src/python_versions.rs:248

• Draft comment:
  The final branch compares the gravity version's (MAJOR, MINOR) to that of the candidate, and conditionally returns either the full version or a version constructed from only MAJOR and MINOR. This logic meets the intended fix. Consider renaming 'newest_in_minor' for clarity (e.g. 'candidate_minor') to better reflect its purpose.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The current name newest_in_minor is actually quite descriptive - it clearly indicates that this variable holds the newest version within a minor version range. The suggested name candidate_minor is more vague and doesn't capture that this specifically tracks the newest version. The current name aligns well with the comment on lines 197-204 that explains its purpose. I could be wrong about the clarity - maybe other developers would find 'candidate' more intuitive. The current name is also quite long which could be seen as a downside. While 'candidate' is shorter, 'newest_in_minor' is more precise and self-documenting. The length is justified by its descriptive value. The current variable name is actually more descriptive and clearer than the suggested alternative. This comment should be removed.

7. backend/windmill-worker/src/python_versions.rs:777

• Draft comment:
  Test expectations have been updated to reflect the new behavior (dropping the PATCH digit when not required). The additional tests (test_python_resolution_8 through test_python_resolution_16) improve coverage of various scenarios. Nice work!
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, describing what has been done in the PR regarding test expectations and additional tests. It does not provide any actionable feedback or suggestions for improvement.

8. backend/windmill-worker/src/python_versions.rs:245

• Draft comment:
  Typo: In the error message, the phrase "MAJOR nor MINOR version" is unusual. Consider rephrasing it to "Cannot get either MAJOR or MINOR version..." for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is about a changed line, it's purely about wording preference in an error message. The existing message is already clear and understandable. The suggested change doesn't fix any actual issues or improve code quality in a meaningful way. The current wording "MAJOR nor MINOR" is technically correct English and conveys the meaning clearly. The suggested change is just a stylistic preference. The comment doesn't identify any real problems - it's just suggesting an alternative phrasing that isn't necessarily better. Delete the comment since it's just about personal wording preference in an error message rather than identifying any actual issues that need fixing.

Workflow ID: wflow_fy9GCKR29jC0kkAe

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed b243255 in 1 minute and 37 seconds. Click for details.

• Reviewed 44 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 6 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. tests/worker.rs:4248

• Draft comment:
  This test file is very long and mixes tests for multiple languages/workflow features. Consider splitting it into multiple modules (e.g. one per supported language) to improve maintainability.
• 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. tests/worker.rs:3880

• Draft comment:
  Several places use .unwrap() on async results. Using .expect() with descriptive messages could help pinpoint errors when tests fail.
• 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. tests/worker.rs:3882

• Draft comment:
  In assert_lockfile, the expected lock file is built via join(" "). A comment on the expected format (and how it’s normalized) would be useful for future maintainers.
• 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.

4. tests/worker.rs:4092

• Draft comment:
  In test_inline_script_metadata_python, the expected version comment is hard-coded (# py: 3.12.1). A brief note on how this normalization is derived would clarify intent.
• 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. tests/worker.rs:4344

• Draft comment:
  In the job_payload module, lazy_static is used to define VERSION_FLAGS. Adding a short comment explaining their purpose (e.g. for testing conditional behavior based on version) would improve clarity.
• 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. tests/worker.rs:3435

• Draft comment:
  Timeout durations (e.g. 5 seconds) are used in several tests. Consider verifying that these values are robust in slower CI environments or add configuration for adjustable timeouts.
• 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_3UWZUFQl5Vn9G3Qb

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 4af8023 in 54 seconds. Click for details.

• Reviewed 17 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/src/python_versions.rs:253

• Draft comment:
  Refactored version construction improves readability by introducing a local variable. Consider adding a brief inline comment clarifying that only the major and minor parts are used when no patch version is specified.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_Swy9gzCDWKuLjnlP

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed a65c14b in 1 minute and 16 seconds. Click for details.

• Reviewed 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/src/python_versions.rs:991

• Draft comment:
  Removed duplicate 'pyv("2.3.1")' from the available versions in the test. This ensures test data is unambiguous and aligns with the intent of patch assignment only when required.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining the reason for removing a duplicate version in the test data. It doesn't suggest any changes or ask for confirmation on the intention. According to the rules, purely informative comments should be removed.

Workflow ID: wflow_HEMWeW2pELhLbOKe

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed a9d5639 in 1 minute and 6 seconds. Click for details.

• Reviewed 22 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/src/python_versions.rs:988

• Draft comment:
  The test test_python_resolution_15 was removed. This test verified that when the gravitational version was provided with a full PATCH (e.g. "2.3.0"), the resolve function returned the full patch version. Its removal suggests that the behavior now strips the patch unless explicitly needed. Please ensure that this change in behavior is well-documented and that existing tests (like test_python_resolution_16) cover all intended resolution scenarios.
• 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_gnwsaYAENrDrnFon

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}