Change a logic in pipeline test regarding TF #20710
Conversation
| with self.assertRaises(Exception): | ||
| outputs = summarizer("This " * 1000) | ||
| # For TF models, if the weights are initialized in GPU context, we won't get expected index error from | ||
| # the embedding layer. |
There was a problem hiding this comment.
This is necessary if we want to use tiny models from the Hub for pipeline tests.
|
The documentation is not available anymore as the PR was closed or merged. |
| if not ( | ||
| isinstance(model, TFPreTrainedModel) | ||
| and get_gpu_count() > 0 | ||
| and len(summarizer.model.trainable_weights) > 0 |
There was a problem hiding this comment.
At the moment on main, this condition len(summarizer.model.trainable_weights) is False, as the model is not obtained from from_pretrained but by model_class(config) which won't create any TF weight at this point.
There was a problem hiding this comment.
So should the model be properly created with weights (guessing it needs a forward pass on dummy inputs)?
There was a problem hiding this comment.
@sgugger On current main, there is no test failure around what I mentioned in this PR. We don't get the TF weights at this line, and they will be created inside pipeline forward (which is being in CPU context, despite we have GPU), and we get the expected exception --> not test failure.
This PR is just to make the necessary changes so my WIP PR could be merged without failure, where len(summarizer.model.trainable_weights) > 0 will become True --> and we need to skip in this case.
There was a problem hiding this comment.
PR title is somehow misleading though, sorry.
There was a problem hiding this comment.
I still don't get why it's useful to support a model with no weights here. The fix is to make sure they have weights.
There was a problem hiding this comment.
Let me explain:
Current main
- pipeline tests use tiny models created dynamically
- they are created using
model_class(config) - they will have different weights each time the tests are launched
- (a major part of the) tests don't test the outputs against expected values - I think the tests just make sure the pipelines could run + some other checks (but not the outputs)
- if I make them having weights before pipeline forward, then the current CI will fail without this PR on GPU
- they are created using
This PR:
- It doesn't
support a model with no weights- When a TF model having weights + on GPU --> it skips the check of
exception should be given
- When a TF model having weights + on GPU --> it skips the check of
ydshieh
changed the title
sgugger
left a comment
sgugger
left a comment
There was a problem hiding this comment.
ok, thanks for the explanations!
|
cc @gante to this one - he's been working on a way to check for invalid indices even for embedding layers on GPU |
* Fix the pipeline test regarding TF * Fix the pipeline test regarding TF * update comment Co-authored-by: ydshieh <[email protected]>
What does this PR do?
model_class(config), and for TF models, the weights are not created at this point. Then we set the device (which turns to be CPU instead of GPU!), and the wegiths are created in CPU context --> we get the expected exceptions.from_pretrainedIn order to use the tiny models from the Hub without any pipeline test failure, we will have to skip this check under the above described situation.
From @Rocketknight1