-
Notifications
You must be signed in to change notification settings - Fork 6k
Allow remote code repo names to contain "." #11652
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
base: main
Are you sure you want to change the base?
Conversation
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.
Looks pretty good! Should we add a test for it here?
diffusers/tests/pipelines/test_pipelines.py
Line 1009 in c934720
class CustomPipelineTests(unittest.TestCase): |
@@ -154,12 +154,29 @@ def check_imports(filename): | |||
return get_relative_imports(filename) | |||
|
|||
|
|||
def get_class_in_module(class_name, module_path): | |||
def get_class_in_module(pretrained_model_name_or_path, class_name, module_path): |
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.
Could we do:
def get_class_in_module(class_name, module_path, pretrained_model_name_or_path=None):
This will ensure the change is not backward breaking.
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.
Thanks for the feedback, I've made the proposed change
To test this we need a repo that both has a dot in the name and has custom remote code. Currently I'm testing it on a private repo. @sayakpaul could you help by creating a new repo on |
Added a test for it in |
What does this PR do?
When the repo path for a model containing custom code contains
.
, the code fails to import it correctly because.
is interpreted by the Python import machinery as a module separator. This issue was reported in Transformers (huggingface/transformers#28919) and was fixed in huggingface/transformers#29175. The same issue exists in Diffusers, so this PR brings over that fix with minor changes to account for how Diffusers names the cache directory.Before submitting
Who can review?
@yiyixuxu @sayakpaul @asomoza