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

Restore original module type on first use #22

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

Conversation

Y-T-G
Copy link
Contributor

@Y-T-G Y-T-G commented Jan 17, 2025

@glenn-jocher Assigns the original module to the variable on the first use, so that there's no LazyLoader being middle-man anymore. It uses the original module directly on subsequent use.

In [2]: from autoimport import lazy

In [3]: with lazy():
   ...:    from torchvision import ops
   ...:

In [4]: type(ops)
Out[4]: autoimport.main.LazyLoader

In [5]: ops.nms
Out[5]: <function torchvision.ops.boxes.nms(boxes: torch.Tensor, scores: torch.Tensor, iou_threshold: float) -> torch.Tensor>

In [6]: ops.nms
Out[6]: <function torchvision.ops.boxes.nms(boxes: torch.Tensor, scores: torch.Tensor, iou_threshold: float) -> torch.Tensor>

In [7]: type(ops)
Out[7]: module

In [10]: ops
Out[10]: <module 'torchvision.ops' from '/opt/conda/lib/python3.11/site-packages/torchvision/ops/__init__.py'>

This will work for global import. It won't work for local import, because in local import the module is assigned to locals(), but locals() is not passed into lazy_import(). Only globals() is passed. Even though there's an argument for locals, it's not passed. It's always None. So it means it won't work inside test functions.

We could modify the context manager to pass locals() manually:
with lazy(locals()):

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Improves the LazyLoader for better handling of lazy imports, enabling more robust and reliable module loading. 🚀

📊 Key Changes

  • Enhanced LazyLoader class to track the originating module's name (from_name) and optionally receive the globals dictionary to manage lazy imports better.
  • Updated _load_module to replace the global reference with the actual loaded module if the global matches the lazy instance.
  • Modified lazy_import function to pass globals and from_name when initializing LazyLoader.

🎯 Purpose & Impact

  • Purpose: These changes aim to make lazy imports more reliable and handle edge cases where global references to modules were incomplete or incorrect during lazy loading.
  • Impact:
    • Developers benefit from improved module-loading behavior, reducing the chances of runtime errors associated with incomplete lazy import setups.
    • Makes the lazy_import utility more functional and robust, especially when used in complex projects or dynamic import scenarios. 💡

@UltralyticsAssistant UltralyticsAssistant added the enhancement New feature or request label Jan 17, 2025
@UltralyticsAssistant
Copy link
Member

👋 Hello @Y-T-G, thank you for submitting an ultralytics/autoimport 🚀 PR! Your contribution to improving the repository is greatly appreciated. To ensure a smooth integration of your work, please review the checklist below:

  • Define a Purpose: From your description, it seems like the purpose of this PR is to enhance the lazy import utility. This is great! Please make sure that the purpose is detailed and links to any relevant issues. Clear, concise commit messages are also encouraged.
  • Synchronize with Source: Ensure your PR is synchronized with the ultralytics/autoimport main branch. You can do this by clicking the 'Update branch' button or running git pull and git merge main locally.
  • Ensure CI Checks Pass: Verify that all Ultralytics Continuous Integration (CI) checks pass. If you encounter any failures, please address them before proceeding.
  • Update Documentation: If your updates affect usage, provide updates to the documentation so users can understand the changes.
  • Add Tests: If applicable, ensure that your modifications include appropriate test coverage. It's highly recommended to confirm that all tests are passing.
  • Sign the CLA: If this is your first Ultralytics PR, please sign our Contributor License Agreement (CLA) by commenting "I have read the CLA Document and I sign the CLA".
  • Minimize Changes: Limit PR changes to just the areas needed for this fix or feature. 🚀

For more detailed guidance on our contribution process, refer to the Contributing Guide. If your changes are addressing a bug, a minimum reproducible example (MRE) demonstrating the issue before and after your updates would help us evaluate and review your PR more effectively.

🎉 Thank you again for your contribution! This is an automated response, but rest assured an Ultralytics engineer will review your submission and provide additional feedback or approval soon. If you have any questions in the meantime, feel free to comment below. 🚀✨

Signed-off-by: Mohammed Yasin <[email protected]>
@Y-T-G Y-T-G changed the title Restore original module on first use Restore original module type on first use Jan 17, 2025
@glenn-jocher
Copy link
Member

@Y-T-G oh this is a really innovative solution!

@glenn-jocher
Copy link
Member

All the tests seem to be passing correctly.

@Y-T-G
Copy link
Contributor Author

Y-T-G commented Jan 19, 2025

@glenn-jocher It shouldn't affect the local import functionality. Just that it can't restore or swap the module to the original one for local import.

@Y-T-G
Copy link
Contributor Author

Y-T-G commented Jan 19, 2025

Global Test

In [2]: def test_global():
   ...:     from autoimport import lazy
   ...:     import sys
   ...:     global torch
   ...:     with lazy():
   ...:         import torch
   ...:     print("Type before use:", type(torch))
   ...:
   ...:     torch.nn
   ...:     print("Still LazyLoader?:", hasattr(torch, "_module"))
   ...:     print("torch pointing to the actual module?", torch is sys.modules["torch"])
   ...:     print("Type after use:", type(torch))
   ...:

In [3]: test_global()
Type before use: <class 'autoimport.main.LazyLoader'>
Still LazyLoader?: False
torch pointing to the actual module? True
Type after use: <class 'module'>

The actual reference gets swapped so torch is now pointing directly to the module instance of torch, not just faking it.

Local Test

In [2]: def test_local():
   ...:     from autoimport import lazy
   ...:     import sys
   ...:     with lazy():
   ...:         import torch
   ...:     print("Type before use:", type(torch))
   ...:
   ...:     torch.nn
   ...:     print("Still LazyLoader?:", hasattr(torch, "_module"))
   ...:     print("torch pointing to the actual module?", torch is sys.modules["torch"])
   ...:     print("Type after use:", type(torch))
   ...:

In [3]: test_local()
Type before use: <class 'autoimport.main.LazyLoader'>

Still LazyLoader?: True
torch pointing to the actual module? False
Type after use: <class 'module'>

LazyLoader instance is faking being torch, when in reality, it's still LazyLoader. torch is still pointing to the LazyLoader instance.

@glenn-jocher
Copy link
Member

@Y-T-G oh I understand, so we need to predefine the packages as global before importing them with the context manager here to get the full benefit of using the actual package instead of the passthrough LazyLoader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants