Separate check_model_inputs into capture_outputs and merge_with_config_defaults + ensure correctness#43862
Separate check_model_inputs into capture_outputs and merge_with_config_defaults + ensure correctness#43862Cyrilvallez merged 20 commits intomainfrom
check_model_inputs into capture_outputs and merge_with_config_defaults + ensure correctness#43862Conversation
|
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. |
Cyrilvallez
force-pushed
the
cleaner-decorators
branch
from
27acb6c to
28bbcce
Compare
Cyrilvallez
changed the title
check_model_inputs into capture_outputs and merge_with_config_defaultscheck_model_inputs into capture_outputs and merge_with_config_defaults + ensure correctness
|
[For maintainers] Suggested jobs to run (before merge) run-slow: afmoe, aimv2, albert, apertus, arcee, aria, audio_spectrogram_transformer, audioflamingo3, bert, bert_generation, bitnet, blip, blip_2, blt, camembert, chameleon |
vasqu
left a comment
vasqu
left a comment
There was a problem hiding this comment.
LGTM overall, does the chain order have any influence?
| # The sam model variants need this annoying exception as well... | ||
| if "mask_decoder_attentions" in capturable_flags: | ||
| recordable_keys["output_mask_decoder_attentions"] = kwargs.get( | ||
| "output_attentions", getattr(self.config, "output_attentions", False) | ||
| ) |
There was a problem hiding this comment.
Not super relevant to this PR as it goes a bit beyond the intended scope here but I've seen a few more exceptions I think with image_hidden_states etc. Could be worthwhile to checkout at some point
There was a problem hiding this comment.
If a Model uses a submodel that captures "hidden_states", and then later wants to rename it by doing return ModelOutput(image_hidden_states=vision_model_output.hidden_states,...) it's not a problem.
This issue is when "mask_decoder_attentions" is the key provided in _can_record_outputs directly. I cannot find any occurence of "image_hidden_states" as a direct key in any _can_record_outputs so should be fine here - and there was not any exception for this case either before
| args_with_config_defaults = [ | ||
| "use_cache", | ||
| "vision_feature_layer", | ||
| "vision_feature_select_strategy", | ||
| "vision_aspect_ratio", | ||
| ] |
There was a problem hiding this comment.
This is now merge_with_config_defaults? Do we always apply both decorators now? E.g. llama needs it for use_cache no?
|
No, none at all! (except obviously how each model is handling the outputs its submodels may capture) |
…config_defaults` + ensure correctness (huggingface#43862) * start * switch imports everywhere * simplify * fix * latest model * add test * improve test * fix * fix * fix * oupsi * doc * better test * fix a few * fix * more elegant fix * finalize fixing output capturing layers * fix all finally * and the last nit * damn the test value was plainly wrong
3 participants
What does this PR do?
As per the title.
check_model_inputsis becoming very complex and doing more than what it should. Let's separate into 2 clear decorators:capture_outputs: everything related to capture outputsmerge_with_config_defaults: everything related to handling of somekwargsAlso, add a test to ensure that
capture_outputsis not called several time in a chain wrongly for the same output, and fix affected models.This is very important for correctness and needs a proper test to ensure it.