FEAT add SATA masking converter - #2404
FEAT add SATA masking converter#2404Alireza Aminzadeh (alireza-aminzadeh) wants to merge 5 commits into
Conversation
|
@microsoft-github-policy-service agree |
Add a dependency-free converter that selects content words and replaces them with [MASK] so SATA can be composed with TaskFramingConverter.
Keep TaskFramingConverter composition outside this converter, reject mixed selection_strategy configuration, and identify only the parameters that affect output.
336e688 to
8c065a8
Compare
…ess, and notebook outputs.
…h SelectiveTextConverter.
| if prefix_end == len(token): | ||
| return "", token, "" | ||
| body = token[prefix_end:] | ||
| trailing = _TRAILING_NONWORD_RE.search(body) |
There was a problem hiding this comment.
The leading scan is linear now, but this trailing search() still retries \W+$ at every position when a long punctuation run is followed by a word character. On the current head, "a" + "-" * 32000 + "b" takes about 9.8 seconds through convert_async. Could we reuse the anchored leading matcher on the reversed body (or another truly linear suffix scan) and change the regression test to this interior-run shape? The current suffix-only tests do not exercise the remaining quadratic path.
There was a problem hiding this comment.
The trailing scan now reuses the same anchored ^\W+ matcher on the reversed remainder, so both sides stay linear. The regression test uses "a" + "-" * 32000 + "b" (the interior-run case) and stays well under the time bound.
| ``WordSelectionStrategy`` (including those used with ``SelectiveTextConverter``). | ||
| Strategies receive every whitespace-delimited token, including | ||
| punctuation-only tokens, so index-based selection matches | ||
| ``SelectiveTextConverter`` on space-separated text. |
There was a problem hiding this comment.
This is aligned for single-space-separated input, but the claim is broader than the implementation. This converter splits on \s+ and drops empty pieces, while SelectiveTextConverter uses split(" "), so leading spaces, doubled spaces, tabs, and newlines shift the index space or behave differently. Could we either narrow the documented compatibility boundary or align the tokenization fully? A few parametrized cases around those inputs would make the contract clear.
There was a problem hiding this comment.
Agreed that the previous claim was too broad. The docstring now states that WordIndexSelectionStrategy indices match SelectiveTextConverter only for single-space-separated prompts, and that tabs, newlines, and doubled/leading spaces are a different index space because this converter tokenizes on \s+. Added parametrized tests for those cases.
| """ | ||
| params: dict[str, Any] = { | ||
| "mask_token": self._mask_token, | ||
| "selection_strategy": self._selection_strategy.__class__.__name__, |
There was a problem hiding this comment.
The default-strategy identifier is complete now, but custom strategies still collapse to the class name. Two ContentWordSelectionStrategy instances with different max_words, skip_first, or candidate sets can produce different output while this converter reports the same identifier. Could the strategy expose its behavioral identifier parameters and have this converter delegate to them? That would also avoid maintaining a second copy of the default strategy configuration here.
There was a problem hiding this comment.
WordSelectionStrategy now exposes get_identifier_params(), and SATAMaskingConverter delegates to that instead of keeping a second copy of the default-strategy config. Custom ContentWordSelectionStrategy instances with different max_words / skip_first / candidate sets now produce different identifiers; an equivalent explicit strategy matches the default constructor.
…ce limits, and delegate identifiers to strategies.
2 participants
Summary
SATAMaskingConverterand a reusableContentWordSelectionStrategythat selects content words with a deterministic, dependency-free heuristic (no NLTK or POS-tagger download).TaskFramingConverterviaSATA_TASK_TEMPLATE(the wiki-infill prompt from the SATA paper). It also works withSelectiveTextConverterif you want to apply a different sub-converter to the same word selection.I chose the dependency-free selector so this stays easy to test and does not add model-data downloads. Feedback welcome if maintainers would rather plug in an optional POS tagger later.
Test plan
pytest tests/unit/converter/test_sata_masking_converter.py tests/unit/converter/test_text_selection_strategy.py tests/unit/converter/test_task_framing_converter.py tests/unit/converter/test_selective_text_converter.py tests/unit/docs/test_converter_documentation.py(148 passed)doc/code/converters/1_text_to_text_converters.py