Fix: honor disabled xdist in L2 dispatcher

[doraemonmj]

Summary

• determine L2 xdist availability from pytest's active plugin state instead of package importability
• keep inherited -p no:xdist authoritative and run L2 children serially
• document the behavior and cover disabled and active command construction

Testing

• python -m pytest tests/ut/py/test_l2_dispatch.py -q
• issue reproduction on a2a3sim with two runtimes, -p no:xdist, and --max-parallel 2
• control run on a2a3sim with xdist enabled and two workers

Fixes #1993

[coderabbitai]

Warning

Review limit reached

Next included review available in 28 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b715d513-bff7-46cb-8a21-a01b3e362fd8

📥 Commits

Reviewing files that changed from the base of the PR and between fb658d5 and 64e686f.

📒 Files selected for processing (3)

• conftest.py
• docs/testing.md
• tests/ut/py/test_l2_dispatch.py

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ChaoZheng109]

Review

Diffed fb658d55...64e686f2 (merge-base against upstream/main). The diagnosis and the fix are both correct: swapping importability (import xdist) for plugin activation (cfg.pluginmanager.hasplugin("xdist")) is exactly the right distinction, since those two are only equivalent when nobody passes -p no:xdist. Docs land in the same commit, and the new comment states a present-tense fact rather than narrating the change. No must-fix findings.

Below is what I verified plus four smaller things worth a look before merge.

Verified: the regression test genuinely fails pre-fix

I replayed the new test's scenario against the unpatched conftest.py on main, driving the real _dispatch_test_phases with only subprocess.run mocked:

>>> child commands built by the current (unpatched) conftest:
    ... -p no:xdist --max-parallel 2 --runtime host_build_graph        --level 2 -n 2 --dist loadfile
    ... -p no:xdist --max-parallel 2 --runtime tensormap_and_ringbuffer --level 2 -n 2 --dist loadfile

>>> self-contradictory commands: 2/2

So this is a real regression barrier, not a test written to pass. I also confirmed the plugin-state semantics on pytest 9.0.1 / pluggy 1.6.0:

invocation	hasplugin("xdist")	is_blocked("xdist")
default	True	False
-p no:xdist	False	True
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1	False	False
-n 0	True	False

Undersold in the PR body: this fixes two more configurations

The new check also repairs cases the old import xdist probe could never see, and which would have produced the same broken child command:

• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 — xdist installed but not loaded, and the env var is inherited by the child.
• -p no:xdist placed in addopts (pyproject.toml / pytest.ini) rather than on the command line — invocation_params.args does not include addopts, so the old probe was blind to it while the child still honored it.

Worth a line in the PR body; the fix is broader than #1993 describes.

Should fix: the warning branch has no coverage, and it is reachable

max_parallel > 1 and not xdist_active and not is_blocked("xdist") is the only branch in the new code with a side effect (the printed warning), and neither test reaches it — the disabled test sets xdist_blocked=True, the active test takes the other side.

The obvious justification for skipping it ("xdist is never missing") does not hold, but not for the reason it first appears: pyproject.toml lists pytest-xdist>=3.0 in the [test] extra, so uninstalled really is close to unreachable — yet PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 lands on exactly this branch (see table above) and is a configuration people do use. A third case with xdist_active=False, xdist_blocked=False asserting, via capsys, that the warning still prints and the command still omits -n would lock in the one behavior the fix deliberately preserves.

Should fix: leftover -j reference in the function being edited

The PR correctly drops the stale -j from the old warning string ("-j > 1 but pytest-xdist not installed"), since conftest.py:168 states outright that "pytest reserves lowercase short options for itself, so no -j short is registered". One sibling survives two lines upstream of the edit, in the helper _dispatch_test_phases calls:

# conftest.py:904
"""Parse the -j/--max-parallel CLI value; 'auto' → platform-aware default."""

Cheap to fold in. (simpler_setup/scene_test.py:2090,2097,2100 carry the same ghost against an argparse parser that also registers only --max-parallel — broader, and clearly out of scope here.)

Consider: -n 0 is the other way to disable xdist, and it is not honored

-p no:xdist is not the only idiomatic disable; -n 0 is xdist's own. Under -n 0 the plugin is active, so hasplugin returns True and the dispatcher still appends -n <max_parallel> --dist loadfile. Argparse last-wins, so the user's explicit "zero workers" is silently overridden.

This does not crash the way -p no:xdist does, so it is materially less severe — but the PR title says "honor disabled xdist", and this is a disable the fix does not honor. Handling it or explicitly scoping it out in the PR body / docs would both be fine; silently leaving it is the only option I would push back on.

Consider: -n/--dist already present at top level

Related and pre-existing, noting it only so it is on the record: if the top-level invocation carries its own -n/--dist, the dispatcher appends a second pair and last-wins quietly discards the user's values. Not introduced here and not this PR's job.

Checked, no action needed

• _l2_poison_retry is unaffected — it never appends -n/--dist (conftest.py:1549), so it was already serial and stays consistent with the L2 path after the change.
• No doc drift elsewhere — docs/ci.md:352 mentions -p no:xdist, but only to describe per-process DFX smoke isolation; still accurate. The new docs/testing.md paragraph is correct that Resource-phase concurrency is unaffected, since it uses the _ps.run_jobs subprocess pool rather than xdist.
• The retained # noqa: PLR0912 is harmless — RUF is not in the ruff select list, so RUF100 (unused-noqa) will not fire even though the branch count dropped.
• Test style matches the tree — the _load_root_conftest / _ROOT = parents[3] shape is identical to tests/ut/py/test_resource_failure_summary.py.

Blast radius: not reachable from any current CI invocation

For anyone weighing merge urgency — I walked every -p no:xdist call site:

call site	multi-runtime?	can it hit the bug?
_st-sim-a2a3.yml / _st-sim-a5.yml DFX smokes	no — each targets one runtime directory	no (direct-run fast path)
_profiling-flags-smoke.yml	no	no
run-onboard-dfx-smokes/action.yml	no, and --device "$device" is a single card → max_parallel == 1	no (two independent reasons)

So this is a latent defect only reachable from a hand-written multi-runtime invocation. Merge risk is correspondingly low — the change cannot alter any CI job's behavior — but so is urgency.

Verdict

Approve-with-suggestions from my side (leaving the formal vote to a maintainer). The two Should fix items are small and independent of the core change; -n 0 only needs a decision, not necessarily code.