fix(training,calibration): resolve DataLoader workers everywhere, and guard it (#207) - #209
Merged
Merged
Conversation
#207) #206 fixed `eval test` by resolving its DataLoader workers, and added `resolve_dataloader_workers` whose docstring says "Every caller that builds a loader goes through this function, so that guard lives in one place". The validation loader inside the training loop did not, and was hardcoded to 0 three lines below the call that resolves the train loader. The cost is once per epoch. On a 1,176,763-chunk binary val set the GPU sat at ~30% for ~5 minutes at every epoch boundary and then recovered -- ~75 minutes of near-idle accelerator across a 15-epoch run, and it scales with val size. That is the same shape as #205: `__getitem__` being cheap does not mean one process can saturate a GPU, because collate, pin, host-to-device and the forward pass still serialize onto that core. The memory half of the old comment is real, but narrower than a blanket 0. `LeechDataset` stacks per-chunk tensors into contiguous buffers *precisely* so a fork COW-shares them (see the note at dataset.py `_try_stack`); only the list fallback, taken when per-chunk shapes are inconsistent, makes each worker fault N PyObject headers into private copies and multiply peak RSS. So the exception is now scoped to exactly that case instead of penalising every run to protect it -- and it wins even over an explicit `--num-workers N`, because OOM is not a throughput tradeoff. The logic lives in `resolve_val_dataloader_workers` beside its sibling rather than inline in `train()`, which keeps the "one place" claim true and makes it testable without standing up a training loop. Six tests, and both halves are mutation-checked: removing the list-fallback guard fails two of them, and reverting to the old always-zero fails three. Other call sites that still bypass the resolver, not touched here: `calibration.py` 193/543 (runs on CUDA, most likely to matter next), `commands/benchmark.py` 63, and the legacy `SignalCNN` path at training.py 261/265. `gridsearch.py` is fine -- its pool workers are daemonic, where the resolver returns 0 anyway.
…le class Both `calibration.py` loaders passed `num_workers=num_workers` with a default of 0, so on CUDA they got literally zero rather than AUTO -- the same starvation as #205 and #207, in the third place. Both now go through `resolve_val_dataloader_workers`: they feed validation datasets, so the list-fallback exception applies to them as well. The more useful half is the guard. `num_workers` may no longer be a bare literal anywhere in the package; it must come from a resolver, from a local whose name says it carries a resolved count, or carry a call-site marker `dataloader-workers: unresolved` with a reason. Two such markers exist: `commands/benchmark.py` (the worker count is the independent variable being benchmarked) and the legacy SignalCNN path in `training.py` (SignalDataset is not a LeechDataset and has no `_signals_tensor`, so the val guard would force 0 and change behaviour -- converting it needs its own measurement). The guard took three attempts, and the first two are why it is written this way: 1. A file-scoped allow-list. Worse than nothing: `("training.py", None)` exempted the whole file, so reintroducing the #207 bug PASSED. 2. Inspecting `DataLoader(...)` call sites. Also passed the #207 reintroduction, because that bug lives in a `val_loader_kwargs` dict that reaches the loader via `**kwargs`, and the enclosing function resolves a DIFFERENT loader -- which is precisely the shape #207 had. So the check is on the VALUE wherever it is bound, not on loader construction. Mutation-tested against both real regressions: reintroducing the #207 literal fails it naming training.py, and reintroducing the calibration.py literal fails it naming calibration.py.
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.
Fixes #207.
#206 fixed
eval testand addedresolve_dataloader_workers, whose docstring says:Nothing enforced that. Two callers didn't.
What was broken
training.pyval loadernum_workers=0, three lines below the call that resolves the train loadercalibration.py×2num_workers=num_workerswith default0— on CUDA that is literally zero, not AUTOObserved on an A30 with a 1,176,763-chunk val set:
≈ 75 minutes of near-idle GPU per 15-epoch run, scaling with val size. Same root cause as #205:
__getitem__being cheap does not mean one process can saturate a GPU — collate, pin, host-to-device and the forward pass all serialize onto that core.Why not just delete the zeros
The old val comment gave two reasons and they aged differently:
__getitem__is trivially fast, so workers add no benefit" — refuted by the measurement.LeechDatasetstacks per-chunk tensors into contiguous buffers precisely so a fork COW-shares them (the_try_stacknote indataset.py). Only the list fallback — inconsistent per-chunk shapes — makes each worker fault N PyObject headers into private copies and multiply peak RSS by(1 + workers).So
resolve_val_dataloader_workersscopes the exception to exactly that case, and it wins over an explicit--num-workers N, because OOM is not a throughput tradeoff. It lives beside its sibling indataset.pyrather than inline intrain(), which restores the "one place" claim and makes it testable without standing up a training loop.The guard, and why it looks like this
num_workersmay no longer be a bare literal anywhere in the package. It must come from a resolver, from a local whose name says it carries a resolved count, or carry a call-site markerdataloader-workers: unresolvedwith a reason. Two markers exist:commands/benchmark.py(the worker count is the independent variable being benchmarked) and the legacySignalCNNpath (SignalDatasethas no_signals_tensor, so the val guard would force 0 and change behaviour).It took three attempts, and the first two failed their own mutation test:
("training.py", None)exempted the whole file, so reintroducing the The in-training validation loader bypasses resolve_dataloader_workers, starving the GPU once per epoch #207 bug passed.DataLoader(...)call sites — also passed the The in-training validation loader bypasses resolve_dataloader_workers, starving the GPU once per epoch #207 reintroduction, because that bug lives in aval_loader_kwargsdict reaching the loader via**kwargs, and the enclosing function resolves a different loader. That is exactly the shape The in-training validation loader bypasses resolve_dataloader_workers, starving the GPU once per epoch #207 had.So the check is on the value wherever it is bound, not on loader construction.
Tests
TestValLoaderWorkers(6) covers the resolver: stacked datasets get workers, val matches train, the list fallback stays serial, CPU stays serial, an explicit N is honoured when stacked and overridden when not.The guard is mutation-tested against both real regressions:
training.pytraining.py:1614calibration.pyliteralcalibration.py:202Full suite green;
ruff formatandcheckclean.