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 recursion error when setting tp_wrapped_module #122 #123

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ar-Kareem
Copy link

To fix the error mentioned in #122
This happens whenever the attribute tp_wrapped_module is changed (for example inside LoRA or other PEFT methods)

I am not 100% sure this works as I have not started training yet but it has certainly made my example in #122 work as expected.

@BlackSamorez
Copy link
Owner

Fix the style, please. black . && isort .

@Ar-Kareem
Copy link
Author

Fixed styling, although I'm not sure why the tests are failing.

@BlackSamorez
Copy link
Owner

The failing tests are not related to this issue. The Falcon-40B related tests fail to load the model off the web. I'll look into it. I'll also look into what you've done in more detail because it's always complicated with getattr/setattr.

@Ar-Kareem
Copy link
Author

Ar-Kareem commented Oct 3, 2023

Sure, all I did is made sure to re-add tp_wrapped_module to __dict__, this is necessary for any use case where you need to do
wrapper.tp_wrapped_module = some_module (which is what I wanted LoRA to do)
And when that's executed, obviously the set_attr for nn.Module will be called and specifically the below line will clear tp_wrapped_module from the __dict__
module.py#L1726
Thus the get_attr for the wrapper will cause a recursion error as doing self.tp_wrapped_module will recursively call get_attr

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.

None yet

2 participants