Load sub-configs from composite configs #34410
Conversation
|
Modular is trying to overwrite InstructBlip and remove Other failing tests have no relation to the current PR |
|
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. |
| @@ -110,6 +115,34 @@ def create_and_test_config_from_and_save_pretrained_subfolder(self): | |||
|
|
|||
| self.parent.assertEqual(config_second.to_dict(), config_first.to_dict()) | |||
|
|
|||
| def create_and_test_config_from_and_save_pretrained_composite(self): | |||
| config = self.config_class(**self.inputs_dict) | |||
There was a problem hiding this comment.
okay, as long as the attributes here are different than default we are testing good stuff
| sub_configs = ["text_config", "qformer_config", "vision_config"] | ||
| text_config_class = "AutoConfig" | ||
| qformer_config_class = "InstructBlipVideoQFormerConfig" | ||
| vision_config_class = "InstructBlipVideoVisionConfig" |
There was a problem hiding this comment.
Hi! Super nice work 🔥 I was wondering about this, for composite modular-based new models, could we add something in the converter to enforce this structure?
There was a problem hiding this comment.
Sounds good having it automated, I'll try to enable it through this PR if I can find a way. But I am wondering how would we automatically know which class does each sub_config map to if no info is given except for the code. Hmm, should we parse the code body then
There was a problem hiding this comment.
maybe with AST? we have typed args for the configs that inform of which config goes where, too
There was a problem hiding this comment.
yes, seems to be doable if you enforce config docstring to contain the sub-config class in type-hinting. Maybe we'll add it as the next PR so we can take time for making modular better. Also I've another PR on improving modular in general, might as well go there as a new feature
There was a problem hiding this comment.
cc @Cyrilvallez since you are working closely with modular. Do you think this is smth we can add in your PR or maybe subsequent PRs?
There was a problem hiding this comment.
Hey, sorry for the late reply! This is definitely do-able - it would require parsing the __init__ to check subconfig-specific assignments and add them as class attributes in the body.
However, it breaks a bit the "no magic" policy of the modular, as it means we automatically add stuff based on simple naming conventions.
zucchini-nlp
force-pushed
the
composite-configs
branch
from
a8dac97 to
b53833b
Compare
| for key in config.sub_configs.keys(): | ||
| sub_config = getattr(config, key) | ||
| curr_attn_implementation = ( | ||
| requested_attn_implementation | ||
| if not isinstance(requested_attn_implementation, dict) | ||
| else requested_attn_implementation.get(key, None) | ||
| ) | ||
| sub_config._attn_implementation_internal = curr_attn_implementation |
…SAM model tests - Added attributes for sub_configs as per PR huggingface#34410. - Enabled tests for configs, ensuring the composite model (SAM) has several sub-configs in the main config. - Added class attribute _is_composite=True to the tester class - test_sdpa_can_dispatch_composite_models added
* save/load sub-configs * nit forgot these * fix copies * move test to common * use dict for sub-configs * add load-save-laod test * clean up modeling check * oops this are correct keys * fix some tests, missed some composite configs * this model was missed
* feat: add support for sdpa and gradient checkpointing * fix: ruff format * fix: config sdpa * fix: sdpa layer naming convention * fix: update test_eager_matches_sdpa_inference to handle vision_hidden_states * test: skip incompatible tests and fix loading issue with sdpa - Updated tests to skip cases flash and dynamic compile. - Minor adjustment to ensure correct loading of model with sdpa for dispatch test. * style: apply Ruff formatting * ruff fix again after rebase * [run-slow] sam * [run-slow] sam * refactor: Address review comments and improve sub-config handling in SAM model tests - Added attributes for sub_configs as per PR #34410. - Enabled tests for configs, ensuring the composite model (SAM) has several sub-configs in the main config. - Added class attribute _is_composite=True to the tester class - test_sdpa_can_dispatch_composite_models added * [run-slow] sam * style: ruff * [run-slow] sam * style: ruff again ... * [run-slow] sam
What does this PR do?
This PR removed overwritten
from_pretrainedin many configs that are part of a bigger config. Instead it moved the loading logic to the base config class throughbase_config_key. Additionally it adds a few more flags to the config, in a similar fashion to processors where each sub-config's class name is recorded. Currently those are needed only for tests to load the sub-config directly and compare withbase_config'svalueWeird thing is that
config._is_compositionis not an indicator of whether the config is composite or not. Several configs were missing the flag and operating good, because the flag is used only to separate out configs that can be init without params and those that require params. Those that require are flagged asis_composition, see more in #25237 description and in #7943 which introduced the flag.I personally see only a few models that require input params (other configs) which are indeed composite, and all others don't need the flag so I removed it. But still I think the naming is quite misleading so maybe we should rename that in long-term or even remove that and find a better way to save configs when default params are overwritten.