Skip to content

Adding support for bf16_full_eval#610

Merged
regisss merged 3 commits into
huggingface:mainfrom
bhargaveede:dev/t5_bf16_full_eval
Jan 3, 2024
Merged

Adding support for bf16_full_eval#610
regisss merged 3 commits into
huggingface:mainfrom
bhargaveede:dev/t5_bf16_full_eval

Conversation

@bhargaveede
Copy link
Copy Markdown

What does this PR do?

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

@bhargaveede bhargaveede requested a review from regisss as a code owner December 21, 2023 09:29
@bhargaveede bhargaveede added the run-test Run CI for PRs from external contributors label Dec 21, 2023
@bhargaveede bhargaveede requested a review from vivekgoe December 21, 2023 09:29
@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

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.

@bhargaveede
Copy link
Copy Markdown
Author

@regisss I have enabled bf16_full_eval and verified the same with t5 inference.
Can you review it?

Comment thread optimum/habana/transformers/trainer.py
Comment thread optimum/habana/transformers/trainer.py
@vivekgoe
Copy link
Copy Markdown
Collaborator

@bhargaveede please also update (1) summarization README file to include this extra argument for t5-3b prediction example, (2) performance targets in test for both G1 and G2.

@bhargaveede bhargaveede added run-test Run CI for PRs from external contributors and removed run-test Run CI for PRs from external contributors labels Dec 27, 2023
@bhargaveede
Copy link
Copy Markdown
Author

bhargaveede commented Dec 27, 2023

@regisss Can your review and merge this?

@bhargaveede
Copy link
Copy Markdown
Author

@regisss Can we merge this?

@vivekgoe
Copy link
Copy Markdown
Collaborator

vivekgoe commented Jan 2, 2024

Looks good to me. We can merge it once @regisss also reviews.

@regisss
Copy link
Copy Markdown
Collaborator

regisss commented Jan 2, 2024

I'll review it this week 👍

Comment on lines 17 to +26
("facebook/bart-large-cnn", "Habana/bart", 4.691, 26.0688, 2, 1),
("t5-3b", "Habana/t5", 2.28, 21.56, 2, 1),
("t5-3b", "Habana/t5", 2.88, 21.56, 2, 1),
],
}
else:
# Gaudi1 CI baselines
MODELS_TO_TEST = {
"bf16": [
("facebook/bart-large-cnn", "Habana/bart", 2.588, 26.0688, 2, 1),
("t5-3b", "Habana/t5", 0.585, 21.72, 2, 1),
("t5-3b", "Habana/t5", 0.98, 21.56, 2, 1),
Copy link
Copy Markdown
Collaborator

@regisss regisss Jan 2, 2024

Choose a reason for hiding this comment

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

For Gaudi1, I get a RougeLsum of 21.3831 and a throughput of 1.005. It doesn't matter much since the test passes (no need to update the numbers).
For Gaudi2 however, runs are not reproducible it seems. I get different RougeLsum from one run to another, is it something you also observed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I didn't get different RougeLsum. When I added perf numbers, I ran the test twice to check and I was getting same RogueLsum. Let me check again.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Interesting, did you run it with Synapse 1.13?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I could see the variation. However, I'm seeing variation on v1.9-release too for the test "test_run_summarization_t5-small_multi_card". Can you confirm if it's same on your end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I cannot run multi-card tests on my Gaudi2 instance at the moment but if you observed the same behavior for "test_run_summarization_t5-small_multi_card" it means that this "issue" was already there before.
Anyway, tests still pass so I'm going to merge it and I'll investigate that later.

@regisss regisss merged commit dd02a7b into huggingface:main Jan 3, 2024
jychen21 pushed a commit to jychen21/optimum-habana that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-test Run CI for PRs from external contributors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants