-
Notifications
You must be signed in to change notification settings - Fork 90
feat(core): Modernize Dependency Management with Auto-UVX #284
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
base: main
Are you sure you want to change the base?
feat(core): Modernize Dependency Management with Auto-UVX #284
Conversation
- Remove unused 'is_non_interactive' import in install.py - Use stderr for installation status message in install.py
- Upgrades execution to 'uv run' if uv is detected - Solves dependency isolation and Pydantic version conflicts - Eliminates need for pip install step for Python servers
WalkthroughThe changes introduce non-interactive mode support across install and uninstall commands by extending the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mcpm/commands/install.py (2)
124-126: Passforceto recursive call for consistency.The recursive call omits the
forceparameter. While currently unreachable in non-interactive mode (the function returns early), passing it maintains consistency and prevents subtle bugs if the logic changes.if not result.strip() and required and not default: console.print("[yellow]Warning: Required value cannot be empty.[/]") - return prompt_with_default(prompt_text, default, hide_input, required) + return prompt_with_default(prompt_text, default, hide_input, required, force)
446-451: Remove or implement the-mheuristic block.This block contains only a
passstatement and extensive comments describing a heuristic that's never executed. Either implement the module-to-package name inference or remove this dead code to avoid confusion.- # If args start with '-m', the next arg is the module. - # Often module == package (e.g. mcp_server_time -> mcp-server-time? No, dashes vs underscores). - # But 'uv run --with <module> python -m <module>' usually works if PyPI name matches. - - if not target_package and mcp_args and mcp_args[0] == "-m" and len(mcp_args) > 1: - # Heuristic: Assume package name matches module name (with _ -> - maybe?) - # Ideally, the registry should provide 'package'. - # For now, we only auto-upgrade if we have a package name OR if we are brave. - # Let's rely on package_name variable extracted earlier from selected_method.get("package") - pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcpm/commands/install.py(8 hunks)src/mcpm/commands/uninstall.py(2 hunks)src/mcpm/utils/non_interactive.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Always format Python code with
ruff.
Files:
src/mcpm/commands/install.pysrc/mcpm/utils/non_interactive.pysrc/mcpm/commands/uninstall.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: pathintegral-institute/mcpm.sh PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T08:20:00.958Z
Learning: Use `uv` for all Python dependency management.
🧬 Code graph analysis (2)
src/mcpm/commands/install.py (2)
src/mcpm/utils/non_interactive.py (1)
should_force_operation(35-45)src/mcpm/schemas/full_server_config.py (1)
to_server_config(72-84)
src/mcpm/commands/uninstall.py (1)
src/mcpm/utils/non_interactive.py (1)
should_force_operation(35-45)
🔇 Additional comments (4)
src/mcpm/commands/uninstall.py (1)
10-10: LGTM! Consistent non-interactive handling.The integration of
should_force_operation(force)correctly unifies force behavior with the install command, respecting both the CLI--forceflag and theMCPM_FORCEenvironment variable.Also applies to: 63-63
src/mcpm/utils/non_interactive.py (1)
35-45: LGTM! Clean extension of the force operation check.The backward-compatible signature change with a default value and the OR logic correctly enables force behavior from either CLI flags or environment variables. The case-insensitive env var check is a good practice.
src/mcpm/commands/install.py (2)
155-156: LGTM! Comprehensive non-interactive mode integration.Good implementation:
- Progress messages to stderr keeps stdout clean for structured output
- Confirmation dialogs and method selection correctly gated by
should_force_operation(force)- Force semantics properly propagated to
global_add_serverBased on learnings, the use of
uvfor Python dependency management aligns with project guidelines.Also applies to: 180-182, 227-227, 481-481
435-435: I apologize for the cloning failure. Let me reconsider based on the information available.Looking at the review comment and the code context provided, the concern about
piphandling is logically sound even without examining the registry files:Analysis of the concern:
- Line 435 checks:
if mcp_command in ["python", "python3", "pip"] and shutil.which("uv"):- The subsequent logic (lines 453-464) wraps the command:
uv run --with <package> <original_command> <args>- For
pipcommands, this would produce:uv run --with <pkg> pip install ...- This is semantically incorrect because
uv run --withis designed for running Python modules/executables, not for invoking package managersThe concern is valid because:
uv run --with <pkg>is meant to run Python code with additional dependencies, not to wrap arbitrary commandspipis a package management tool and should not be treated the same aspythonorpython3executables- If this is executed, it would either fail or produce unexpected behavior
Since I cannot access the repository to verify if
pipis actually used in registry entries, but the concern itself is technically correct, I'll preserve the original review comment with validation that the concern is sound.Reconsider including
pipin auto-UVX upgrade.Wrapping
pipcommands withuv run --with <pkg> pip ...is semantically incorrect. Theuv run --withpattern is designed for running Python executables/modules, not for package installation commands. Either excludepipfrom this check, or ifpipcommands in the registry actually represent something else (unlikely), add a clarifying comment explaining the intent.
User description
This PR introduces automatic dependency isolation for Python-based MCP servers by leveraging
uv.Key Changes
piporpythonexecution, anduvis detected on the system, MCPM automatically upgrades the execution command touv run --with <package>.uv.Why this matters
This allows the MCPM Core to be upgraded to the latest dependencies (Pydantic v2, modern MCP SDK) without breaking compatibility with older servers that require legacy libraries.
PR Type
Enhancement, Bug fix
Description
Implement Auto-UVX injection for Python servers using
uv run --withfor dependency isolationConsolidate force operation logic with
should_force_operation()helper functionAdd explicit non-interactive mode support via
MCPM_NON_INTERACTIVEenvironment variableRedirect installation status message to stderr for cleaner output
Diagram Walkthrough
File Walkthrough
install.py
Auto-UVX injection and non-interactive mode supportsrc/mcpm/commands/install.py
shutilimport andshould_force_operationutility import forconsolidated force logic
prompt_with_default()to support explicit non-interactive modevia
MCPM_NON_INTERACTIVEenvironment variableinstance
forceflag checks withshould_force_operation(force)calls throughout
uvavailability andupgrades
python/pipcommands touv run --withfor dependencyisolation
uninstall.py
Consolidate force operation logicsrc/mcpm/commands/uninstall.py
should_force_operationutility functionforceflag check withshould_force_operation(force)call for confirmation logic
non_interactive.py
Add CLI force flag parameter to should_force_operationsrc/mcpm/utils/non_interactive.py
should_force_operation()signature to accept optionalcli_force_flagparameterMCPM_FORCEenvironment variable is setSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.