fix(tests): ✅ Stop asserting joblib's dispatch timing - #174
Merged
Conversation
`test_repeats_span_multiple_processes` asserted the four repeats landed on more
than one worker PID. That is joblib's dispatch timing, not a promise this code
makes: the runs are short enough that one worker can take all four before the
others have finished starting, which is what happened on the macOS 3.11 runner:
AssertionError: all repeats shared one process: {2078}
PID 2078 was not the parent, so the parallel branch had worked exactly as
intended -- the assertion was simply testing something the implementation never
guaranteed.
What actually separates the two branches is the parent process: the sequential
branch runs there and loky never does. The test now asserts the repeats did not
run in the parent, and is renamed for what it checks.
This does not weaken the regression. Restoring the #169 bug still fails it:
AssertionError: repeats ran in the parent, so the parallel branch was
skipped: {18952}
Ran 10 consecutive times against the fix with no flake; full suite 707 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修 #173 上
Tests 3.11 on macos-latest的失败。是测试写错了,不是代码有问题。失败
原因
断言写的是 4 次重复要落在多于一个 worker PID 上:
但 PID
2078不是父进程 —— 前一条断言pids != {os.getpid()}是过了的。也就是说并行分支完全正常工作、loky worker 确实起来了,只是 joblib 把 4 个任务都派给了同一个 worker。joblib 按 worker 就绪情况派发任务,而这些测试任务极短,第一个 worker 完全可能在其他 worker 启动完成之前就把 4 个都做完。这是调度时序,不是这段代码承诺的行为,我不该断言它。
改法
真正区分两个分支的是父进程:串行分支在父进程里跑,loky 永远不在。所以改成断言重复没有在父进程里执行,测试名也改成它实际检查的内容(
test_repeats_runs_in_worker_processes)。没有削弱回归能力
把 #169 的 bug 放回去,测试立刻挂:
修复在位时连跑 10 次全绿,全量套件
707 passed,ruff / format 通过。与其他 PR 的关系
合进 dev 后 #173 会自动带上,
Tests 3.11 on macos-latest应当转绿。Tests 3.13 on windows-latest仍会红 —— 那是 dev 上七个月的老问题(#171),与此无关。