Repository navigation
Fix tie_weights skipping logic is not tied to model thread scope #44940
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
Closed
Qubitium
wants to merge
11
commits into
huggingface:main
from
Qubitium:fix-tie-weights-concurrency-bug
Closed
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
5b1b4aa
fix tie_weights skipping logic is not thread-safe
Qubitium 3a82e73
cleanup
Qubitium 2e7a151
better var name
Qubitium c8a7f55
sync name change
Qubitium 6dc7c5f
fix unit to load dummy model that requires tie_weights to execute
Qubitium 8d56ea1
Merge branch 'main' into fix-tie-weights-concurrency-bug
Qubitium ca30f00
Merge branch 'main' into fix-tie-weights-concurrency-bug
Qubitium 3750813
Merge branch 'main' into fix-tie-weights-concurrency-bug
Qubitium 5fae1f1
Merge branch 'main' into fix-tie-weights-concurrency-bug
Qubitium 8ef14ff
Merge branch 'main' into fix-tie-weights-concurrency-bug
Qubitium a690991
Merge branch 'main' into fix-tie-weights-concurrency-bug
Qubitium File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the functionality under this decarator. We can simply check the value set to the context var or something, instead of delaying to
modeling_utilsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Let me check if this is dosable to reduce the impact to modeling_utils and limit changes to the decorator as much as possbile. But I would still want remove the very ugly set
empty_funcmethod currently in place.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cyrilvallez I simulated some threaded scenarios and keeping the monkeypatch is still not threadsafe, even we keep the current
empty_func()version or switch it to a wrapper that reads the new contextvar state.Issue is the contextvar is thread local but monkeypatch itself is still a class method override. So two overlapping
no_tie_weights()calls in different threads can still race on read/write of the patched method.I think the correct design is to keep
no_tie_weights()as a scope marker only and make the real tie_weights() decide whether to skip. If we want this part of Transformers to behave cleanly under threads, I think we need to move away from the monkeypatch pattern and keep the state-based one instead.Having a single state var just for this purpose may be overkill and you have mentioned there are other parts that also need this type of state keeping so there is opportunity to have a more general
contextvarscop/state to store all the different loading state asbitsbut that should be another PR/bigger refractor that strips away all the monkeypatching during loading and use pure states/scope read/writes.