Repository navigation
fix(e2e): unbreak trtllm lanes broken by mcp 2.0.0a1 - #1675
Conversation
Every trtllm e2e job dies at the model-download step since the TensorRT-LLM 1.3.0rc18 bump (#1663): rc18 newly depends on mcp (unconstrained), the trtllm install runs pip with --pre (required for NVIDIA pre-release wheels), and pip resolves mcp to 2.0.0a1 — which removed the legacy mcp.server.fastmcp module. Our e2e dependency floor mcp>=1.0 treats the alpha as already satisfied, and e2e_test/infra eagerly imports the FastMCP-based mock at package import, so even ci_download_model.sh crashes with ModuleNotFoundError before any test runs. rc14 never depended on mcp, which is why this only surfaced now. Two fixes, both on our side of the contract: - Declare the real requirement: mcp>=1.2,<2 (fastmcp was added in 1.2 and removed in 2.0). PEP 440 excludes 2.0 pre-releases from <2, so the e2e deps install now replaces the alpha with stable 1.x instead of keeping it. TensorRT-LLM's own mcp requirement is unconstrained, so the downgrade stays consistent. - Stop importing the mock MCP server eagerly in infra/__init__: only the sglang-only responses lane uses it, yet the eager import made model download and every chat/router lane depend on the mcp SDK. The symbols now resolve lazily via module __getattr__. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request defers import of mock MCP server symbols in ChangesLazy Mock MCP Resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces lazy loading for mock MCP server symbols in e2e_test/infra/__init__.py to avoid importing the mcp SDK at import time, and restricts the mcp dependency version to mcp>=1.2,<2 in e2e_test/pyproject.toml. The reviewer suggested optimizing the lazy loading implementation by caching the resolved attributes in the module's globals to bypass subsequent __getattr__ lookups.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def __getattr__(name: str) -> object: | ||
| if name in _LAZY_MOCK_MCP: | ||
| from . import mock_mcp | ||
|
|
||
| return getattr(mock_mcp, name) | ||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
There was a problem hiding this comment.
To optimize module attribute lookups, we can cache the resolved attributes in the module's globals. This ensures that __getattr__ is only triggered once per attribute, and subsequent accesses will bypass __getattr__ entirely by retrieving the value directly from the module's namespace.
| def __getattr__(name: str) -> object: | |
| if name in _LAZY_MOCK_MCP: | |
| from . import mock_mcp | |
| return getattr(mock_mcp, name) | |
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") | |
| def __getattr__(name: str) -> object: | |
| if name in _LAZY_MOCK_MCP: | |
| from . import mock_mcp | |
| value = getattr(mock_mcp, name) | |
| globals()[name] = value | |
| return value | |
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
Summary
Every trtllm e2e job has been failing at the model-download step (before any test runs) since the TensorRT-LLM 1.3.0rc18 bump (#1663):
Root cause (with receipts from job 80871944044)
tensorrt-llm==1.3.0rc18newly depends onmcp(unconstrained) — rc14 did not (Collecting mcp (from tensorrt-llm==1.3.0rc18)).ci_install_trtllm.shinstalls withpip install --pre(required for NVIDIA pre-release wheels) — which lets every transitive dep resolve to pre-releases →mcp-2.0.0a1(note the neighbours:pydantic-2.14.0a1,lxml-7.0.0a2).mcp.server.fastmcpmodule (verified by inspecting the wheel: zerofastmcpentries).mcp>=1.0treats the alpha as already satisfied (Requirement already satisfied: mcp>=1.0 ... (2.0.0a1)), so the e2e deps install keeps it.e2e_test/infra/__init__.pyeagerly imports the FastMCP-based mock — used only by the sglang-onlyresponses/lane — so evenci_download_model.sh(which just wants model specs) crashes.Passing rc14 runs confirm the contrast: there
mcpcame only from e2e deps and resolved to stablemcp-1.27.2.Fix
mcp>=1.2,<2— the honest requirement (mcp.server.fastmcpwas added in 1.2, removed in 2.0). Per PEP 440,<2excludes pre-releases of 2.0 (verified withpackaging:SpecifierSet(">=1.2,<2").contains("2.0.0a1", prereleases=True) == False), sopip install e2e_test/now replaces the alpha with stable 1.x. TensorRT-LLM's ownmcprequirement is unconstrained, so the downgrade leaves the environment consistent.__getattr__ininfra/__init__.py— model download and chat/router lanes no longer depend on themcpSDK at all. Theresponses/consumers (from infra import MockMcpServer,from infra.mock_mcp import mock_mcp_server) are unaffected.Verification
mcp/server/fastmcpcontent; specifier semantics verified withpackaging.e2e-1gpu-chat (trtllm)lane is the live regression test — it exercises the exact failing path (engine setup → e2e deps → model download → tests).Summary by CodeRabbit