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

change version #29097

Merged
merged 9 commits into from
Feb 19, 2024
Merged

change version #29097

merged 9 commits into from
Feb 19, 2024

Conversation

ArthurZucker
Copy link
Collaborator

What does this PR do?

Try to change cached version

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

@ArthurZucker ArthurZucker marked this pull request as ready for review February 19, 2024 11:35
@ydshieh
Copy link
Collaborator

ydshieh commented Feb 19, 2024

We still have failing for test_save_load_low_cpu_mem_usage which are already on main.

On this PR, the accelerate is 0.27.2 and on main, the CI uses accelerate main. So I don't think the change of this PR (on the accelerate part address the issue)

@amyeroberts mentions she has already some work on solving test_save_load_low_cpu_mem_usage .

@ArthurZucker mentions he don't want to use accelerate main on our CI.

So let's keep this PR as it is despite there is test_save_load_low_cpu_mem_usage failing, if @amyeroberts agrees.

cc @muellerzr

@ydshieh
Copy link
Collaborator

ydshieh commented Feb 19, 2024

p.s. test_beam_search_low_memory also happens for which I don't see on main CI.

@amyeroberts
Copy link
Collaborator

amyeroberts commented Feb 19, 2024

@ydshieh Do we know the original reasons for using accelerate main rather than the stable release in the CI?

@amyeroberts
Copy link
Collaborator

@ydshieh Just to confirm - changing the accelerate version here would mean that test_save_load_low_cpu_mem_usage starts to fail despite the fixes on main. Or that there's still failures on main?

@ydshieh
Copy link
Collaborator

ydshieh commented Feb 19, 2024

@amyeroberts I didn't know the fix of test_save_load_low_cpu_mem_usage is already on main.

However, that fix is merged 4 days ago, right? But last night's run

https://app.circleci.com/pipelines/github/huggingface/transformers/84737/workflows/c2fc5312-c0bf-4620-b523-74c1a99b0d1b/jobs/1096153/tests

having those failures.

So it's not this PR introducing this again. (i.e. those failures are still on main after the fix).

@ydshieh
Copy link
Collaborator

ydshieh commented Feb 19, 2024

I propose to revert the changes regarding accelerate in this PR (as it didn't fix the issue).

If eventually everyone agrees with @ArthurZucker, we can open another PR to use released accelerate for CI - but this is not the scope of this PR (and I don't want to mix the 2 different stuff in a single PR).

@amyeroberts
Copy link
Collaborator

@ydshieh OK, what I suggest for the moment is skipping all the models where this fails at the moment.

Separate from this PR, I think there's a issue with our test fetcher. There's been quite a few PRs recently where the whole CI has been green and then after merging into main, another PR which touches related (but different) code suddenly starts to have CI failures. These tests are one example. Two other cases would be:

@amyeroberts
Copy link
Collaborator

If eventually everyone agrees with @ArthurZucker, we can open another PR to use released accelerate for CI - but this is not the scope of this PR (and I don't want to mix the 2 different stuff in a single PR).

I'm pro splitting and pro having stable accelerate for PR's CI and main accelerate on a nightly run

@ydshieh
Copy link
Collaborator

ydshieh commented Feb 19, 2024

Will take a look over the test fetcher. But remember that test fetcher have some design decision (by great @sgugger ) to balance the coverage as well as the speed of CI, so the situation described is not surprising (at least to me since I faced it a few times already and some fix were already done).

(One related PR but not exact is #28816)

@ydshieh
Copy link
Collaborator

ydshieh commented Feb 19, 2024

Regarding skipping all the models where this fails at the moment, I will leave it to anyone that is so motivated to beat me, otherwise I can do it in about 9 hours.

For now, I will just wait the CI of this PR and merge it.

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.

LGTM thanks

@ydshieh ydshieh merged commit b2724d7 into main Feb 19, 2024
18 of 22 checks passed
@ydshieh ydshieh deleted the pin-pytest branch February 19, 2024 14:08
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Feb 19, 2024
* change version

* nuke

* this doesn't make sense

* update some requirements.py

* revert + no main

* nits

* change cache number

* more pin

* revert

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
@ArthurZucker
Copy link
Collaborator Author

Thanks both 🤗

itazap pushed a commit that referenced this pull request May 14, 2024
* change version

* nuke

* this doesn't make sense

* update some requirements.py

* revert + no main

* nits

* change cache number

* more pin

* revert

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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