Skip to content

Fix/test plugin registration stub - #21

Merged
heavyrain-lzy merged 2 commits into
verl-project:mainfrom
kahlun:fix/test-plugin-registration-stub
Sep 22, 2026
Merged

heavyrain-lzy merged 2 commits into
verl-project:mainfrom
kahlun:fix/test-plugin-registration-stub

Conversation

@kahlun

@kahlun kahlun commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a pre-existing bug in the shared, session-scoped _stub_training_engine_runtimes
fixture in tests/test_plugin_registration.py. This fixture backs the registration
tests for every vendor in this repo (TPU, MLU, MetaX, Iluvatar, FlagOS, XPU, ...),
not just XPU — it was already broken for all of them against current verl-core, it
just hadn't been exercised yet (see Motivation). Fix the issue #20

Motivation

While retesting the XPU plugin against a newer verl-core checkout, pytest tests/test_plugin_registration.py failed at fixture setup with:

ImportError: cannot import name 'FSDPTurboEngineWithLMHead' from 'verl.workers.engine.fsdp' (unknown location)
../verl/verl/workers/engine/__init__.py:17: ImportError

Root cause: verl-core's verl/workers/engine/__init__.py does
from .fsdp import FSDPEngine, FSDPEngineWithLMHead, FSDPTurboEngineWithLMHead.
This fixture replaces verl.workers.engine.fsdp in sys.modules with a bare
stand-in module so the full registration suite can run without a real FSDP/Megatron
runtime — but the stand-in predates FSDPTurboEngineWithLMHead, which verl-core
added upstream on 2026-08-17 (d4701e4e, "add fsdpturbo backend engine support"),
and was never updated to include it.

This is not XPU-specific and not something introduced by this repo's own history:
this repo's CI has not hit it yet only because pyproject.toml pins verl>=0.7.0,
and the latest PyPI release stayed at 0.9.0 (2026-08-14, predates the new class)
until 0.9.1 shipped on 2026-09-20. Any CI run against 0.9.1 or a current
verl-core main will hit this same error regardless of which vendor's test runs
first — confirmed by reproducing it with an unmodified verl-core checkout and the
unmodified fixture on main.

A second, related bug surfaced once the missing symbol above was patched: the
fixture built its _StubEngine class and only assigned it to the mocked module
attributes after entering mock.patch.dict(sys.modules, ...). In that window,
verl-core's own internal mindspeed wiring re-imports verl.workers.engine, which
runs a live @EngineRegistry.register(...) decorator that asserts
issubclass(engine_class, BaseEngine) at class-definition time. It observed the
pre-assignment placeholder instead of _StubEngine and failed with
AssertionError at verl/workers/engine/base.py:376.

Changes

  • tests/test_plugin_registration.py_stub_training_engine_runtimes fixture:
    • Add the missing FSDPTurboEngineWithLMHead stub to the mocked
      verl.workers.engine.fsdp module, matching the real module's __all__.
    • Reorder setup so BaseEngine is imported for real and _StubEngine is defined
      and assigned to every mocked module attribute before mock.patch.dict(sys.modules, ...)
      takes effect, instead of after. Removes the placeholder/real-stub split that let
      verl-core's own internal imports observe a non-BaseEngine object mid-setup.

No new test cases are added and no XPU-specific code is touched — this is scoped
entirely to the shared fixture (1 file changed, 28 insertions, 22 deletions).

Testing

  • pytest tests/ -v passes — 62 passed, 6 warnings on real B60 hardware.
    Before this fix: 1 error (ImportError above). After stubbing the missing
    symbol alone (partial fix): 1 error (AssertionError above). Both resolved
    together by this PR.
  • Manually verified on target hardware (if applicable) — N/A, test-infra-only
    change; no engine or runtime behavior is touched.

Acceptance Baseline (for new hardware adaptation PRs)

N/A — this PR does not add or modify a hardware platform/engine. It fixes a bug in
shared test infrastructure used by every vendor in this repo.

Checklist

  • Code follows the project's style and passes pre-commit checks
  • [] Documentation updated (if applicable) — N/A, no user-facing behavior change
  • No secrets or credentials included

…test fixture

_stub_training_engine_runtimes (session-scoped, autouse) stubs
verl.workers.engine.fsdp in sys.modules so registration tests don't need
real GPU training runtimes. It only defined FSDPEngine and
FSDPEngineWithLMHead, but verl-core's fsdp/__init__.py now also exports
FSDPTurboEngineWithLMHead -- every vendor's test that imports through
this stub (confirmed: XPU, MLU) fails with ImportError, not just XPU's.

Not device-specific -- this stub is shared across all vendor tests.
_ImportPlaceholder was assigned first, sys.modules got mocked with it
still in place, THEN _StubEngine got defined and re-assigned -- but the
BaseEngine import in between (from verl.workers.engine.base import
BaseEngine) is what actually triggers verl/workers/engine/__init__.py to
run, including its mindspeed import (MindspeedEngineWithLMHead subclasses
MegatronEngineWithLMHead under a live @EngineRegistry.register(...)
decorator that asserts issubclass(engine_class, BaseEngine) at
class-definition time). That assertion saw the still-unpromoted
_ImportPlaceholder and failed for real -- confirmed via AssertionError at
verl/workers/engine/base.py:376 on real B60 hardware.

Fix: import BaseEngine and populate every stub module with _StubEngine
before patching sys.modules at all, so nothing ever observes a bare
placeholder.
@kahlun
kahlun marked this pull request as ready for review September 22, 2026 06:21
kahlun added a commit to kahlun/verl-hardware-plugin-fork that referenced this pull request Sep 22, 2026
…tform-unit-tests

PR #4 depends on the fixture fix from PR #3/verl-project#21 to pass in isolation --
without it, verl>=0.9.1 breaks at fixture setup with ImportError before
ever reaching the new XPU tests this branch adds.
@heavyrain-lzy
heavyrain-lzy requested a lite review from Copilot September 22, 2026 06:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Move the BaseEngine import under the environment patch to preserve runtime isolation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

This PR updates the shared plugin-registration fixture for newer verl-core versions.

Changes:

  • Adds the missing FSDPTurboEngineWithLMHead stub.
  • Initializes stub classes before patching runtime modules.
File Summary
tests/​test_plugin_registration.py Updates engine stubs, but BaseEngine is imported before the environment patch, potentially loading hardware plugins and defeating fixture isolation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

# cleanly on its own (its try/except ImportError guards handle the
# genuinely-missing torchtitan/veomni/automodel/mindspeed/megatron
# packages already).
from verl.workers.engine.base import BaseEngine
@heavyrain-lzy
heavyrain-lzy merged commit 15ee50b into verl-project:main Sep 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants