Fix test_configuration_tie in FlaxEncoderDecoderModelTest

[ydshieh]

What does this PR do?

Fix the test_configuration_tie in FlaxEncoderDecoderModelTest.

The test_configuration_tie in FlaxEncoderDecoderModelTest would fail, as shown in run_tests_flax. See below.

FlaxEncoderDecoderModel didn't have encoder and decoder - they are inside FlaxEncoderDecoderModule, and need an indirect way to access them.

However, it might be too much to add these just for a test without other interesting use case.
If we don't want to add this to FlaxEncoderDecoderModel, we could probably move the changes to be inside test_configuration_tie.

BTW, this test is @slow, so won't be run by CircleCI. I saw it is mentioned CircleCI does not run the slow tests, but github actions does every night! in How to contribute to transformers?.

So test_configuration_tie failed on github actions, and I am wondering when a slow test fails, what are the actions? Is it only for Hugging Face internal?
(I finally found the report under GitHub actions tab)

CircleCI test results

    def _check_configuration_tie(self, model):
>       assert id(model.decoder.config) == id(model.config.decoder)
E       AttributeError: 'FlaxEncoderDecoderModel' object has no attribute 'decoder'

[patrickvonplaten]

Thanks a lot for fixing the test! The implemented approach is clean and makes total sense. However, I'm not really in favor of exposing the encoder module this way. The reason is the following. In PyTorch callling model.encoder (of model = EncoderDecoderModel(...)) gives one the encoder class with all the weights and PreTrainedModel functions attached. However this would not be the case in Flax. model.encoder.params would not work since model.encoder is a Flax module not a model. It's not really possible to retrieve the Flax model actually in the Flax encoder decoder design IMO, so I would prefer to just not add such properties and rather change the config test.

@patil-suraj - could you take a look here as well?

[ydshieh]

I agree with @patrickvonplaten , especially the exposure here is only for the testing purpose.

By change the config test, do you mean we still want to keep the testing logic of test_configuration_tie, but getting the encoder/decoder modules inside it rather than exposing them in FlaxEncoderDecoderModel? (I am OK with this option.)

P.S. Currently, FlaxVisionEncoderDecoderModelTest doesn't have this test after our discussion in another thread.

#13359 (comment)

[patil-suraj]

Agree with @patrickvonplaten .

@ydshieh I think we could still keep the test. For this use case, we could use the bind method, which makes the flax module stateful so we could directly get the encoder/decoder.

module = model.module.bind(model.params)
enc_config = module.encoder.config
dec_config = module.decoder.config

WDYT @patrickvonplaten?

[ydshieh]

Agree with @patrickvonplaten .

@ydshieh I think we could still keep the test. For this use case, we could use the bind method, which makes the flax module stateful so we could directly get the encoder/decoder.

module = model.module.bind(model.params)
enc_config = module.encoder.config
dec_config = module.decoder.config

It works, thank you @patil-suraj .

https://app.circleci.com/pipelines/github/huggingface/transformers/29684/workflows/1392381e-9fbe-488d-9164-0bd4add6b393/jobs/299243

[patrickvonplaten]

I let you merge if you're happy with the PR @patil-suraj :-)

[patil-suraj]

LGTM now! Thanks a lot for fixing this @ydshieh !