Repository navigation
fix(measure): every measurement leaves the model on the device it came in on - #9
Merged
Merged
Conversation
…e in on
Several entry points moved the caller's model to the measuring device in place
and never moved it back, so a CUDA model came out of a CPU profiling call parked
on the CPU — and every later timing of that model was silently a CPU timing.
Sites fixed (all now go through one helper, core._on_device):
- speed.py:72 _forward_latencies (compute_speed / _multi, sweep_threads,
sweep_batch_sizes, sweep_latency)
- memory.py:53 _gpu_metrics (compute_memory / _multi)
- memory.py:91 _cpu_metrics (compute_memory / _multi)
- profiling.py:181 _profile_layers (LayerProfiler.profile)
- roofline.py:307 RooflineAnalyzer.profile
- energy.py:98 compute_energy (compute_energy_multi)
Checked and left alone: compute_size (no move), compute_compute (moves the
sample to the model's device, not the reverse), benchmark() (delegates, and
already restores the train/eval flag).
_on_device(model, dev) records the device of the first parameter or buffer,
moves the model to dev, and restores it in finally. A model with no tensors has
nothing to restore. No metric value, signature or message changes; train/eval
handling is untouched.
_profile_layers combines both context managers in one `with`. speed, energy and
roofline keep a nested `with` because _ensure_device_supported / measure_peaks
must run between resolving the device and moving the model.
Privacy: this repo is public, so two notebook mentions of a private sibling
package (nbs/analysis/roofline.ipynb `usage`, nbs/tutorials/roofline.ipynb
`callout`) were reworded without naming any package.
Test coverage, plainly: CI covers only the CPU->CPU contract (a CPU model
measured with device="cpu" is still on the CPU afterwards). The actual restore
is proven locally by the `#| notest` cells, which put a model on CUDA, measure
it on the CPU and assert it comes back — all seven are red on origin/master and
green here, including the user-side benchmark() path. Energy has no CI coverage
at all: compute_energy runs codecarbon, so both its assertions are `#| notest`.
Claude-Session: https://claude.ai/code/session_0177tJZDQTTjQdycXV1EZtdJ
CI (fastai/workflows/nbdev3-ci) resolves nbdev 3.3.15; the local dev env is on
3.0.17. The two disagree, so the CI gate ("clean + export must leave the tree
unchanged") would be red on master today, with or without a PR. Two causes:
1. nbdev 3.3.15 / nbformat 5.10.4 write cell `source` as a list of lines, and
drop `execution_count`/`outputs` from markdown cells (invalid there per the
nbformat v4 schema). 12 notebooks were stored in the older single-string
shape. Reserialized here, once.
2. nbs/index.ipynb declared `#| default_exp __init__` with two `#| export`
cells, so 3.3.15's `pkg_mdoc` rewrote fasterbench/__init__.py's docstring
from the index notebook (description + every exporting index cell, fenced
as python, plus a Modules list) on every export. index.ipynb no longer
exports: the `#| default_exp` cell and the bare `__version__` cell are gone
and the imports cell stays as a plain (still executed) cell.
fasterbench/__init__.py is hand-maintained from now on, as in fasterai. The
generation scaffolding is gone — the "AUTOGENERATED / File to edit" and "Docs:"
header lines, the `# %% auto #0` marker with its dead `__all__ = []` (always
overwritten by the real one below), and the two `# %% ../nbs/index.ipynb #...`
provenance markers, which pointed at cells this commit deletes. The docstring
first line, __version__, the imports and the real __all__ are byte-identical:
`import fasterbench` still gives __version__ == "0.1.0" and 32 __all__ entries,
all resolvable.
Content-neutral: for all 12 reserialized notebooks, joined `source` and
`outputs` per cell are identical to the previous commit (scripted check);
the only source edit is the intentional nbs/index.ipynb one above.
Verified with the CI stack itself: two nbdev-clean + nbdev-export passes are
idempotent, nbdev-export leaves __init__.py byte-identical, and
`git status --porcelain -uno` is empty afterwards. No nbs/llms.txt is produced,
so nothing to delete or ignore. Dev-env suite still green, and the seven
`#| notest` device-restore cells still green with flags='notest'.
Claude-Session: https://claude.ai/code/session_0177tJZDQTTjQdycXV1EZtdJ
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.
Summary
Six measurement paths moved the caller's model to the measuring device in place and never moved it back, so a CUDA model came out of a CPU profiling call parked on the CPU, and any later timing of it was silently a CPU timing. Every measurement now leaves the model on the device it came in on.
speed.py:72_forward_latenciescompute_speed,compute_speed_multi,sweep_threads,sweep_batch_sizes,sweep_latencymemory.py:53,:91_gpu_metrics,_cpu_metricscompute_memory,compute_memory_multiprofiling.py:181_profile_layersLayerProfiler.profileroofline.py:307RooflineAnalyzer.profileenergy.py:98compute_energycompute_energy_multiChecked and left alone:
compute_size(reads only),compute_compute(moves the sample to the model's device),benchmark()(delegates, restores the train/eval flag). One private context manager,_on_device(model, dev)infasterbench.core, yields the model ondevand restores the original device in afinally(a model with no parameters or buffers stays put). Each site becomeswith _on_device(model.eval(), dev) as model:and its body is re-indented, nothing else: no metric, signature or message changes. Inspeed,energyandrooflinethe newwithstays nested inside the device context because a check (_ensure_device_supported) or a peak measurement runs between the two and must see the model before it moves.Also in this commit: two prose mentions of a private package in
nbs/analysis/roofline.ipynbandnbs/tutorials/roofline.ipynbare reworded; this library measures, nothing here decides.Red → green
The seven new
#| notestcells (a tiny conv net on CUDA, measured withdevice='cpu', every parameter asserted back on CUDA) run againstorigin/master:and on this branch:
CI is CPU-only, so it covers the CPU→CPU contract only; the CUDA restore, which is the bug, is proven by the
#| notestcells run locally withflags='notest'(all seven notebooksSuccess.).compute_energyhas no CI coverage at all (its test would run a codecarbon tracker), stated rather than hidden. Full default suite, 17 notebooks:Export + clean idempotent,
_modidx.pygains the one new entry,__version__stays 0.1.0.Second commit: the tree in the shape CI's nbdev expects
CI (
fastai/workflows/nbdev3-ci, nbdev 3.3.15) last ran on this repo in July and would be red on master today, independently of this fix:nbdev-cleanreserializes 12 notebooks (single-stringsourceto a list of lines, two markdown cells shedding keys the nbformat schema forbids), andnbdev-exportregenerates the__init__.pydocstring from everyindex.ipynbcell carrying an export directive. The chore commit reserializes the notebooks (content-neutral: joinedsourceandoutputsidentical per cell, checked by script), stopsindex.ipynbfrom exporting__init__(its cells stay as plain executed cells), and makesfasterbench/__init__.pyhand-maintained as in fasterai, byte-identical in its version, imports and__all__. After it, CI's clean + export leavegit status --porcelain -unoempty on two passes, and so does the dev environment's older nbdev.https://claude.ai/code/session_0177tJZDQTTjQdycXV1EZtdJ