Handle shared installer venvs by Python/platform compatibility - #263
Merged
Conversation
arjunsuresh
requested changes
Jul 18, 2026
arjunsuresh
left a comment
Contributor
There was a problem hiding this comment.
We should call the test in a suitable github action test
Contributor
|
@copilot resolve the merge conflicts in this pull request |
Copilot
AI
changed the title
[WIP] Fix venv detection for platform compatibility
Handle shared installer venvs by Python/platform compatibility
Jul 18, 2026
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
🤖 AI PR Review Summary\n\nRefactor the Unix installer script to improve virtual environment (venv) compatibility handling by introducing platform and Python version suffixes to venv directories. This avoids overwriting incompatible venvs by creating or reusing platform- and Python-specific sibling directories. The PR centralizes architecture normalization and Python version extraction, enhances venv compatibility checks, and updates the GitHub workflow and documentation accordingly. Risks include potential edge cases in architecture normalization and compatibility detection, but extensive test coverage mitigates these. The design improves robustness and user experience when managing multiple Python environments. |
🤖 AI PR Review Summary\n\nRefactor the Unix installer script to improve virtual environment (venv) compatibility handling by introducing platform and Python version suffixes for venv directories. This avoids conflicts when multiple Python versions or architectures are used. The changes include defining helper functions for architecture normalization, Python version extraction, and compatibility checks. The installer now resolves and uses a compatible venv directory, removing stale incompatible ones. The workflow and documentation are updated accordingly. Risks include potential edge cases in architecture normalization and compatibility checks, but tests are added to validate behavior. |
Contributor
Author
Contributor
|
@copilot please address the above review comment |
Contributor
Author
🤖 AI PR Review Summary\n\nThis PR improves the mlcflow Unix installer and related CI workflows by enhancing Python virtual environment compatibility handling based on platform and Python minor version. It introduces functions to normalize architecture names, generate compatibility signatures, and resolve or fallback virtual environment directories accordingly. The installer script now uses a configurable Python command variable and better logs compatibility decisions. The repo_action.py adds validation for extra_git_args to prevent injection of newlines or carriage returns and ensures type correctness. Minor fixes in GitHub workflows improve code clarity and robustness. Risks include potential edge cases in architecture normalization and compatibility checks, and the complexity of venv resolution logic, but overall changes improve maintainability and user experience. |
🤖 AI PR Review Summary\n\nThis PR improves the mlcflow Unix installer and related CI workflows by enhancing Python virtual environment compatibility checks and management. It introduces architecture normalization, Python version detection helpers, and a more robust mechanism to select or create compatible virtual environments based on platform and Python minor version. The PR also refactors subprocess error handling and adds validation for extra git arguments in repo actions. Documentation is updated to explain the new venv directory suffix behavior. Risks include potential edge cases in architecture normalization and compatibility detection, and the need to ensure backward compatibility with existing venvs. Overall, the design improves maintainability and user experience by avoiding venv conflicts across Python versions and platforms. |
Contributor
|
@copilot the format of the AI PR review comments are broken. Please fix it |
Contributor
Author
🤖 AI PR Review Summary\n\nThis PR improves the AI PR review GitHub workflow by adding robust validation and base64 encoding/decoding for inline comments to handle special characters safely. It also refactors the Unix installer script to better handle Python virtual environments by detecting platform and Python minor version compatibility, creating or reusing suffixed venv directories accordingly. The installer now uses a configurable Python command variable and normalizes architecture names. Documentation is updated to explain the new venv suffix behavior. Risks include potential shell compatibility issues with the new base64 decoding approach and subtle bugs in venv compatibility detection, but overall the changes improve reliability and maintainability. |
arjunsuresh
marked this pull request as ready for review
July 19, 2026 10:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Unix installer currently reuses any existing venv at the requested path, which breaks when
$HOMEis shared across architectures or Python minor versions. This updates reuse logic to validate venv compatibility first and fall back to a versioned sibling path when needed.Compatibility check
architecture|major.minor.Deterministic fallback naming
mlcflow_x86_64_py3.11mlcflow_aarch64_py3.12Installer behavior
Coverage and docs
mainflowExample
✅ PR Checklist
dev📌 Note: PRs must be raised against
dev. Do not commit directly tomain.✅ Testing & CI
📚 Documentation
📁 File Hygiene & Output Handling
🛡️ Safety & Security
🙌 Contribution Hygiene