[WIP] Ponytail's code cleanup - #10
Open
matanbt wants to merge 1 commit into
Open
Conversation
Repo-wide over-engineering audit (via the ponytail plugin) and its fixes. Net -409 lines of production code, +120 of new tests. No behavior changes beyond those noted below. Dependencies - Drop pandas, scikit-learn and requests from core deps. All three existed only for tropt/utils/refusal_dir.py, which now uses `datasets` (already a dep, lazily imported) for the AdvBench CSV and stdlib `random` for the train/test split. `pip install tropt` no longer pulls scipy. - Move tenacity into the openai/google/voyage extras, where it is actually used, and import it lazily. De-duplication - compute_grad_from_tokens / compute_grad_from_embeds were ~115 near-identical lines each; both now delegate to _grad_wrt_leaves, which differentiates whichever leaf it is handed. - The gemini and voyage encoders had cloned transient-error predicates, retry logging and chunk loops; both now use tropt/model/api_retry.py. openai moves from a blanket retry to the same transient-only policy, so a 4xx no longer burns five attempts. - _get_trigger_variations was copy-pasted across three optimizers; replaced by a single vectorised random_single_flips in optimizer/utils. - Hoisted the identical tensor-sanitising loop out of the wandb and trackio trackers into BaseTracker._scalarize. Deletions - Six loss "category" base classes with one subclass each. Losses resolve by parameter name, not by type, so these enforced nothing; their require_* flags now sit on the concrete losses. PrefillBasedLoss (4 subclasses) and TextBasedLoss stay. - FlopCounterBase (one implementation), its never-called count_backward, and 65 lines of commented-out track_flops. - InputsManager ABC: its __init__ raised and its abstract method had divergent signatures in both subclasses. - NFlipScheduler ABC and ConstantScheduler; a scheduler is now any (step) -> int callable, so the constant case is a lambda. - Dead symbols: TextTrigger, BaseModel.__init__, Targets.n_templates, contains_loss_type, OpenAITokenizer._parse_ids, an identity lookup dict. Docs and tooling - CLAUDE.md documented runner/main.py and TESTING.md; neither exists. Removed those, the commented-out hydra deps, and the stale autodoc mocks. - Extracted the future-annotations AST injector, duplicated between build_docs.py and deploy_docs.yml, into docs/scripts/inject_annotations.py. build_docs.py now calls it; the matching CI change is left out of this commit for separate review. - build_docs.py cleanup uses rmtree's error hook instead of a chmod tree-walk (the Drive-lock retry stays, it is load-bearing). Behaviour notes - random_single_flips draws all flips in one vectorised call, so a seeded run produces a different RNG stream than before (same distribution). - The refusal-direction train/test split no longer matches sklearn's exact permutation, so a different subset of prompts is sampled. Verified: ruff, ty, and pytest all clean. New tests cover the embedding branch of the gradient helper and both modes of random_single_flips, including a chain-rule identity pinning the two gradient paths together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
[Human:] this is pending a human review; currently backlogged. |
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.
Repo-wide over-engineering audit — run with the ponytail plugin's
/ponytail-audit— and its fixes.Net −409 lines of production code, +120 of new tests, −3 core dependencies.
ruff,tyandpytestall clean.Dependencies
pip install troptwas pulling pandas, scikit-learn and requests (and transitively scipy) for exactly one file:tropt/utils/refusal_dir.py, itself used by one recipe. It now reads the AdvBench CSV viadatasets(already a dependency, lazily imported) and splits train/test with stdlibrandom.tenacitymoves to theopenai/google/voyageextras where it's actually used.De-duplication
compute_grad_from_tokens/compute_grad_from_embeds_grad_wrt_leavestropt/model/api_retry.py_get_trigger_variationsrandom_single_flipsinoptimizer/utils_logBaseTracker._scalarizeopenai also moves from a blanket retry to the shared transient-only policy, so a 4xx no longer burns five attempts.
Deletions
require_*flags now sit on the concrete losses.PrefillBasedLoss(4 subclasses) andTextBasedLossstay because they carry something real.FlopCounterBase(one implementation), its never-calledcount_backward, and 65 lines of commented-outtrack_flops.InputsManagerABC — its__init__raised and its abstract method had divergent signatures in both subclasses.NFlipSchedulerABC +ConstantScheduler; a scheduler is now any(step) -> intcallable, so the constant case is a lambda and users can pass a plain function.TextTrigger,BaseModel.__init__,Targets.n_templates,contains_loss_type,OpenAITokenizer._parse_ids, an identity lookup dict.Docs and tooling
CLAUDE.mdpointed atrunner/main.pyandTESTING.md— neither exists in the repo. Removed those, the commented-out hydra deps, and the stale autodoc mocks. This is the first thing an outside contributor would have hit.build_docs.pyanddeploy_docs.yml) intodocs/scripts/inject_annotations.py.build_docs.pycleanup usesrmtree's error hook instead of a chmod tree-walk. The Drive-lock retry stays — it's load-bearing on Windows.Tests
New
tests/test_optimizer_utils.pycovers the two paths the end-to-end suite doesn't reach: the embedding branch of the gradient helper (PEZ / SoftPrompt flow) and both modes ofrandom_single_flips. It includes a chain-rule identity —dL/d(onehot) == dL/d(embeds) @ Eᵀ— that pins the two gradient paths together, so a mis-wired leaf in either branch fails the build.Reviewer notes
Two intentional behaviour changes:
random_single_flipsdraws all flips in one vectorised call, so a seeded run produces a different RNG stream than before (same distribution, so results are statistically equivalent — but exact reruns of old seeds won't match).One hunk is deliberately not in this PR: the matching
.github/workflows/deploy_docs.ymlchange, which swaps CI's inline heredoc for the extracted script. Until that lands, CI keeps its own working copy of the injector — no breakage, just the duplication left standing.Three audit findings were investigated and dropped rather than forced:
<details>— they'resphinx-designgrid-item-carddirectives, so the swap means hand-rolling the card grid, a bigger change than the 30 lines of JS it removes.CombinedLoss's sevenrequire_*properties in a loop —BaseLossdefines them as ClassVars, so__getattr__never fires, and thesetattralternative trades 20 lines for broken IDE autocomplete.DictTracker/JSONTracker/PrintTracker— each is a distinct output sink and they're public API on a package mid-promotion.🤖 Generated with Claude Code