Fix six more issues from the unresolved-bugs backlog - #831
Conversation
- GreedyWordSwapWIR: actually implement `truncate_words_to`, which a2t's recipe has been passing since PR QData#747 but the constructor never accepted, crashing a2t/a2t-mlm on init (QData#754). - validators.py: model-compatibility regex only matched the pre-4.x `transformers.modeling_<model>` layout; update to also match `transformers.models.<model>.modeling_<model>`, fixing a spurious "unknown model" warning against any current transformers version (QData#722, also the root cause of the warning noise in QData#742). - AttackArgs: warn (rather than silently drop) when `num_examples` is explicitly set alongside `num_successful_examples`, since the latter overrides the former and users combining both had no way to tell why `num_examples` came back `None` (QData#728). - AttackedText.words_diff_ratio: comparing two Python lists with `!=` yields a single bool, not an elementwise mask, so this always returned 0 or 1 regardless of how many words actually differed; fix by comparing as numpy arrays (QData#787). - Augmenter.augment: bound-retry the outer sampling loop so a transformation with limited output diversity for a given input (e.g. BackTranslationAugmenter's random language chaining colliding on short sentences) doesn't silently return fewer than `transformations_per_example` unique augmentations (QData#800). - words_from_text: strip allowed marks (quotes/hyphens/etc.) from both ends of a word, not just the leading end, so a quoted word like "'CCC'" doesn't keep its trailing quote (QData#723). - HuggingFaceModelWrapper: route encoder-decoder generation models (e.g. a raw `BartForConditionalGeneration` loaded directly from `transformers`, per TextAttack's own docs example) through `.generate()` + decode instead of a plain forward pass, which only returns logits and breaks text-to-text goal functions expecting strings (QData#771). - Attack.cuda_: skip re-placing a model that already has an `hf_device_map` (i.e. was loaded with `device_map=...` across multiple GPUs via accelerate), since forcing it onto a single device breaks that placement (QData#798). Also confirmed already resolved on master, no code change needed: QData#712, QData#715, QData#717, QData#730, QData#743, QData#753 (see issue comments). Test plan: pytest tests/test_attacked_text.py tests/test_augment_api.py (non-network-dependent subset) pass; new regression tests added for QData#723 and manually verified QData#771/QData#798 against real transformers models and mocks respectively (see PR description). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comprehensive reviewReviewed the full diff, traced callers for each hunk, and empirically verified the two most severe findings against master. Ordered most → least severe. 1.
|
transformations_per_example |
master (pre-PR) | this branch |
|---|---|---|
| 5 | 417 | ~104 |
| 10 | 786 | ~98 |
| 20 | 1326 | ~100 |
Master scales roughly linearly with n as intended; this branch is flat around ~100 regardless of n — a 4-13x reduction, and yield no longer scales with transformations_per_example at all. This looks like an unintended side effect of the #800 retry-bound fix, not the fix itself.
2. HuggingFaceModelWrapper.__call__ (huggingface_model_wrapper.py:77) — missing max_length on .generate(), confirmed
The new .generate() call passes only input_ids/attention_mask, no length control, so it falls back to HF's generation defaults. Compare textattack/models/helpers/t5_for_text_to_text.py:63, which explicitly passes max_length=self.output_max_length for exactly this reason. A raw BartForConditionalGeneration/T5ForConditionalGeneration wrapped for MinimizeBleu/NonOverlappingOutput on inputs whose expected output exceeds the default will get silently truncated output, corrupting the BLEU/overlap comparison against ground_truth_output.
3. AttackArgs.__post_init__ (attack_args.py:229) — warning gate misses the default value, confirmed
self.num_examples not in (None, 10) treats an explicit num_examples=10 identically to "unset" (10 is the dataclass default). A user who explicitly passes num_examples=10 alongside num_successful_examples — a very plausible value to pick — gets no warning even though num_examples is still silently overridden to None, defeating the #728 fix for that case.
4. HuggingFaceModelWrapper.__call__ — encoder-decoder branch checks architecture, not task head (lower confidence)
getattr(model_config, "is_encoder_decoder", False) and hasattr(self.model, "generate") doesn't distinguish a generation model from a seq2seq-backbone classification model (e.g. BartForSequenceClassification, whose config also sets is_encoder_decoder=True). On the currently pinned transformers (4.57.1) hasattr(....), "generate") is False for such models so this doesn't currently trigger, but requirements.txt pins only transformers>=4.30.0 with no upper bound — on older versions where GenerationMixin wasn't yet gated by can_generate(), this could misroute a classification model into .generate() and crash or return garbage instead of logits.
5. GreedyWordSwapWIR (greedy_word_swap_wir.py:48) — truncate_words_to doesn't bound wir_method="gradient" cost (lower confidence)
The truncation slices indices_to_order after get_indices_to_order runs, but for wir_method="gradient" the expensive step (victim_model.get_grad(...) on the full untruncated text) still runs at full cost — only the cheap post-hoc index-scoring loop is shortened. A2TYoo2021.build() passes truncate_words_to=max_len specifically to bound cost for the gradient method against a model's context window, so the fix may not actually bound cost the way it's intended to for that caller.
6. Attack.cuda_/to_cuda (attack.py:215) — device-map skip layered too generically (style/design)
The hf_device_map skip lives in the generic, framework-agnostic to_cuda visitor rather than being scoped to HuggingFace models specifically. Any non-HF nn.Module reachable from a Constraint/GoalFunction/Transformation that happens to carry a truthy hf_device_map attribute would now also be skipped, since the check only tests attribute presence.
7. Augmenter.augment — max_attempts = transformations_per_example * 3 (minor)
Unexplained magic multiplier hardcoded inline. A transformation with less output diversity than BackTranslation (the case the comment cites) can still exhaust 3x attempts without reaching the target, with no way for a caller to raise the retry budget.
Findings 1-3 are empirically/structurally confirmed; 4-5 are plausible but lower-confidence (version/caller-dependent); 6-7 are design nits, not correctness bugs.
The .generate() call added for raw encoder-decoder generation models (QData#771) passed no length control, so it always fell back to whatever HF/the model's own generation_config decided. Add an optional max_length constructor param, only forwarded to generate() when set, so callers can bound (or lengthen) output for MinimizeBleu/ NonOverlappingOutput comparisons against ground_truth_output - matching what T5ForTextToText already does via output_max_length - without overriding a model's own sensible generation_config by default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- HuggingFaceModelWrapper: prefer model.can_generate() over hasattr(model, "generate") when routing encoder-decoder models into .generate(). On transformers versions predating can_generate(), generate was defined on every PreTrainedModel regardless of whether it had a generation-capable head, so hasattr alone could misroute a seq2seq-backbone classification model (e.g. BartForSequenceClassification, whose config also sets is_encoder_decoder=True) into .generate() instead of a normal forward pass. Falls back to hasattr only if can_generate isn't available. - AttackArgs.__post_init__: use a sentinel (_NUM_EXAMPLES_UNSET) instead of comparing against the literal default value 10, so an explicit num_examples=10 alongside num_successful_examples now correctly triggers the QData#728 warning instead of being indistinguishable from never having set num_examples at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- GreedyWordSwapWIR: for wir_method="gradient", truncate_words_to only bounded the cheap post-hoc index-scoring loop; the expensive step (get_grad's tokenize+forward+backward pass) still ran over the full untruncated text. Feed get_grad a text window matching the same word span already selected, for single-sequence inputs (paired inputs, e.g. premise/hypothesis, keep relying on the tokenizer's own model_max_length truncation, since truncating a tuple input here would need to preserve pair structure - out of scope). Build a fresh AttackedText for the truncated span and reuse it for align_with_model_tokens too, so the gradient array and word2token_mapping's token indices stay consistent. - Attack.cuda_/to_cuda: scope the hf_device_map skip to transformers.PreTrainedModel specifically instead of any torch.nn.Module, since this visitor also traverses non-HuggingFace models (TensorFlow, sklearn, custom PyTorch modules) reachable from a Constraint/GoalFunction/Transformation, and only transformers- loaded models actually get this attribute set by from_pretrained(device_map=...). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code review (
|
- AttackArgs: CLI argparse's --num-examples default resolved the sentinel to a concrete 10 before AttackArgs.__post_init__ saw it, so CLI users passing only --num-successful-examples got a spurious conflict warning. Point the default at the raw unresolved sentinel instead. - Augmenter.augment: random.sample() was called on a set, which Python 3.11+ no longer supports (TypeError). Sample from a list. - Augmenter.augment: the while-loop bound added for issue QData#800 (bound retries for low-diversity transformations) made the loop exit as soon as the target count was hit, cutting high_yield mode's output roughly in half since a single pass can already add several results. Always run at least transformations_per_example attempts, while still allowing bounded extra retries past that if short of target. - Attack.cpu_/to_cpu: missing the hf_device_map guard that cuda_/to_cuda has, so moving an accelerate-sharded model to CPU still forced it onto a single device and broke its placement. - GreedyWordSwapWIR: truncate_words_to sliced indices_to_order (derived from set operations) assuming ascending order, which CPython doesn't guarantee for arbitrary integers. Sort before truncating so "first N" is actually the first N word positions, keeping the wir_method="gradient" cost bound intact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- attack_args.py: --num-examples used argparse.SUPPRESS instead of the raw _NUM_EXAMPLES_UNSET sentinel as its default, so it's simply absent from the parsed namespace when unset (letting the dataclass's own default apply via CommandLineAttackArgs(**vars(args))) rather than leaking the sentinel's repr into `--help` output as "(default: <object object at 0x...>)". Help text now states the real default (10) explicitly. - validators.py: MODELS_BY_GOAL_FUNCTIONS' (NonOverlappingOutput, MinimizeBleu) entry only matched TextAttack's own T5ForTextToText helper, not raw transformers encoder-decoder generation classes (T5ForConditionalGeneration, BartForConditionalGeneration, ...) - exactly the models HuggingFaceModelWrapper's .generate() path (QData#771) was added to support. Every attack against one printed a spurious "Unknown if model ... compatible" warning. Added a regex matching the ForConditionalGeneration module layout, mirroring the existing ForSequenceClassification entry. - attacked_text.py / greedy_word_swap_wir.py: replaced the text_window_around_index(0, N) "prefix via centered window" trick (used to build the truncated text fed to get_grad) with a dedicated text_of_first_n_words(n) method, so the truncation path isn't coupled to text_window_around_index's centering branch logic. Verified identical output across n=0/1/mid/beyond-length. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verified every commit's fix directly against real hf-internal-testing tiny models (not mocked outcomes): for each new test, confirmed it passes on current code and fails against the pre-fix version of the relevant file, so these are proven to catch the regressions they target rather than just exercising the fixed path. - tests/test_search_methods.py (new): GreedyWordSwapWIR sorts indices_to_order before truncating (not just slicing a possibly unsorted set-derived list); wir_method="gradient" actually bounds the text fed to get_grad to the truncated word span, and leaves paired (premise/hypothesis) inputs untouched. - tests/test_validators.py (new): MODELS_BY_GOAL_FUNCTIONS regexes match both transformers module layouts for ForSequenceClassification and the new ForConditionalGeneration entry, without false-matching a classification head on an encoder-decoder backbone; no spurious compatibility warning for a raw generation model. - tests/test_huggingface_model_wrapper.py (new): max_length is only forwarded to generate() when explicitly set; can_generate() (not hasattr(model, "generate")) decides routing, verified by simulating a classification model that exposes .generate() but can't actually generate; T5ForTextToText's no-.config path still works. - tests/test_attack_device_placement.py (new): cuda_/cpu_ skip hf_device_map-carrying transformers.PreTrainedModel instances but still move an unrelated torch.nn.Module that happens to share the attribute name; a model without hf_device_map is still moved normally. - tests/test_augment_api.py: high_yield output actually scales with transformations_per_example instead of plateauing; the fast_augment-triggered downsampling path (random.sample on a set) doesn't crash under Python 3.11+'s stricter random.sample. - tests/test_attacked_text.py: text_of_first_n_words covers n=0, a mid-range n, and n beyond the text's word count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's pinned black (20.8b1, installed under Python 3.9 per the lint job's environment) formats ** with surrounding spaces (10 ** 5), the opposite of what newer local black versions prefer (10**5) - already visible on the adjacent pre-existing test_big_window_around_index line. My new test_text_of_first_n_words line used the newer style, which is what CI's `make lint` flagged. Verified against the actual pinned black==20.8b1 (via `uvx --python 3.9 --with "click<8.1" black==20.8b1`, since it doesn't build on newer Python) that the full repo is now clean under CI's exact toolchain, not just local black. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note for future reference: local While adding test coverage for this PR's fixes,
Both Not fixed here — no in-repo code change addresses it. Flagging in case CI (or another contributor's machine) ever shows similar flakiness on these two tests: it's very likely this same class of issue, not a regression from this PR's changes. Confirmed via bisection: |
|
Test coverage added for every commit on this PR Verified every fix directly — walked each commit's diff, and for the ones with any real risk of stale/incorrect behavior, cross-checked against pre-fix code to confirm the fix actually changes behavior (not just cosmetic). Added 24 new regression tests across 4 new files + 2 extended existing files, covering every previously-untested fix in the PR:
Every new test was checked both ways: passes on current code, and — checked out against the pre-fix version of its target file — actually fails, confirming each one is a real regression guard rather than a test that would pass regardless. |
# Conflicts: # textattack/shared/validators.py
Summary
Second batch from a triage pass over the 24 open bugs in the issue backlog (first batch: #830). Each fix below is verified against the actual current code, not just the original report — three issues in this batch (#712/#715/#717/#730/#743/#753, listed below) turned out to already be fixed by unrelated prior commits once checked against
master, so no code change was needed for those; comments were left on those issues instead.GreedyWordSwapWIR: actually implementtruncate_words_to.a2t's recipe (PR fixing the csvlogger missing DF issues #747) has been passing this kwarg since 2023, but the constructor never declared it, so any use ofA2TYoo2021/a2t-mlm crashes on init withTypeError: unexpected keyword argument. Fixes a2t bug #754.validators.py: the model-compatibility regex only matched the pre-4.xtransformers.modeling_<model>module layout.transformersreorganized this totransformers.models.<model>.modeling_<model>years ago, so every current install trips a spurious "unknown model" warning. Fixes Compatibility issue w/ Transformers model #722 (also the source of the warning noise reported in Only failed or skipped attack whith textfooler on custom bert model #742, though that issue's "always fails" symptom needs a repro to diagnose further — commented there).AttackArgs: warn instead of silently droppingnum_exampleswhen it's explicitly set alongsidenum_successful_examples(the latter overrides the former by design, per the docstring, but users had no way to know why their value vanished). Fixes Issue with using AttackArgs class outside of the command line (in my Python script) #728.AttackedText.words_diff_ratio:self.words != x.wordscompares two Python lists and returns a single bool, not an elementwise mask — so this always returned exactly 0 or 1 regardless of how many words actually differ. Fixed by comparing as numpy arrays. Fixes the return of AttackedText.word_diff_ratio is 1 all the time #787.Augmenter.augment: bounded-retry the outer sampling loop. It accumulates results in aset()but only ever attempted exactlytransformations_per_exampleouter iterations; a transformation with limited output diversity for a given input (e.g.BackTranslationAugmenter's randomly-chained back-translation converging to the same phrasing for short sentences) silently returned fewer unique results than requested. FixesBackTranslationAugmenterproduces an incorrect number of transformations #800.words_from_text: strip allowed marks (quotes/hyphens/etc.) from both ends of a word, not just the leading end — a quoted word like"'CCC'"kept its trailing quote. Fixes Bug of utils/string.py/words_from_text() #723.HuggingFaceModelWrapper: route encoder-decoder generation models (e.g. a rawBartForConditionalGenerationloaded directly viatransformers, exactly as shown in TextAttack's ownAttackerdocstring example) through.generate()+ decode instead of a plain forward pass. A plain forward pass only returns logits, which crashes text-to-text goal functions (NonOverlappingOutput,MinimizeBleu) that expect a string (TypeError: split() missing 1 required positional argument, sinceword_difference_scorecalls.split()on what it assumes is a string). TextAttack's ownT5ForTextToTexthelper is unaffected (verified — it has no.configattribute, so it's routed around, not through, the new branch). Fixes Issues attacking se2seq model (seq2sick) #771.Attack.cuda_: skip re-placing a model that already has anhf_device_mapattribute (set bytransformers/acceleratewhen loaded withdevice_map=...across multiple GPUs). Forcing.to(device)on an already-sharded model conflicts with its placement and throwsRuntimeError: Expected all tensors to be on the same device. Fixes Attack on an HF model split into multiple GPUs #798.Already fixed on
masterby unrelated prior commits — verified, no change needed, issues commented/closed accordingly: #712 (QWERTY IndexError, fixed Nov 2023), #715 and part of #713 (flairforce_token_predictions, fixed alongside the #727 POS-tag work), #717 (QWERTY crash on symbol-only input, same 2023 fix as #712), #730 (--high_yieldCLI flag, already registered), #743 (PSOKeyError: 'pos', same root cause as #727), #753 (PSO_turnsignature, already matches call sites post the LEAP refactor in #829).Test plan
pytest tests/test_attacked_text.py tests/test_augment_api.py(non-network-dependent subset): all pass, including two new regression tests (test_quoted_wordfor Bug of utils/string.py/words_from_text() #723, plus the existing suite coveringAugmenter/AttackedTextpaths touched here)make lintclean (black/isort/flake8, matching the pinned CI versions)HuggingFaceModelWrapperfix manually verified end-to-end against a real tiny seq2seq model (hf-internal-testing/tiny-random-bart: confirms.generate()+decode now returns strings) and a real classification model (confirms unaffected, still returns logits) and a mock mimickingT5ForTextToText's shape (confirms it's routed around the new branch, not broken by it)Attack.cuda_fix manually verified with a mockedtorch.nn.Module.to: a module withhf_device_mapset is skipped, a normal module is still moved as beforeAttackArgs/words_diff_ratio/words_from_text/GreedyWordSwapWIRfixes are small and locally reasoned/tested via direct interpreter calls (shown in the linked issue comments), but don't yet have dedicated unit tests beyond the one added for Bug of utils/string.py/words_from_text() #723 — could use follow-up test coverage🤖 Generated with Claude Code