Skip to content

Conversation

@NosimusAI
Copy link
Contributor

@NosimusAI NosimusAI commented Feb 16, 2025

What does this PR do?

Fixes # (issue)

To resolve the issue where the model is saved twice when using save_strategy="epoch", we need to prevent the redundant save at the end of training. The save triggered by the end of the last epoch is sufficient, so we skip the final save when the strategy is set to epoch.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@Rocketknight1
Copy link
Member

cc @muellerzr @SunMarc

Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

LGTM !

@NosimusAI
Copy link
Contributor Author

Hi, these tests are flicking. Can we restart pipeline?

@NosimusAI NosimusAI force-pushed the fix/second-save-on-steps-save branch from a03067b to 46a8f22 Compare February 18, 2025 22:48
@NosimusAI
Copy link
Contributor Author

NosimusAI commented Feb 19, 2025

@shethaadit @SunMarc can you merge? It says that 2 workflows awaiting approval, but they are completed.

@SunMarc SunMarc requested a review from muellerzr February 19, 2025 12:07
@SunMarc
Copy link
Member

SunMarc commented Feb 19, 2025

just waiting a last review

@SunMarc
Copy link
Member

SunMarc commented Feb 19, 2025

Could you also try to add a test to this, that would be nice

Copy link
Contributor

@muellerzr muellerzr 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 the fix, all green from me after we add a test please!

@HuggingFaceDocBuilderDev

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.

@NosimusAI
Copy link
Contributor Author

@SunMarc @muellerzr @shethaadit could you approve pls. Added unit test.

Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

Thanks a lot !

@SunMarc SunMarc merged commit effaef3 into huggingface:main Feb 20, 2025
21 checks passed
skshetry added a commit to treeverse/dvclive that referenced this pull request Apr 29, 2025
skshetry added a commit to treeverse/dvclive that referenced this pull request Apr 29, 2025
* ci: try to fix test-full

also use 3.12 to run full tests

* fix mypy errors

* hf: fix deprecated arguments in transformers.TrainingArguments

* tests: adjust assertions in test_huggingface_log_model

Due to huggingface/transformers#36219.
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.

7 participants