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

Nail in edge case of torch dtype being overriden permantly in the case of an error #35845

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

muellerzr
Copy link
Contributor

@muellerzr muellerzr commented Jan 22, 2025

What does this PR do?

@BenjaminBossan and I have been trying to hunt down an issue causing some headache over the last few days, as to why some tests were failing if done in specific instances.

What we cornered it to, is if a user:

  1. Inits a model (either from_pretrained or _from_config), with a dtype other than the torch default dtype
  2. And then inside of a very specific window of time the model loading fails before we have a chance to restore the old torch dtype

What will occur is the torch dtype will permantly change, which is why some users may "discover" this issue and it goes away after a full restart (a likely scenario that could happen). What we currently do doesn't catch if we have an exception, this does, and is used as an additive

This PR introduces a safety decorator called restore_default_torch_dtype which will restore whatever torch's original dtype was prior to calling the function (since we restore it anyways at the end).

This PR also adds a new test_injection function. The aim is for this function to be mocked at critical points where we need testing, and we cannot easily get to it (e.g. in this case, there was only one function we could mock in from_pretrained and zero functions we could mock in _from_config to make sure this fix worked!)

Fixes # (issue)

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.

@SunMarc @BenjaminBossan @ArthurZucker

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.

1 participant