fix: TG013 downloads and device restores, and preprocessors are not models - #51
Merged
Conversation
…odels
Finishes the triage: TG013 was the one rule left without a verdict, and
TG002 had two causes still standing.
TG013, 3 false of 10, from two narrow causes.
A download is not a redundant upload. `_device_argument` accepted a
"cpu" literal as a destination, so `pinmem_nonblock.py` was reported for
looping 100 times over `tensor.to("cpu", non_blocking=True)` -- on a
tensor created with `device="cuda"`, in a tutorial whose whole subject is
measuring transfer behaviour. Wrong twice over.
Restoring the device after a deliberate `.cpu()` is required, not
redundant. `fast_neural_style` does `transformer.eval().cpu()`, writes a
checkpoint, then `transformer.to(device).train()`. Hoisting that out
would leave the model on the host for the rest of training.
The other 7 are the mild `device=` advice the rule exists to give, plus
one genuinely hoistable constant in torchtune.
TG002: `from_pretrained` sits in MODEL_WRAPPERS and matched before
anything could object, so `AutoTokenizer.from_pretrained(...)` was a
model and `tokenizer(x["question"])` was a forward pass -- we reported a
missing `no_grad` around tokenisation. Feature extractors and image
processors the same. The exclusion is by class name and runs first; a
guard test asserts `AutoModelForCausalLM.from_pretrained` still counts.
Across the seven repos: errors 13 -> 11, warnings 49 -> 46, five removed,
no new findings.
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.
Finishes the triage. TG013 was the one rule left without a verdict, and TG002 had two causes still standing.
TG013 — 3 false of 10, two narrow causes
A download is not a redundant upload.
_device_argumentaccepted a"cpu"literal as a destination, so this was reported:That's
pinmem_nonblock.py— a tutorial whose entire subject is measuring transfer behaviour, copying to the host, on a tensor already on the device. Wrong twice over.Restoring the device after a deliberate
.cpu()is required.fast_neural_styledoes:Hoisting it would leave the model on the host for the rest of training.
The other 7 are true — the mild
device=advice the rule exists to give (a host factory ortorch.tensor(python_list)inside a loop), plus one genuinely hoistable constant,self.STOP_TOKENS_TENSOR.to(self._device)in torchtune.TG002 —
from_pretrainedmade a tokenizer a modelfrom_pretrainedsits inMODEL_WRAPPERSand matched before anything could object, soAutoTokenizer.from_pretrained(...)was a model and this was a forward pass:We reported a missing
no_gradaround tokenisation. Feature extractors and image processors the same. The exclusion is by class name and runs first; a guard test assertsAutoModelForCausalLM.from_pretrainedstill counts as a model.Result
Five removed, zero new findings.
Where the triage ends up
Every rule now has a verdict. Remaining:
TG004207 (verified accurate, notes),TG00831,TG0033 (true, fixed upstream in pytorch/examples#1424),TG0137,TG00110,TG0026.Two items are left as decisions rather than fixes, recorded in the TODO:
noteunder RFC 0003, so it deserves the same test: is this code defective, or merely untuned?labelnaming heuristic on non-tensor lists, and container element types. One finding is not yet a pattern.Testing
447 tests, 4 new — two per fix, each including the guard for the case the fix must not silence.
Stacks on #49 and #50.