idefics2 enable_input_require_grads not aligned with disable_input_re…

[sywangyi]

…quire_grads

make peft+idefics2 checkpoints disable fail

error like
AttributeError: 'GaudiIdefics2ForConditionalGeneration' object has no attribute '_require_grads_hook'

• vision models: @amyeroberts

[amyeroberts]

Thanks for opening this PR and fixing!

For my own understanding of the changes - it seems there's two changes happening here: one which ensures proper disabling of hooks, the other which adds peft-related logic to the tests. Is the latter necessary to catch the former?

[sywangyi]

Thanks for opening this PR and fixing!

For my own understanding of the changes - it seems there's two changes happening here: one which ensures proper disabling of hooks, the other which adds peft-related logic to the tests. Is the latter necessary to catch the former?

yes, the latter is used to catch the former.

[amyeroberts]

@sywangyi Could you explain a bit why? In the case of the test, _hf_peft_config_loaded=False seems to be testing the previous logic (and I assume passing on main). The reason I ask is that it's good we find a way to catch possible issues but the relation to PEFT isn't obvious to me -- this indicates that either the original test isn't properly capturing what we thought or there's a different behaviour in the case of peft which e might need to address.

In particular, I think that we probably want a peft-specific test to be separated out of the current test to make sure what's being tested and why is clear

[sywangyi]

see logic in https://github.com/huggingface/transformers/blob/main/src/transformers/modeling_utils.py#L2380 and https://github.com/huggingface/transformers/blob/main/src/transformers/modeling_utils.py#L2429 this path (_hf_peft_config_loaded=True) is not tested. so I set _hf_peft_config_loaded to False and True to test both path.

[amyeroberts]

@sywangyi OK, I see. Hmmm -- we shouldn't really need to know about PEFT + trainer within the modeling utils but alas it's there. Could you split up the tests so that we have a separate one for the PEFT condition and resolve the failing tests? After than I think the PR will be good to go

[sywangyi]

I check the failure case
FAILED examples/tensorflow/test_tensorflow_examples.py::ExamplesTests::test_run_image_classification - ValueError: Task image-classification is not compatible with this dataset! Available tasks: []
======== 1 failed, 5 passed, 3 skipped, 8 warnings in 300.61s (0:05:00) ========

this case has nothing to do with the PR

[amyeroberts]

Thanks for fixing and iterating on this!