chore(typing): Add type checking to src/transformers/generation#44233
chore(typing): Add type checking to src/transformers/generation#44233Cyrilvallez merged 22 commits intomainfrom
src/transformers/generation#44233Conversation
| self.loop = asyncio.get_running_loop() | ||
| self.has_asyncio_timeout = hasattr(asyncio, "timeout") | ||
| timeout_context = getattr(asyncio, "timeout", None) | ||
| self.has_asyncio_timeout = sys.version_info >= (3, 11) and callable(timeout_context) |
There was a problem hiding this comment.
we can tighten this with the python version (function was added in 3.11)
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
8c59847 to
1dfddc1
Compare
zucchini-nlp
left a comment
There was a problem hiding this comment.
Apart from a few nits, my only question is about audio-model specific attributes on generation config. We indeed have several special attributes, and I think whisper has the largest amount
Is there any way to not add them all as class attributes in the general GenerationConfig, because they don't need to be accessible when loading a text only model?
a0a0445 to
a212761
Compare
Yeah we don't want to pollute The right pattern imho:
That avoids polluting text-only configs while keeping static typing strict where those attrs are actually used. |
Cyrilvallez
left a comment
There was a problem hiding this comment.
Nice in general! Just left a few comments to try to make it a bit simpler!
| cache_to_check = self._cache.self_attention_cache if self.config.is_encoder_decoder else self._cache | ||
| if isinstance(self._cache, EncoderDecoderCache): | ||
| cache_to_check = self._cache.self_attention_cache | ||
| elif isinstance(self._cache, StaticCache): | ||
| cache_to_check = self._cache |
There was a problem hiding this comment.
I believe both checks are equivalent indeed, but curious to know why we need it to change?
There was a problem hiding this comment.
I changed it to use isinstance-based narrowing for two reasons:
- static typing => config flags don’t narrow self._cache’s runtime type, so this avoids attribute-type issues;
- robustness => if
self._cacheis ever aStaticCachewhileconfig.is_encoder_decoderisTrue(stale/custom cache state), the old ternary can hit.self_attention_cacheon the wrong type. Theisinstanceversion is defensive and keeps behavior the same in expected cases.
388547f to
b3e6799
Compare
|
run-slow: clvp, musicgen, musicgen_melody |
src/transformers/generationsrc/transformers/generation
|
This comment contains models: ["models/clvp", "models/musicgen", "models/musicgen_melody"] |
|
run-slow: clvp, musicgen, musicgen_melody |
|
This comment contains models: ["models/clvp", "models/musicgen", "models/musicgen_melody"] |
Cyrilvallez
left a comment
There was a problem hiding this comment.
Nice! Would love to be able to remove the conditional inheritance though, I believe we cam by annotating a few more self?
9c65644 to
db703e7
Compare
Cyrilvallez
left a comment
There was a problem hiding this comment.
Alright, thanks for updating! Let's just avoid redefining a new GenerativePreTrainedModel class in generation/utils.py, everything could and should be inside _typing.py! Feel free to merge afterwards!
Add type declarations for mixin host-class attributes on GenerationMixin, class-level annotations for dynamically-set attributes on GenerationConfig, and fix minor typing issues in candidate_generator, watermarking, and stopping_criteria. Create _typing.py Protocol for documentation/reuse.
…with if guards - Replace assert narrowing with if guards (candidate_generator.py) - Add if-guard for model_kwargs None in _prepare_model_inputs and _maybe_initialize_input_ids_for_generation - Add if-guard for encoder_input_ids None checks - Fix save_directory.split() for PathLike by wrapping with str() - Fix tuple[Tensor] reassignment in compute_transition_scores - Add type annotation for self.model in WhisperTimeStampLogitsProcessor - Add type annotation for self.sequence_bias in SequenceBiasLogitsProcessor - Add no-assert rule to AGENTS.md typing strategy
e21d1d5 to
ba7ae36
Compare
|
run-slow: clvp, musicgen, musicgen_melody |
|
This comment contains models: ["models/clvp", "models/musicgen", "models/musicgen_melody"] |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: clvp, musicgen, musicgen_melody |
Improve LLM factory typing so generated model classes expose generation
APIs for static analysis.
- Add GenerationMixin import in the LLM registry module.
- Introduce PreTrainedModelWithGenerationMixin as a typed base.
- Update ModelAndTokenizer to return this mixed model type.
Note:
- Recent upstream PR introduced GenerativePreTrainedModel Protocol:
huggingface/transformers#44233
- This appears to trigger analyzer warnings from protocol checks.
- The warning behavior is suspected upstream typing friction and is
temporarily ignored.
Extends
tycoverage tosrc/transformers/generationutils/check_types.py.Makefileto runtychecks through the wrapper in bothstyleandcheck-repo.src/transformers/_typing.pyTransformersLoggerGenerativePreTrainedModelWhisperGenerationConfigLikeclvp,musicgen,musicgen_melody