FEAT add SATA masking converter - #2404
FEAT add SATA masking converter#2404Alireza Aminzadeh (alireza-aminzadeh) wants to merge 2 commits into
Conversation
|
@microsoft-github-policy-service agree |
| Build the converter identifier with SATA masking parameters. | ||
|
|
||
| Returns: | ||
| ComponentIdentifier: The identifier for this converter. |
There was a problem hiding this comment.
This identifier can be misleading when a custom strategy is supplied: it still records num_masks and skip_first even though those values are ignored. It also omits task_template, so converters that produce different framed output can have identical identifiers. Please make the identifier describe the behavior actually in use, including the framing template when applicable.
There was a problem hiding this comment.
The identifier now always records mask_token and the selection-strategy class. num_masks, skip_first, and min_word_length are included only for the default strategy, since a custom strategy ignores them. Framing is no longer applied inside this converter, so there is no template parameter to list.
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
| ) | ||
|
|
||
| _WHITESPACE_RE = re.compile(r"(\s+)") | ||
| _WORD_AFFIX_RE = re.compile(r"^(\W*)(.*?)(\W*)$", re.UNICODE) |
There was a problem hiding this comment.
This pattern has quadratic backtracking for tokens with a long interior run of non-word characters. On the current branch, a 16K-character token takes roughly 3.5 seconds, and this runs synchronously inside convert_async, so one model-controlled token can stall the event loop. Could we split the leading and trailing \W* scans into separate anchored matches (or otherwise use a linear tokenizer) and add a regression case for a long token?
| if self._uses_default_strategy: | ||
| params["num_masks"] = self._num_masks | ||
| params["skip_first"] = self._skip_first | ||
| params["min_word_length"] = self._min_word_length |
There was a problem hiding this comment.
Thanks for removing the ignored default parameters for custom strategies. One collision remains for the internally-created strategy: stopwords and candidate_words both change which words are masked, but neither is represented here. For example, converters with different candidate_words produce different output while comparing equal by identifier. Please retain these values in a stable, order-independent form and include them in the identifier, with an inequality test.
| sata_masked = await sata_mask.convert_async(prompt=prompt) # type: ignore | ||
| print("SATA Mask:", sata_masked) | ||
| sata_frame = TaskFramingConverter(task_template=SATA_TASK_TEMPLATE) | ||
| print("SATA Framed:", await sata_frame.convert_async(prompt=sata_masked.output_text)) # type: ignore |
There was a problem hiding this comment.
Could you re-execute this notebook before merge? The committed .ipynb contains this source but no SATA Mask or SATA Framed output. The converter modality notebook also needs regeneration so doc/code/converters/0_converters.ipynb includes SATAMaskingConverter, as required for new converters.
| raise ValueError(f"Input type {input_type} not supported") | ||
|
|
||
| pieces, words = self._tokenize(prompt) | ||
| cores = [core for _, core, _ in words] |
There was a problem hiding this comment.
Passing only word cores changes the index space and matching behavior compared with SelectiveTextConverter, even though the class docstring says its strategies can be reused here. Empty tokens and punctuation-only tokens are dropped, so the same WordIndexSelectionStrategy can select a different word in the two converters. Could we either preserve the raw token/index stream expected by existing strategies, or explicitly document that these strategies are not index-compatible?
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