-
Notifications
You must be signed in to change notification settings - Fork 8.2k
feat: update dependencies to support new mcp-composer version #10644
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
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughUpdates dependency constraints across multiple pyproject.toml files (loosening weaviate-client and pywin32 pins, adding fastmcp, updating mcp and pydantic, relaxing cachetools). Modifies MCP Composer startup invocation from uvx-based to Python module execution with added debug logging. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant install_mcp_config
participant ProcessManager
participant Subprocess
Note over Caller,Subprocess: Old Flow (uvx-based)
Caller->>install_mcp_config: install_mcp_config()
install_mcp_config->>ProcessManager: Launch uvx mcp-composer
ProcessManager->>Subprocess: exec(["uvx", "mcp-composer", ...])
Subprocess-->>ProcessManager: Process output
ProcessManager-->>install_mcp_config: Return
Note over Caller,Subprocess: New Flow (Python module)
Caller->>install_mcp_config: install_mcp_config()
install_mcp_config->>ProcessManager: Launch Python module
rect rgb(220, 240, 255)
ProcessManager->>Subprocess: exec(["uv", "run", "python", "-m", "mcp_composer", ...])
end
rect rgb(240, 255, 240)
Subprocess-->>ProcessManager: Process output + debug logs
ProcessManager->>ProcessManager: Non-blocking read stdout/stderr
ProcessManager->>ProcessManager: Log command (obfuscated), PID
end
ProcessManager-->>install_mcp_config: Return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (39.58%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## main #10644 +/- ##
==========================================
- Coverage 39.02% 31.46% -7.57%
==========================================
Files 1482 1330 -152
Lines 85917 63725 -22192
Branches 10347 10346 -1
==========================================
- Hits 33531 20052 -13479
+ Misses 51333 42620 -8713
Partials 1053 1053
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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: 2
🧹 Nitpick comments (2)
src/backend/base/langflow/api/v1/mcp_projects.py (1)
631-646: uv-based MCP Composer command looks correct; consider deduping and verify flagsSwitching the Composer path to
uv run python -m mcp_composer --mode stdio --sse-url ...is consistent with the new module-based startup flow and still uses a dynamically computed SSE URL (good for non-local deployments, based on learnings). To avoid the two call sites drifting over time, it may be worth centralizing the construction of the Composer command/args soinstall_mcp_config()and_start_project_composer_process()share the same logic. Also, please double‑check that the--mode,--sse-url, and--disable-composer-toolsflags match the MCP Composer version you’re targeting and thatuvis on PATH in environments where this config is installed.pyproject.toml (1)
30-30: Core dependency ranges updated; consider aligning cachetools stubs with runtimeThe new ranges (
weaviate-client>=4.10.2,<5.0.0,pywin32>=307,<400; sys_platform == "win32", plusfastmcp==2.13.0andmcp>=1.17.0,<2.0.0) look coherent with the backend/base constraints and the MCP Composer changes in this PR. One thing to keep an eye on: other pyprojects now requirecachetools>=6.0.0, while dev dependencytypes-cachetools>=5.5.0.20240820,<6.0.0still targets pre‑6.x. If you plan to run cachetools 6.x everywhere, it may be worth updating the stub constraint to cover 6.x as well (or confirming that the existing stubs still match the new API).Also applies to: 40-40, 108-109
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
pyproject.toml(3 hunks)src/backend/base/langflow/api/v1/mcp_projects.py(1 hunks)src/backend/base/pyproject.toml(2 hunks)src/lfx/pyproject.toml(1 hunks)src/lfx/src/lfx/services/mcp_composer/service.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T21:19:22.567Z
Learnt from: deon-sanchez
Repo: langflow-ai/langflow PR: 9158
File: src/backend/base/langflow/api/v1/mcp_projects.py:404-404
Timestamp: 2025-07-23T21:19:22.567Z
Learning: In langflow MCP projects configuration, prefer using dynamically computed URLs (like the `sse_url` variable) over hardcoded localhost URLs to ensure compatibility across different deployment environments.
Applied to files:
src/backend/base/langflow/api/v1/mcp_projects.py
🪛 GitHub Actions: Ruff Style Check
src/lfx/src/lfx/services/mcp_composer/service.py
[error] 420-420: F841 Local variable settings is assigned to but never used.
🪛 GitHub Check: Ruff Style Check (3.13)
src/lfx/src/lfx/services/mcp_composer/service.py
[failure] 481-481: Ruff (T201)
src/lfx/src/lfx/services/mcp_composer/service.py:481:9: T201 print found
[failure] 472-472: Ruff (T201)
src/lfx/src/lfx/services/mcp_composer/service.py:472:9: T201 print found
[failure] 547-547: Ruff (T201)
src/lfx/src/lfx/services/mcp_composer/service.py:547:25: T201 print found
[failure] 534-534: Ruff (T201)
src/lfx/src/lfx/services/mcp_composer/service.py:534:25: T201 print found
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Lint Backend / Run Mypy (3.11)
- GitHub Check: Lint Backend / Run Mypy (3.12)
- GitHub Check: Lint Backend / Run Mypy (3.13)
- GitHub Check: Lint Backend / Run Mypy (3.10)
- GitHub Check: Run Frontend Tests / Determine Test Suites and Shard Distribution
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 4
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 5
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 2
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 1
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 3
- GitHub Check: Run Backend Tests / Integration Tests - Python 3.10
- GitHub Check: Run Backend Tests / LFX Tests - Python 3.10
- GitHub Check: Test Starter Templates
- GitHub Check: Run Ruff Check and Format
- GitHub Check: Update Component Index
- GitHub Check: Update Starter Projects
- GitHub Check: Optimize new Python code in this PR
🔇 Additional comments (2)
src/lfx/pyproject.toml (1)
29-29: cachetools bump to 6.x looks consistent; please verify stub/runtime alignmentRaising the minimum to
cachetools>=6.0.0(and dropping the upper bound) matches the rest of the repo, but the rootpyproject.tomlstill pinstypes-cachetools>=5.5.0.20240820,<6.0.0. If you intend to run cachetools 6.x everywhere, consider checking that those stubs still match the 6.x API or relaxing their upper bound accordingly.src/backend/base/pyproject.toml (1)
35-35: Dependency range updates (pydantic, cachetools, mCP) – ensure compatibility is verifiedThe updated constraints (
pydantic~=2.11.0,cachetools>=6.0.0,mcp>=1.17.0,<2.0.0) align with the rest of the PR, but they touch core runtime pieces (validation, caching, MCP protocol). Please make sure your backend tests (including MCP-related flows) are run against these versions, and check any pinned plugins/extensions that might depend on earlier versions.Also applies to: 39-39, 83-83
| """Start the MCP Composer subprocess for a specific project.""" | ||
| settings = get_settings_service().settings | ||
| cmd = [ | ||
| "uvx", |
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.
Remove unused settings and temporary print debug to fix Ruff failures
Ruff is correctly flagging two issues here:
- Line 420:
settings = get_settings_service().settingsis assigned but never used (F841). - Lines 472, 481, 534, and 547:
print(...)calls (T201) duplicate information that’s already being logged vialogger.adebug/logger.aerror.
Since logging already covers these messages, the print calls can be dropped, and the unused settings assignment removed to unblock the style check and avoid confusion about unused configuration.
A minimal cleanup could look like:
@@ async def _start_project_composer_process(
- """Start the MCP Composer subprocess for a specific project."""
- settings = get_settings_service().settings
- cmd = [
+ """Start the MCP Composer subprocess for a specific project."""
+ cmd = [
"uv",
"run",
"python",
"-m",
"mcp_composer",
@@
- # TEMPORARY DEBUG: Print command being executed
- safe_cmd = self._obfuscate_command_secrets(cmd)
- await logger.adebug(f"Starting MCP Composer with command: {' '.join(safe_cmd)}")
- print(f"[MCP Composer] Starting command: {' '.join(safe_cmd)}")
+ safe_cmd = self._obfuscate_command_secrets(cmd)
+ await logger.adebug(f"Starting MCP Composer with command: {' '.join(safe_cmd)}")
@@
- await logger.adebug(f"Monitoring MCP Composer startup for project {project_id} (PID: {process.pid})")
- print(f"[MCP Composer] Process started with PID: {process.pid}")
+ await logger.adebug(f"Monitoring MCP Composer startup for project {project_id} (PID: {process.pid})")
@@
- if process.stderr and select.select([process.stderr], [], [], 0)[0]:
+ if process.stderr and select.select([process.stderr], [], [], 0)[0]:
try:
stderr_line = process.stderr.readline()
- if stderr_line:
- print(f"[MCP Composer STDERR] {stderr_line.strip()}")
+ if stderr_line:
@@
- if process.stdout and select.select([process.stdout], [], [], 0)[0]:
+ if process.stdout and select.select([process.stdout], [], [], 0)[0]:
try:
stdout_line = process.stdout.readline()
- if stdout_line:
- print(f"[MCP Composer STDOUT] {stdout_line.strip()}")
+ if stdout_line:(Only the print(...) lines are removed; the select calls and logger usage remain unchanged for now.)
This should clear the current Ruff errors without changing runtime behavior.
Also applies to: 470-473, 481-481, 534-535, 547-548
🧰 Tools
🪛 GitHub Actions: Ruff Style Check
[error] 420-420: F841 Local variable settings is assigned to but never used.
🪛 GitHub Check: Ruff Style Check (3.13)
[failure] 420-420: Ruff (F841)
src/lfx/src/lfx/services/mcp_composer/service.py:420:9: F841 Local variable settings is assigned to but never used
🤖 Prompt for AI Agents
In src/lfx/src/lfx/services/mcp_composer/service.py around lines 420 (and also
affecting 470-473, 481, 534-535, 547-548): remove the unused assignment
"settings = get_settings_service().settings" at line 420 and delete the
temporary print(...) debug calls at the listed locations so Ruff F841 and T201
warnings are resolved; keep the existing logger.adebug/logger.aerror and select
calls intact so behavior is unchanged.
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.
🧩 Analysis chain
Guard select.select on subprocess pipes for Windows compatibility
The nonblocking reads use:
if process.stderr and select.select([process.stderr], [], [], 0)[0]:
...
if process.stdout and select.select([process.stdout], [], [], 0)[0]:
...On Unix this is fine, but on Windows select.select only supports sockets, not regular file handles, so these calls can raise and break composer startup even though they’re only for debug output.
Given this is auxiliary logging, consider guarding the select usage, e.g.:
- if process.stderr and select.select([process.stderr], [], [], 0)[0]:
- try:
- stderr_line = process.stderr.readline()
- if stderr_line:
- ...
- except Exception: # noqa: BLE001
- pass
+ try:
+ if process.stderr and os.name != "nt" and select.select([process.stderr], [], [], 0)[0]:
+ stderr_line = process.stderr.readline()
+ if stderr_line:
+ ...
+ except Exception: # noqa: BLE001
+ pass
@@
- if process.stdout and select.select([process.stdout], [], [], 0)[0]:
- try:
- stdout_line = process.stdout.readline()
- if stdout_line:
- ...
- except Exception: # noqa: BLE001
- pass
+ try:
+ if process.stdout and os.name != "nt" and select.select([process.stdout], [], [], 0)[0]:
+ stdout_line = process.stdout.readline()
+ if stdout_line:
+ ...
+ except Exception: # noqa: BLE001
+ pass(or any equivalent pattern that skips the select path on Windows).
That way, startup behavior remains robust across platforms while still providing extra debug output where supported.
I'll verify the concerns in the review comment by examining the actual code and checking Windows compatibility of select.select().
🏁 Script executed:
#!/bin/bash
# Examine the actual code at lines 528-550 and surrounding context
cat src/lfx/src/lfx/services/mcp_composer/service.py | sed -n '510,570p'Length of output: 3120
🏁 Script executed:
#!/bin/bash
# Check imports at the top of the file
head -50 src/lfx/src/lfx/services/mcp_composer/service.py | grep -E "^import|^from"Length of output: 291
🏁 Script executed:
#!/bin/bash
# Search for all select usage in the file
rg -n "select\." src/lfx/src/lfx/services/mcp_composer/service.pyLength of output: 169
🏁 Script executed:
#!/bin/bash
# Check if os module is imported
rg -n "^import os|^from os" src/lfx/src/lfx/services/mcp_composer/service.pyLength of output: 11
🏁 Script executed:
#!/bin/bash
# Check the full method containing these lines to understand context
sed -n '500,600p' src/lfx/src/lfx/services/mcp_composer/service.pyLength of output: 5560
🌐 Web query:
Python select.select Windows compatibility file handles pipes
💡 Result:
Short answer: On Windows select.select only works with sockets — it cannot wait on arbitrary file handles or pipes. Use one of these alternatives:
- Use threads: read blocking pipes (subprocess stdout/stderr) in a background thread and send data to your main loop. (stackoverflow.com)
- Use asyncio’s Windows proactor-based APIs (ProactorEventLoop / asyncio.create_subprocess_exec) which use overlapped I/O for pipes. (docs.python.org)
- Use Windows-specific APIs (pywin32 / win32file, overlapped I/O or ctypes) to wait on Win32 handles. (stackoverflow.com)
If you tell me which approach you prefer (threads, asyncio, or native Win32), I can show a short example.
Citations:
- 1: https://stackoverflow.com/questions/22251809/python-select-select-on-windows?utm_source=openai
- 2: https://docs.python.org/3/library/asyncio-subprocess.html?utm_source=openai
- 3: https://stackoverflow.com/questions/375427/a-non-blocking-read-on-a-subprocess-pipe-in-python?utm_source=openai
Guard select.select() calls for Windows compatibility—startup will fail on Windows
The code at lines 530 and 543 calls select.select() on subprocess pipes outside the try/except block:
if process.stderr and select.select([process.stderr], [], [], 0)[0]:
try:
stderr_line = process.stderr.readline()
...
except Exception: # noqa: BLE001
passOn Windows, select.select() does not support pipes or regular file handles—it only works with sockets. This causes an exception during startup, breaking the MCP Composer initialization on Windows machines, even though this is only auxiliary debug logging.
The exception handling only wraps readline(), not the select() call itself, so the error is uncaught.
Wrap the select calls in a platform guard or exception handler, for example:
if process.stderr and os.name != "nt" and select.select([process.stderr], [], [], 0)[0]:
try:
stderr_line = process.stderr.readline()
...
except Exception: # noqa: BLE001
passApply the same pattern to the process.stdout block at line 543.
🧰 Tools
🪛 GitHub Check: Ruff Style Check (3.13)
[failure] 547-547: Ruff (T201)
src/lfx/src/lfx/services/mcp_composer/service.py:547:25: T201 print found
[failure] 534-534: Ruff (T201)
src/lfx/src/lfx/services/mcp_composer/service.py:534:25: T201 print found
🤖 Prompt for AI Agents
In src/lfx/src/lfx/services/mcp_composer/service.py around lines 528-550, the
calls to select.select() on process.stderr and process.stdout are unguarded and
will raise on Windows (select doesn't support pipes), causing startup to fail;
modify both blocks to avoid calling select.select() on non-socket handles by
guarding with a platform check (e.g., skip select when os.name == "nt") or
wrapping the select.select() call in a try/except so any exception is caught
before attempting readline(), keeping the existing readline() try/except around
I/O intact; apply the same guard to both the stderr and stdout blocks.
|
Merging this into this - #10626 |
Updates mcp-composer and dependencies.
Summary by CodeRabbit