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 tests after huggingface_hub 0.24 #32054

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Fix tests after huggingface_hub 0.24 #32054

merged 3 commits into from
Jul 19, 2024

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Jul 18, 2024

See discussion on slack (private).

In latest huggingface_hub update (0.24.0!), some logic has been added to avoid empty commits. This mechanism should prevent bloating git history when no changes have been detected. This change broke 2 tests in transformers's CI which should be fixed by this PR (previously tested on main...ci-test-huggingface-hub-v0.24.0.rc0). Note: when an empty commit is prevented, a warning message is logged to the user. This is what we are testing in this PR.

cc @ydshieh with whom we discussed this

EDIT: tests are failing since CI is still running on huggingface_hub==0.23.5. I guess rerunning them tomorrow should be good?

@Wauplin Wauplin requested review from ydshieh and a team July 18, 2024 12:29
@Wauplin Wauplin changed the title Fix test after 0.24 Fix tests after huggingface_hub 0.24 Jul 18, 2024
self.assertIn(f"Training in progress, step {i}", commits)
nb_expected_commits = len(range(5, max_steps, 5))

self.assertGreaterEqual(nb_empty_commits + nb_epoch_commits, nb_expected_commits)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you leave a short comment why is it >=?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in ff09978

Copy link
Collaborator

@ydshieh ydshieh 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 taking care of this

@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.

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 18, 2024

Let me check the version

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 18, 2024

It is huggingface-hub==0.23.5 used

@Wauplin
Copy link
Contributor Author

Wauplin commented Jul 18, 2024

It is huggingface-hub==0.23.5 used

Yes I think we need to wait for tomorrow (building docker image once a day right?)

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 18, 2024

yes! although we can build manually but it's a bit more involved

Copy link
Collaborator

@amyeroberts amyeroberts 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!

Bonus points for the clear comments that really helped provide context to the tests ❤️

@Wauplin
Copy link
Contributor Author

Wauplin commented Jul 18, 2024

I like bonus points 😍 (though it was thanks to @ydshieh's request 😄)

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 18, 2024

I am a lazy programmer so I ask a lot of clear comments 😅

@amyeroberts amyeroberts merged commit 0fdea86 into main Jul 19, 2024
20 checks passed
@amyeroberts amyeroberts deleted the fix-test-after-0.24 branch July 19, 2024 18:32
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