Skip to content

Fix some TF GPT-J CI testings#16454

Merged
ydshieh merged 9 commits intohuggingface:mainfrom
ydshieh:fix_tf_gptj_ci_testings
Mar 29, 2022
Merged

Fix some TF GPT-J CI testings#16454
ydshieh merged 9 commits intohuggingface:mainfrom
ydshieh:fix_tf_gptj_ci_testings

Conversation

@ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Mar 28, 2022

What does this PR do?

Fix some TF-GPT-J CI testing (scheduled)

  • test_mixed_precision: require some casting
  • test_saved_model_creation and test_saved_model_creation_extended: require shape_list instead of shape
  • test_model_from_pretrained: skip for now otherwise GPU OOM

With the changes this PR, only the following test fails: test_gptj_sample_max_time: for example

model.generate(input_ids, do_sample=False, max_time=MAX_TIME, max_length=256)

the PT gives a quite short generation sequence (say 19), while TF gives a sequence of length 256, and it takes much more time and therefore fails the tests.

I feel this remaining issue is better to be addressed in another PR.

@ydshieh
Copy link
Collaborator Author

ydshieh commented Mar 28, 2022

I need to check why

@unittest.skipIf(len(tf.config.list_physical_devices("GPU")) > 0, "skip testing on GPU for now to avoid GPU OOM.")

causes problems in other tests (torch, pipeline etc ...)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 28, 2022

The documentation is not available anymore as the PR was closed or merged.

@ydshieh ydshieh marked this pull request as draft March 28, 2022 17:28
@ydshieh ydshieh changed the title Fix tf gptj ci testings Fix some TF GPT-J CI testings Mar 28, 2022
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing!

Copy link
Contributor

@gante gante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for looking at these, @ydshieh 🙏

@gante
Copy link
Contributor

gante commented Mar 28, 2022

Regarding the max_time/generate test -- when I first reviewed the GPT-J PR, I had little understanding of the current state of TF generate. Now I can tell that this test makes no sense :D Contrarily to PT's generate, TF generate has no time-based stopping criteria, so it is natural that the test fails. I'd remove it.

@ydshieh
Copy link
Collaborator Author

ydshieh commented Mar 28, 2022

Regarding the max_time/generate test -- when I first reviewed the GPT-J PR, I had little understanding of the current state of TF generate. Now I can tell that this test makes no sense :D Contrarily to PT's generate, TF generate has no time-based stopping criteria, so it is natural that the test fails. I'd remove it.

OK, thank you for the feedback.

But just curious (off-topic): TF generate has no time-based stopping criteria --> do we plan to support this in the future ??

(not very sure, but I remembered before TF can generate short sequences too. And if TF can't stop earlier, it looks like a quite big drawback ..? Anyway, we shouldn't discuss this generation thing in this PR.)

@gante
Copy link
Contributor

gante commented Mar 28, 2022

But just curious (off-topic): TF generate has no time-based stopping criteria --> do we plan to support this in the future ??

(not very sure, but I remembered before TF can generate short sequences too. And if TF can't stop earlier, it looks like a quite big drawback ..? Anyway, we shouldn't discuss this generation thing in this PR.)

The plan we have for the refactoring does not mention extras like the stopping criteria, so I can only tell that it probably won't happen in the next 2-3 months :) We can generate short sentences with TF if we pass the max_length argument, where generate() generates up to max_length tokens.

@ydshieh ydshieh marked this pull request as ready for review March 28, 2022 21:01
@ydshieh ydshieh merged commit 86cff21 into huggingface:main Mar 29, 2022
@ydshieh ydshieh deleted the fix_tf_gptj_ci_testings branch March 29, 2022 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants