fix: three false positives found by re-triaging warnings and notes - #50
Merged
Conversation
Read all 283 findings across the seven repos rather than clustering them, and at every severity. The errors had been re-read three times across this work; TG007 had never been looked at since it shipped, because it never produces one. It was the worst rule in the set. TG007 was 6/6 false, and each finding was the rule reporting its own advice. Every one was `correct += (predicted == labels).sum().item()` in a validation loop nested in a training loop -- verbatim what the hint tells you to write. The batch-loop exemption matched iterable *names* (`loader`, `dataloader`, `batches`) so it missed `dev_iter` and `valloader`. Lengthening the list would have patched the instance; the rule now requires evidence of per-element iteration instead, since a loop over `range(...)` indexes elements and a loop over anything else yields batches whatever it is called. TG002 reported that `fgsm_tutorial.py`'s `test()` "never calls `.backward()`" while the call sat nine lines below. An adversarial attack iterates `test_loader` and backwards through it deliberately, to get gradients with respect to the input. The carve-out being corrected exists for functions that both train and validate, where a backward elsewhere should not excuse the validation loop -- it just has to check the backward is not in *this* loop. TG001's bare-name return rule was too blunt. `return logps` where `logps = torch.cat(all_logps, dim=0)` is not the container being handed back, it is a reduction of it, and returning it hands the caller a graph the GRPO objective backwards. A returned bare name now counts unless it is itself a holder, which cannot be decided until every holder has been seen. Six findings across trl and torchtune. 14 removed, no new findings. Errors 21 -> 13, warnings 55 -> 49. TG004 came out accurate: a sample of the 207 notes were all genuine DataLoader calls missing num_workers or pin_memory. The problem was volume, which making it a note already solved.
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.
Read all 283 findings across the seven repos — individually, and at every severity, not just errors. The errors had already been re-read three times across this work. TG007 had never been looked at since it shipped, because it never produces an error. It was the worst rule in the set.
TG007 was 6/6 false, and every finding was the rule reporting its own advice
That is verbatim what TG007's hint tells you to write: "Do the reduction on the device and sync once —
(preds == targets).sum().item()." All six findings were this shape, in a validation loop nested inside a training loop.The rule already had a batch-loop exemption — but it matched iterable names (
loader,dataloader,batches), so it misseddev_iterandvalloader.Lengthening that list would have patched the instance. Instead the rule now requires evidence of per-element iteration: a loop over
range(...)indexes elements, a loop over anything else yields batches whatever it happens to be called. That is also the shape the rule's own docstring uses to explain itself.TG002 reported a
.backward()nine lines above the callfgsm_tutorial.py'stest()was reported as "runs a forward pass with autograd enabled but never calls.backward()". It calls it on line 292 — an adversarial attack iteratestest_loaderand backwards through it deliberately, to get gradients with respect to the input.The carve-out at fault exists for functions that both train and validate, where a backward elsewhere shouldn't excuse the validation loop. It just has to check the backward isn't in this loop.
TG001's bare-name return rule was too blunt
From #43: a returned bare name is not evidence of a deferred backward. Correct for
return losses, where the container itself is handed back. Wrong here:Returning that hands the caller a graph the GRPO objective backwards. A returned bare name now counts unless it is itself a holder — which can't be decided until every holder has been seen, so it resolves at
leave_Module. Six findings acrosstrland torchtune.Result
14 removed, zero new findings.
What came out clean
DataLoadercalls missingnum_workersorpin_memory. The problem was only volume and actionability, which making it a note under RFC 0003 already solved. No rule bug.running_loss += current_losswarnings are true, at the right severity since the severity split.Left open, recorded in the TODO
note, so it should face the same test — is this code defective, or merely untuned?tokenizer(...),feature_extractor(...)), and deliberate gradient use for attribution.Testing
443 tests, 3 new — one per fix, each reduced from the file that exposed it.