Repository navigation
[OMNIML-6228] Fix two flaky tests: MTP causality tolerance and example-runner port collisions - #2900
Conversation
test_mtp_later_attention_blocks_remain_causal compares the positions that must not change with rtol=1e-6 and atol=1e-9, tighter than float noise, on unseeded random inputs. Over 400 RNG states the earlier positions differ by at most 3e-8, so tiny_checkpoint1 fails for 1.2% of the states (5 of 400; seed 32 reproduces it: 2.4e-9 against 1e-9 allowed). A serial run sees the same RNG state every time, but under pytest-xdist it varies, and the test failed on 3 of about 22 unit runs of a PR stack that runs the unit tests in parallel. Use rtol=1e-5 and atol=1e-6: no failure in 400 states for either checkpoint, 30x above the noise and about 10^5x below what a real leak does (the shifted last position changes the earlier ones by about 0.1). Signed-off-by: Shengliang Xu <shengliangx@nvidia.com> (cherry picked from commit bf7ae348cd6c1048fc4e9f67667d0a63cd22c1af)
test_distill_puzzletron_anymodel failed twice in two weeks (PR 2819 on 10-15, which passed on the rerun, and PR 2860 on 10-18) with "DistNetworkError: The server socket has failed to listen ... name: EADDRINUSE" from Megatron-Bridge's temporary_distributed_context, when the HF export step binds the rendezvous port again. Whoever held the port is gone by the next attempt, and the in-process runner picks a new port when the process group is not initialized. run_example_command already reruns a step once on a transient HuggingFace error. Treat a port collision the same way, and leave everything else (a real failure) failing on the first attempt. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies both main changes: MTP causality tolerance and example-runner port-collision retries. It is specific and related to the pull request objectives. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Security Anti-Patterns PASS. The PR changes only test files and a test helper under tests/. The diff adds transient-error string matching, retry logic, a test, and relaxed numeric tolerances. It adds no `torch.load(..., w…Full details: Security Anti-Patterns
-
Explanation
PASS. The PR changes only test files and a test helper under
tests/. The diff adds transient-error string matching, retry logic, a test, and relaxed numeric tolerances. It adds notorch.load(..., weights_only=False),numpy.load(..., allow_pickle=True), hardcodedtrust_remote_code=True,eval(),exec(),# nosec, or dependency changes.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2900 +/- ##
==========================================
+ Coverage 76.90% 78.79% +1.88%
==========================================
Files 658 659 +1
Lines 74029 74155 +126
==========================================
+ Hits 56932 58429 +1497
+ Misses 17097 15726 -1371
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
LGTM: the retry change is small and tested; the justified MTP tolerance relaxation needs human sign-off.
Needs action:
- Confirm the tolerance update in
tests/unit/torch/models/test_nemotron_h_mtp.py: the documented float-noise margin justifies relaxing precision while retaining the causality assertion.
No action needed:
- Retry coverage exercises transient HF errors, port collisions, and immediate propagation of non-transient failures.
- Reuses the existing bounded retry loop and existing per-attempt port selection; no new retry subsystem is introduced.
- Tests were inspected, not executed.
|
3 participants
What does this PR do?
Type of change: Bug fix (tests)
Two flaky CI failures from the last two weeks:
test_mtp_later_attention_blocks_remain_causal(tests/unit/torch/models/test_nemotron_h_mtp.py) compared the positions that must not change withrtol=1e-6, atol=1e-9, tighter than float noise, on unseeded random inputs. Over 400 RNG states per checkpoint those positions differ by at most 3e-8, andtiny_checkpoint1fails 5 of 400 states at the old tolerance (for example 2.4e-9 against the 1e-9 allowed). A serial run sees the same RNG state every time, so serial CI never showed it; under pytest-xdist (the [OMNIML-6178] [1/11] Run the unit tests in parallel with pytest-xdist #2845 stack) the state varies and the test failed on 3 of about 22 unit runs. The new tolerance,rtol=1e-5, atol=1e-6, has no failure in 400 states for either checkpoint, sits 30x above the noise and about 10^5x below what a real leak does (shifting the last position changes the earlier ones by about 0.1).test_distill_puzzletron_anymodel(tests/examples/megatron_bridge/test_distill.py) failed twice (PR runs on 10-15, which passed on the re-run, and 10-18) withDistNetworkError: The server socket has failed to listen ... name: EADDRINUSE, raised inside Megatron-Bridge'stemporary_distributed_contextwhen the HF export step binds the rendezvous port again. I could not reproduce it on CPU (destroying a process group and re-initializing it on the same port worked 300 times out of 300), so the cause of the port being taken is unknown.run_example_commandalready reruns a step once on a transient HuggingFace error; this PR adds the port collision to the transient markers (EADDRINUSE,Address already in use). The in-process runner picks a new port when the process group is not initialized, and nothing else changes: a real failure still fails on its first attempt.Usage
N/A
Testing
tests/unit/test_example_run_command.py::test_run_example_command_retries_only_transient_errors(3 cases: an HF 5xx and a port collision are rerun once, an assertion failure is not). Without the two new markers the port-collision case fails.tests/unit/torch/models/test_nemotron_h_mtp.pypasses (72 tests). With every test reseeded from its id and a seed (a small pytest plugin, not part of this PR), the wholetests/unitsuite failed this test in 1 of 53 runs at the old tolerance and in 0 of 90 runs of the test file at the new one.tests/unit(5,119 tests) under 52 seeds, 3 shuffled file orders and with every file alone, and measured how close eachassert_close/allclosecame to failing (about 1,200 assertion sites, 30 seeds). No other test depends on the RNG state or the file order; the only failure in every run istest_qwen_image_pdd_plugin.py::test_mr210_joint_mask_ignores_padded_token_values, which fails the same way on my local CPU host with and without these changes and passes in CI.Before your PR is "Ready for review"
CONTRIBUTING.md: N/ASummary by CodeRabbit
Bug Fixes
Tests