Skip to content

[inference] fix: Support MCore dev inference mode - #3889

Closed
yaoyu-33 wants to merge 2 commits into
mainfrom
yuya/mcore-dev-autofix-20260519-pr3883
Closed

[inference] fix: Support MCore dev inference mode#3889
yaoyu-33 wants to merge 2 commits into
mainfrom
yuya/mcore-dev-autofix-20260519-pr3883

Conversation

@yaoyu-33

Copy link
Copy Markdown
Contributor

Original bump PR: #3883
Target: dev
Date: 2026-05-19

Failure classification: MCore broke Bridge

Root cause:

  • The current MCore dev bump no longer exposes InferenceMode from megatron.core.inference.utils, so Bridge VLM inference modules failed import during install checks and unit-test collection.
  • The install check also flagged missing cache_position docstring entries in Qwen3ASR forward methods.

Fix summary:

  • Add a narrow VLM MCore compatibility shim for InferenceMode and route VLM code/tests through it.
  • Document cache_position in the Qwen3ASR forward docstrings.
  • Remove stale MCore compatibility guards now that the current MCore main/dev targets agree on the guarded APIs.

Guards:

  • Added src/megatron/bridge/inference/vlm/_mcore_compat.py with TODO removal condition: remove when Megatron-Core dev exposes InferenceMode from megatron.core.inference.utils.
  • Removed stale guards for mtp_block_spec, Mamba hybrid layer helper/kwarg, Muon muon_nesterov, get_mup_config_overrides, async checkpoint/EnergyMonitor imports, and null tokenizer library names.

Validation:

  • Local syntax: python3 -m py_compile <changed Python files> passed.
  • CW interactive unit validation passed on 2026-05-19: srun -A coreai_dlalgo_llm -p interactive ... bash -lc 'cd $WORKDIR && export PYTHONPATH=$PWD/src:$PWD/3rdparty/Megatron-LM:${PYTHONPATH:-} && export UV_NO_SYNC=1 && uv run python -c "import megatron.bridge.inference.vlm.base; import megatron.bridge.inference.vlm.vlm_engine; import megatron.bridge.models.qwen3_asr.hf_qwen3_asr.modeling_qwen3_asr" && uv run python -m pytest tests/unit_tests/inference/vlm/test_base.py tests/unit_tests/inference/vlm/test_vlm_engine.py tests/unit_tests/models/mamba/test_mamba_provider.py tests/unit_tests/recipes/utils/test_optimizer_utils.py tests/unit_tests/training/test_optim.py tests/unit_tests/training/test_state.py tests/unit_tests/training/test_energy_monitor.py tests/unit_tests/training/test_tokenizer.py -q' -> 112 passed, 35 warnings.
  • CW interactive pre-commit passed on 2026-05-19: uv run pre-commit run --all-files.

Note: local uv run could not start hooks/tests because the workstation uv cache path for mamba-ssm==2.3.2.post1 is not writable; the required validation was completed in the CW interactive container.

dimapihtar and others added 2 commits May 19, 2026 06:31
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Yu Yao <yaoyu.094@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented May 19, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@yaoyu-33

Copy link
Copy Markdown
Contributor Author

/ok to test d389b5e


# TODO: remove this guard when Megatron-Core dev exposes InferenceMode from
# megatron.core.inference.utils.
class InferenceMode:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the fallback InferenceMode shim has no dedicated unit test exercising the fallback path (i.e., when the real InferenceMode import fails). The existing test_vlm_engine.py imports InferenceMode but doesn't verify the shim's is_active() -> False / no-op set_active/unset_active behavior in isolation. Since this is a temporary compat shim with a clear TODO, this is low priority — but a quick parametrized test patching the import to force the fallback would catch regressions if the shim's contract changes.

@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Light Review -- PR 3889 -- Clean MCore dev bump with stale compat guard removal. The changes are straightforward and well-scoped. -- Observations -- Guard removals look correct. _mcore_compat.py shim is narrow and safe. Qwen3ASR docstrings consistent. Test updates match source changes. Minor test coverage gap on the fallback InferenceMode shim (see inline comment). -- Suggested test cases -- No perf tests impacted.

@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Light Review -- PR 3889 (formatted re-post) --- Clean MCore dev bump with stale compat guard removal. The changes are straightforward and well-scoped. --- Observations --- Guard removals look correct: mtp_block_spec, hybrid_override_pattern to hybrid_layer_pattern, muon_nesterov, get_mup_config_overrides, get_async_strategy/EnergyMonitor, and null tokenizer fallbacks are all cleanly removed. The surviving hybrid_context_parallel guard in initialize.py has an updated TODO with clear removal condition. | _mcore_compat.py shim is narrow and safe: fallback InferenceMode returns False for is_active() (safe default) and has a clear TODO for removal. The ImportError message check correctly re-raises if the entire module is missing vs just the name. | Qwen3ASR docstrings: cache_position parameter docs added to both forward methods. Formatting is consistent with the existing HF-style parameter docs. | Test updates match source changes: removed Mamba fallback helper tests, simplified test_finalize_uses_compatible_hybrid_layer_count, and hardcoded muon_nesterov assertions. | Minor test coverage gap: the _mcore_compat.py fallback InferenceMode class (the shim itself) has no dedicated test exercising the fallback path. See inline comment. Low priority given the temporary nature of the shim. --- Suggested test cases: No perf tests impacted.

@yaoyu-33 yaoyu-33 added area:model Model implementations and HF bridge logic bug Something isn't working full-test-suite needs-review PR is ready for code review and waiting on a reviewer labels May 19, 2026
@yaoyu-33 yaoyu-33 closed this May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:model Model implementations and HF bridge logic bug Something isn't working full-test-suite needs-review PR is ready for code review and waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants