[https://nvbugs/6529626][fix] Pin mcp<2.0.0 and unwaive the scaffolding tests - #16980
Conversation
|
/bot run |
WalkthroughThe PR constrains ChangesMCP dependency constraint
Scaffolding test waiver removal
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #62371 [ run ] triggered by Bot. Commit: |
2ac3f5f to
963c128
Compare
|
/bot run |
|
PR_Github #62378 [ run ] triggered by Bot. Commit: |
|
PR_Github #62371 [ run ] completed with state |
JunyiXu-nv
left a comment
There was a problem hiding this comment.
Please also unwaive the scaffolding tests.
…ng tests mcp is unpinned, and mcp 2.0.0 (published 2026-07-28 13:45 UTC) removed mcp.server.fastmcp: the package ships 19 files under mcp/server/fastmcp/ in 1.29.0 and none in 2.0.0. Any image built after that resolves the bare requirement to 2.0.0, so the module-level import in tests/unittest/scaffolding/test_mcp_worker.py raises while pytest is collecting it. A collection error is not a test failure. pytest reports "Interrupted: 1 error during collection" and exits 2 before running anything, so the other seven files in tests/unittest/scaffolding never execute and no JUnit report is written at all -- which is why this surfaced only as the outer wrapper's "assert passed, failure reported in unittests", with no inner test named anywhere in the archived stage results. Nor could it be waived at test granularity: apply_waives and apply_waives_ut both run inside pytest_collection_modifyitems and mark collected items, and a module that fails to import produces none. NVIDIA#16978 therefore waived the whole unittest/scaffolding directory as an interim unblock. Pin rather than skip the test. tensorrt_llm/scaffolding/worker.py and tensorrt_llm/scaffolding/contrib/mcp/mcp_utils.py both ship against the 1.x API, and test_mcp_worker.py is the only CI coverage that exercises MCPWorker against the installed mcp. Skipping it would drop that coverage precisely when the dependency took a major version bump, hiding a library-level risk rather than resolving it. With the pin in place the collection error is gone, so NVIDIA#16978's waive is removed in the same change: leaving it would keep all eight scaffolding files skipped indefinitely for a cause that no longer exists. Lifting the pin means migrating the library off the 1.x APIs first, not just the test. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
963c128 to
8ff4b7f
Compare
|
/bot run |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/test_lists/waives.txt`:
- Line 187: Add the required NVIDIA copyright header at the top of the modified
waives.txt file and set its copyright year to the current year, leaving the
existing test entry for test_auto_dtype unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b2a9b0f-7437-4725-8ed8-56e6ac54eaf7
📒 Files selected for processing (2)
requirements.txttests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
|
PR_Github #62404 [ run ] triggered by Bot. Commit: |
|
PR_Github #62378 [ run ] completed with state |
|
PR_Github #62404 [ run ] completed with state
|
BowenFu
left a comment
There was a problem hiding this comment.
Verified the pin can't regress anything already working: nothing in requirements/setup/docker/constraints or any pyproject requires mcp 2.x (security_scanning already caps >=1.28.1,<2.0.0), and the client APIs scaffolding actually uses — ClientSession, sse_client, initialize, list_tools, call_tool — are all present in 1.29.0. Waiver line matches exactly once on main, removed in the same commit as the cause.
Not blocking on it, but worth knowing: the run on this head was aborted by fail-fast before unittest/scaffolding actually executed, so this PR hasn't yet demonstrated the green scaffolding run your description promises — the two failures it did report (modeling_qwen on A30, test_nvfp4 on B200) are unrelated, and modeling_qwen also failed on #16921's build 50499.
Pipeline #50565 — two unrelated failures, and an honest correctionFirst, a correction to what I said above. I claimed that removing the waive makes this PR's own pipeline a direct test of the pin. It did not test it. #50565 completed 40 stages and Also worth noting for anyone reading the bot comment: it says The two actual failures: Ambient rates, from builds that are not this PR's. My scan window covers the last 50 pipelines, which has since moved past #50565 — so the counts below contain no data from this PR, making them independent sightings rather than self-reports:
One methodological caveat I should state rather than let the table imply more than it supports: Why the change is very unlikely to be the cause: the diff is one dependency pin and one waive line. Status of the waive, measured: across the last 50 pipelines the scaffolding wrapper appears 17 times, and in every case it is Re-running, with the hope of drawing a stage that schedules |
|
/bot run |
|
PR_Github #62509 [ run ] triggered by Bot. Commit: |
|
PR_Github #62509 [ run ] completed with state |
Fixes https://nvbugs/6529626
What broke
mcpwas unpinned atrequirements.txt:80. mcp 2.0.0, published 2026-07-28 13:45 UTC, removedmcp.server.fastmcp:mcp/server/fastmcp/tests/unittest/scaffolding/test_mcp_worker.pyimports it at module level, so any image built after that timestamp fails while pytest is collecting the file.Why one broken import takes out an entire stage
A collection error is not a test failure. pytest reports
Interrupted: 1 error during collectionand exits 2 before running anything:tests/unittest/scaffoldingnever execute;--periodic-junit-xmlpathis honoured.That is why this surfaces only as the outer wrapper's
assert passed, "failure reported in unittests", with no inner test named anywhere in the archived stage results — the failing import is visible only in the stage's rawstdout.log.It also cannot be waived.
apply_waives(tests/integration/defs/test_list_parser.py) andapply_waives_ut(tests/unittest/conftest.py) both run insidepytest_collection_modifyitemsand add skip markers to collected items; a module that fails to import produces none. The only waive that works is the directory-level one against thetest_unittests_v2[unittest/scaffolding]wrapper (#16978), which suppresses all eight files to silence one.Why pin rather than skip the test
tensorrt_llm/scaffolding/worker.pyandtensorrt_llm/scaffolding/contrib/mcp/mcp_utils.pyboth ship against the 1.x API, andtest_mcp_worker.pyis the only CI coverage that exercisesMCPWorkeragainst the installedmcp. Guarding the test withimportorskipwould have dropped that coverage precisely when the dependency took a major version bump — hiding a library-level risk instead of resolving it.Note the library's own imports (
from mcp import ClientSession,from mcp.client.sse import sse_client) do still resolve under 2.0.0, so this is not an immediate import break intensorrt_llm/; the point is that the 2.x behaviour is unverified, and the pin keeps the shipped code and its coverage on the version they were written against.Lifting the pin means migrating the library off the 1.x APIs first, not just the test.
Unwaiving
#16978 waived the whole
unittest/scaffoldingdirectory as an interim unblock, because the collection error could not be waived at test granularity. With the pin in place that error is gone, so this change removes the waive in the same commit — leaving it would keep all eight scaffolding files skipped indefinitely for a cause that no longer exists.-full:DGX_H100/unittest/scaffolding SKIP (https://nvbugs/6529626)This also means the run on this PR is a direct test of the fix: if the pin did not work,
unittest/scaffoldingwill fail here rather than being silently skipped.Scope
unittest/scaffoldingruns on H100 only —l0_h100.yml:116is the sole test-db entry, in a block gated onorchestrator: mpi,stage: pre_merge.mcp<2.0.0resolves to 1.29.0, which shipsmcp/server/fastmcp/intact. This is the onlymcpdeclaration in the repo.Dev Engineer Review
mcpinrequirements.txttomcp<2.0.0(with an inline note thatmcp==2.0.0removedmcp.server.fastmcp, while the scaffolding code still relies on the 1.x API).fastmcp”) is still not addressed, but this unblocks existing MCP-based scaffolding tests.QA Engineer Review
tests/integration/test_lists/waives.txt:full:DGX_H100/unittest/scaffolding(SKIP reference tonvbugs/6529626).