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

feat: ensure the indexability of dynamically imported packages #15423

Merged
merged 1 commit into from
Apr 6, 2024
Merged

feat: ensure the indexability of dynamically imported packages #15423

merged 1 commit into from
Apr 6, 2024

Conversation

storyicon
Copy link
Contributor

We should ensure the indexing of dynamically imported packages to prevent them from being completely anonymous, which is crucial for some scenarios that require dynamic patch extension methods or variables.

@storyicon storyicon requested a review from AUTOMATIC1111 as a code owner April 1, 2024 09:15
@AUTOMATIC1111
Copy link
Owner

This looks reasonable but can you please give some examples of how this is useful (or how not doing this is harmful)?

@storyicon
Copy link
Contributor Author

The reason for performing this operation on the extension is similar to what modules/sd_hijack* do. It allows more advanced developers to non-intrusively modify variables or methods within extensions that they do not control, typically to address adaptation or some tricky issues. Intrusive changes are difficult to maintain when the extension is updated.

I understand that this might not matter to most users who run webui in client form, but it becomes highly beneficial when maintaining webui in service form or when there are interactions between extensions under development.

@AUTOMATIC1111
Copy link
Owner

Any code examples?

@storyicon
Copy link
Contributor Author

storyicon commented Apr 2, 2024

To illustrate with a practical scenario:

For instance, the inpaint-anything extension uses the sam_dict variable to transfer data across multiple steps. However, in reality, this poses certain issues for scenarios involving separation of frontend and backend, or instances shared among multiple users. Currently, there is no way to access the sam_dict variable without modifying the extension's code.

https://github.com/Uminosachi/sd-webui-inpaint-anything/blob/main/scripts/inpaint_anything.py#L81

If this change is incorporated, advanced developers will be able to patch variables by accessing the corresponding module through sys.modules.

There are many similar scenarios. In ComfyUI, we have applied similar non-invasive patches to numerous custom nodes, which is predicated on https://github.com/comfyanonymous/ComfyUI/blob/master/nodes.py#L1888, but progress has been impeded in the webui context.

In fact, I can non-invasively alter the behavior of webui by registering the module with sys.modules using the following approach:

def patch_load_module():
   origin_impl = modules.script_loading.load_module
   def load_module(...):
      ...
   modules.script_loading.load_module = load_module

However, from a standardization perspective, allowing a package to be unindexable after dynamic import does not seem like a good idea. I believe this could be helpful for other developers.

@AUTOMATIC1111
Copy link
Owner

Will it be with this added that script named sys.py will fuck up the sys import for other code?

@storyicon
Copy link
Contributor Author

This is an interesting question, and answering it requires two premises:

[1] Referring to the implementation of importlib.util.spec_from_file_location, when the path is relative, it always searches for the package based on the folder os.getcwd().

This means that when the user calls load_module, they always need to pass in an absolute path or a relative path based on the working directory.

[2] The working directory is by default considered one of the Python module search paths.

From [1], we know that if we want to break the import of the requests library through load_module, then the working directory must have a requests.py, but from [2], if the working directory has requests.py, then there is no need to call load_module as the import of the requests library has already been broken by requests.py.

Therefore, this is a paradox, and I don't believe that load_module would break the import of standard libraries or third-party libraries.

@AUTOMATIC1111 AUTOMATIC1111 merged commit badb70d into AUTOMATIC1111:dev Apr 6, 2024
2 of 3 checks passed
@AUTOMATIC1111
Copy link
Owner

AUTOMATIC1111 commented Apr 6, 2024

This was registering scripts under full path in sys.modules, which seems wrong. I changed so that scripts are indexed as scripts.<name>, so that, for example, you can import scripts.lora_script.py and it would work; if you still need full path, it's in the new loaded_scripts dictionary.

@storyicon
Copy link
Contributor Author

Certainly, that is feasible as well. Thank you kindly for your efforts.

@fanchunmeng-98
Copy link

Does this fix cause no attribute 'text_cond' appear again?

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.

3 participants