Address duplicate import issue found in pypi-installed cuda-core packages - #2386
Address duplicate import issue found in pypi-installed cuda-core packages#2386acosmicflamingo wants to merge 5 commits into
Conversation
github-actions
Bot
added
the
cuda.core
|
@mdboom when you have the chance, can you please run |
|
/ok to test |
@mdboom, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
Hm, I've seen this E1 error appear before in another PR (not in this repo though, was |
|
/ok to test 0e12b48 |
Sorry, that was my bad. I have to provide the commit hash in my comment. |
|
It's all good; happy to finally know why that happens ;) Alright, test failed as I wanted: =================================== FAILURES ===================================
__________________________ test_typing_module_imports __________________________
def test_typing_module_imports():
"""
Importing cuda.core.system should not also import cuda.core.cuXX.system
"""
assert "cuda.core.system" in sys.modules
> assert f"cuda.core.cu{cuda_major}.system" not in sys.modules
E AssertionError: assert 'cuda.core.cu13.system' not in {'sys': <module 'sys' (built-in)>, 'builtins': <module 'builtins' (built-in)>, '_frozen_importlib': <module '_frozen_importlib' (frozen)>, '_imp': <module '_imp' (built-in)>, ...}
E + where {'sys': <module 'sys' (built-in)>, 'builtins': <module 'builtins' (built-in)>, '_frozen_importlib': <module '_frozen_importlib' (frozen)>, '_imp': <module '_imp' (built-in)>, ...} = sys.modules
tests/test_duplicate_imports.py:20: AssertionErrorAlthough I've struggled to find relative imports to change to absolute imports within cython files, I did find some in However, I'm struggling with how to handle diff --git a/cuda_core/cuda/core/__init__.py b/cuda_core/cuda/core/__init__.py
index dc6fefdffe..94f7959f5d 100644
--- a/cuda_core/cuda/core/__init__.py
+++ b/cuda_core/cuda/core/__init__.py
@@ -5,8 +5,11 @@
from cuda.core._version import __version__
+# TODO: remove this function altogether after wheel-variants become mainstream
def _import_versioned_module() -> None:
import importlib
+ import pathlib
+ import sys
from cuda import bindings
@@ -15,13 +18,13 @@ def _import_versioned_module() -> None:
raise ImportError("cuda.bindings 12.x or 13.x must be installed")
subdir = f"cu{cuda_major}"
- try:
- versioned_mod = importlib.import_module(f".{subdir}", __package__)
- # Import all symbols from the module
- globals().update(versioned_mod.__dict__)
- except ImportError:
- # This is not a wheel build, but a conda or local build, do nothing
- pass
+ versioned_dir = pathlib.Path(__file__).parent / subdir
+ # This is a wheel build with relevant modules in cuda/core/cu<cuda_major>
+ # directory. Let's add it to module path so imports work as expected.
+ # cuda.core.cu<cuda_major> is not meant to behave as a module itself, and
+ # does not belong in sys.modules
+ if versioned_dir.is_dir():
+ __path__.append(str(versioned_dir))
_import_versioned_module()One other approach could be manipulating diff --git a/cuda_core/cuda/core/__init__.py b/cuda_core/cuda/core/__init__.py
index dc6fefdffe..d536fd39a8 100644
--- a/cuda_core/cuda/core/__init__.py
+++ b/cuda_core/cuda/core/__init__.py
@@ -7,6 +7,7 @@ from cuda.core._version import __version__
def _import_versioned_module() -> None:
import importlib
+ import sys
from cuda import bindings
@@ -18,7 +19,13 @@ def _import_versioned_module() -> None:
try:
versioned_mod = importlib.import_module(f".{subdir}", __package__)
# Import all symbols from the module
+ core_mod_name = __name__
+ versioned_mod_name = versioned_mod.__name__
globals().update(versioned_mod.__dict__)
+ globals()["__name__"] = core_mod_name
+
+ if versioned_mod_name in sys.modules:
+ del sys.modules[versioned_mod_name]
except ImportError:
# This is not a wheel build, but a conda or local build, do nothing
pass |
|
@mdboom I opted for the cleaner approach, but I do have a solution that mutates |
acosmicflamingo
changed the title
|
/ok to test b3717d4 |
|
Whoops, getting this error: Don't know why I didn't see it locally, I'll push a fix in a few hours when I am in front of my keyboard |
|
Given the context of the test and issue, seems like it would be acceptable to just move the import to module level instead of emulate the same behavior within the specific test. @mdboom could you now please run |
|
/ok to test 3922d6d |
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
I understand the challenge with reproducing locally.
Unfortunately, the test currently passes without your changes to __init__.py applied, so we will need some better way to detect the error condition and confirm this change is actually working.
Sounds good! This bug is certainly a slippery one compared to others; I'll try and figure out how to go about it locally and not waste more CI resources. |
|
Trying to take a crack at this again. I am able to reproduce the issue locally using a pip-installed @mdboom mind running |
2 participants
Description
Fixes #2023
Right now, importing certain modules also imports a duplicate version where the cuda major version number resides in the name (e.g. importing
cuda.core.systemwill mean bothcuda.core.systemandcuda.core.cu13.systemare now insys.modules).It's also difficult to reproduce the problem locally. Running
pip install .was not giving me the file path I was expecting to write a failing test, which would allow me to assert that the solution I come up with actually works (assuming that the fix involves recompiling cython modules):Although the issue is exacerbated by relative imports, the underlying issue comes from how
cuda.core.__init__.pyhandles importing the modules from thecuda/core/cu13path, so using absolute imports everywhere will not get rid ofcuda.core.cu13fromsys.modules.The PR's approach is to simply append the
cuda/core/cu<cuda_major>absolute path (if it exists) tocuda.core.__path__attribute. This will mean users do not have to change relative imports to absolute imports.Checklist