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

Accelerate bug in specific versions #1007

Merged
merged 3 commits into from
Aug 28, 2023

Conversation

BenjaminBossan
Copy link
Collaborator

In versions 0.20.* and 0.21.0 (latest), there was/is a bug in unwrap_model when called with keep_fp32_wrapper=False. This results in forward not working after unwrapping the model, which can affect skorch users who use AccelerateMixin.

The bug has been fixed in this PR:

huggingface/accelerate#1838

However, there was no release yet. Therefore, I exclude the specific accelerate versions with that bug. Using a version <0.20 or a future release should work.

To catch this type of issue sooner, I added a test that will trigger the bug.

In versions 0.20.* and 0.21.0 (latest), there was/is a bug in
unwrap_model with keep_fp32_wrapper=False. This results in forward not
working after unwrapping the model, which can affect skorch users who
use AccelerateMixin.

The bug has been fixed in this PR:

huggingface/accelerate#1838

However, there was no release yet. Therefore, I exclude the specific
accelerate versions with that bug. Using a version <0.20 or a later
release should work.

To catch this type of issue sooner, I added a test that will trigger the
bug.
@BenjaminBossan
Copy link
Collaborator Author

BenjaminBossan commented Aug 14, 2023

Argh:

E ImportError: accelerate>=0.20.3 is required for a normal functioning of this module, but found accelerate==0.19.0.
E Try: pip install transformers -U or pip install -e '.[dev]' if you're working with git main

So the transformers version that's installed fails with earlier accelerate versions. We could now start excluding transformers versions with this error message, but I think it's getting too much. My proposal would be to leave this PR open for now and wait for the next accelerate release with the bugfix, then require that accelerate version.

Update: The new accelerate v0.22 with the bugfix was released today, so tests are green now.

@BenjaminBossan BenjaminBossan changed the title Accelerate bug specific versions Accelerate bug in specific versions Aug 15, 2023
@@ -1,4 +1,4 @@
accelerate>=0.11.0
accelerate>=0.11.0,!=0.20.*,!=0.21.0
Copy link
Member

Choose a reason for hiding this comment

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

At this point, what do you think of going with >=0.22 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, it's best to be on the latest version anyway. And since accelerate is not a strict dependency, users can still use skorch with different accelerate versions if they want.

@thomasjpfan thomasjpfan merged commit b8185cf into master Aug 28, 2023
13 checks passed
@BenjaminBossan BenjaminBossan deleted the accelerate-bug-specific-versions branch August 30, 2023 10:35
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.

2 participants