test(sft): capture warnings before prefetch - #358
Merged
Conversation
# ✅ Tests - Move shuffle after the warning monkeypatch and inside try/finally so background prefetch cannot emit the missing-media warning before capture. - Preserve the existing inline, async and prefetch assertions; leave production behavior unchanged.
ShigureNyako
requested review from
NINGBENZHE and
Yangruipis
as code owners
September 22, 2026 17:11
Nyanpasu 审查看板审查状态: ✅ 已通过 审查版本: 已完成审查,未发现需修改的问题;警告捕获已先于预取启动,三种模式及断言保留。本地测试受环境限制未完成;Pre-commit CI 已通过,其余 CI 尚在运行。 没有未解决的审查问题。
Powered by Nyanpasu with gpt-6-astra medium, please check the suggestions carefully.
|
rai-studio-bot
approved these changes
Sep 22, 2026
rai-studio-bot
left a comment
There was a problem hiding this comment.
已审查 87f4ce9b,未发现需要修改的问题。捕获回调现在先于预取线程启动,原有三种模式与断言保持不变,且启动过程纳入了 finally 清理。
本地测试未完成:默认环境缺少 PyTorch,另一现有环境启动 pytest 时退出码为 139。Pre-commit CI 已通过,其余 CI 尚在运行;本次未运行 GPU/多节点集成测试(无对应测试集群)。
Powered by Nyanpasu with gpt-6-astra medium, please check the suggestions carefully.
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.
What
Fix the warning-capture race in
test_streaming_dataset_missing_media_skip_refills_batch. Onlytests/engine/sft/dataset/test_streaming.pychanges: move one call and explain its ordering.Why
Follow-up requested in #273. Verified against current
main(c7d2847378cdf24c99b22aadb4b2fd078ff225a0):shuffle()callsPrefetchBuffer.set_index_order(), which starts background processing immediately. The missing-image warning can therefore be emitted before the test installs its capture callback.How
Move
ds.shuffle(0)after thelogger.warningmonkeypatch, inside the existingtry/finally. This also keeps prefetch startup covered byds.stop()cleanup. Preserve all existing inline/async/prefetch cases and assertions. No production changes, sleeps, retries, or new dependencies.Testing
Local macOS arm64, Python 3.10.21:
pre-commit run --all-files --show-diff-on-failure— all hooks passed using Python 3.10.python -m pytest tests/engine/sft/dataset/test_streaming.py -q— 57 passed.python -m pytest tests/engine/sft/dataset/ tests/utils/data/test_streaming_dataset.py -q -rs— 142 passed, 1 skipped (optional Parquet test:pyarrowunavailable).python -m pytest tests/ -x -q --tb=shortstopped during unrelated collection becausefastapiis unavailable (23 skipped, 1 error). GPU/multi-node integration validation was not run: this host has no required GPU cluster.Type of Change
Screenshots / Logs
A temporary local-only pytest probe wrapped the real
shuffle()and joined its real prefetch thread before returning (10-second timeout; verified both rows were cached). With the same[prefetch]case and scheduling constraint, the original test failed 5/5 runs with empty captured warnings; the fixed test passed 5/5. All three modes also passed in five probe runs. This verifies the ordering mechanism, not the natural flake rate; the probe is not included in the patch.No visual change; screenshots are not applicable.
@SigureMo Please review when convenient.