Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix ci: account for huggingface transformers changes #781

Merged
merged 7 commits into from
Feb 12, 2024

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Feb 12, 2024

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™


This PR fixes the test_huggingface_integration test. Check the inline comment for an explanation.

Closes #768

@mattseddon mattseddon force-pushed the check-huggingface branch 2 times, most recently from 3717370 to 36e0d31 Compare February 12, 2024 02:05
@mattseddon mattseddon changed the title check huggingface failure against local fix ci: account for total_flos being added by huggingface transformers Feb 12, 2024
@mattseddon mattseddon force-pushed the check-huggingface branch 3 times, most recently from b0cb886 to 522bbd8 Compare February 12, 2024 02:30
@mattseddon mattseddon changed the title fix ci: account for total_flos being added by huggingface transformers fix ci: account for huggingface transformers changes Feb 12, 2024
@mattseddon mattseddon self-assigned this Feb 12, 2024
@mattseddon mattseddon force-pushed the check-huggingface branch 5 times, most recently from ff721e7 to ad04ca4 Compare February 12, 2024 04:12
@mattseddon
Copy link
Member Author

My current theory is that a second instance of DVCLiveCallback is being attached to the Trainer.

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2024

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (8406920) 88.79% compared to head (27e38cb) 95.55%.
Report is 16 commits behind head on main.

Files Patch % Lines
src/dvclive/fabric.py 82.66% 10 Missing and 3 partials ⚠️
tests/frameworks/test_fabric.py 83.33% 6 Missing ⚠️
src/dvclive/lightning.py 70.58% 3 Missing and 2 partials ⚠️
src/dvclive/utils.py 80.76% 4 Missing and 1 partial ⚠️
tests/frameworks/test_lightning.py 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #781      +/-   ##
==========================================
+ Coverage   88.79%   95.55%   +6.75%     
==========================================
  Files          53       55       +2     
  Lines        3357     3532     +175     
  Branches      294      314      +20     
==========================================
+ Hits         2981     3375     +394     
+ Misses        337      110     -227     
- Partials       39       47       +8     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@@ -101,6 +101,7 @@ def args():
evaluation_strategy="epoch",
num_train_epochs=2,
save_strategy="epoch",
report_to="none",
Copy link
Member Author

Choose a reason for hiding this comment

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

Docs are here. Having this set to the default of "all" seems to create a second instance of the DVCLiveCallback which causes duplicate entries to be logged in all of the metric files.

@dberenbaum is this something we need to be aware of for #740?

Copy link
Member Author

Choose a reason for hiding this comment

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

total_flos was also added as a metric before report_to is set to "none" (original failure)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @mattseddon! Looks like it's causeg by testing the old callback that was part of dvclive along with transformers>=4.36.0, which auto reports using the new callback that's part of transformers. I will take a look for #740 and ask you to review.

@mattseddon mattseddon marked this pull request as ready for review February 12, 2024 07:17
@mattseddon mattseddon merged commit 3d70888 into main Feb 12, 2024
14 checks passed
@mattseddon mattseddon deleted the check-huggingface branch February 12, 2024 18:54
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.

Test falling for Huggingface
3 participants