-
Notifications
You must be signed in to change notification settings - Fork 603
fix(build): Handle namespace packages for PyPI CUDA detection #2580
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
0f4e496 to
7237bea
Compare
The nvidia package from PyPI (e.g., nvidia-nccl-cu12/cu13) is a PEP 420 namespace package that doesn't have __file__ attribute. This caused get_cuda_include_dirs() to fail with TypeError when building with only NCCL from PyPI. Use nvidia.__path__[0] which works for both namespace packages and regular packages. Fixes: NVIDIA#2331 Signed-off-by: Santosh Bhavani <[email protected]>
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.
Greptile Overview
Greptile Summary
This PR fixes a bug where get_cuda_include_dirs() would crash when the nvidia package is a PEP 420 namespace package (which doesn't have a __file__ attribute). This occurs when users install NCCL and other CUDA packages from PyPI.
Key Changes:
- Replaced
Path(nvidia.__file__).parentwithPath(list(nvidia.__path__)[0]) - Added proper error handling when the nvidia package directory cannot be located
- Solution works for both regular packages (with
__init__.py) and namespace packages (without__init__.py)
How it works:
- All Python packages (both regular and namespace) have a
__path__attribute containing their directory locations - Regular packages also have
__file__, but namespace packages do not - By using
__path__[0], the code handles both package types uniformly
The fix is minimal, targeted, and maintains backward compatibility with existing installations.
Confidence Score: 5/5
- Safe to merge - fixes a real bug without introducing new issues
- The change correctly handles both namespace packages (PEP 420) and regular packages by using path[0] instead of file.parent. The truthiness check on path prevents IndexError from empty lists. The logic is sound and maintains backward compatibility.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| build_tools/utils.py | 4/5 | Fixed namespace package handling by using path[0] instead of file.parent - handles both PEP 420 namespace packages and regular packages correctly |
Sequence Diagram
sequenceDiagram
participant Builder as Build System
participant Utils as get_cuda_include_dirs()
participant Python as Python Import System
participant FS as File System
Builder->>Utils: Call to get CUDA headers
Utils->>Utils: Check cuda_toolkit_include_path()
alt CUDA toolkit found
Utils-->>Builder: Return toolkit include paths
else No toolkit, use PyPI packages
Utils->>Python: import nvidia
alt Import fails
Python-->>Utils: ModuleNotFoundError
Utils-->>Builder: RuntimeError("CUDA not found")
else Import succeeds
Python-->>Utils: nvidia module
Utils->>Utils: Check hasattr(nvidia, "__path__")
alt Has __path__ and not empty
Utils->>Python: Access nvidia.__path__[0]
Python-->>Utils: First path in __path__
Utils->>Utils: cuda_root = Path(__path__[0])
Utils->>FS: cuda_root.iterdir()
FS-->>Utils: List of subdirectories
Utils->>Utils: Filter for dirs with /include
Utils-->>Builder: Return list of include paths
else No __path__ or empty
Utils-->>Builder: RuntimeError("Could not locate nvidia package directory")
end
end
end
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.
Greptile Overview
Greptile Summary
This PR fixes a TypeError when building with PyPI CUDA packages (like nvidia-nccl-cu12/cu13) by handling PEP 420 namespace packages that lack the __file__ attribute. The fix correctly switches from using nvidia.__file__ to nvidia.__path__[0] in get_cuda_include_dirs().
Key Changes
- Modified
get_cuda_include_dirs()inbuild_tools/utils.pyto usenvidia.__path__[0]instead ofnvidia.__file__ - Added proper attribute checking with
hasattr(nvidia, "__path__")before accessing - Added clear error message when nvidia package directory cannot be located
Issues Found
- Incomplete fix:
transformer_engine/common/__init__.pyat lines 249-252 has the samenvidia.__file__pattern that will fail with namespace packages. Line 249 checksif nvidia.__file__ is None, but namespace packages raiseAttributeError(attribute doesn't exist), not returnNone. Consider extending this fix to that location. - Minor optimization:
list(nvidia.__path__)[0]can be simplified tonvidia.__path__[0]since__path__is already indexable.
Confidence Score: 3/5
- This PR is safe to merge with moderate confidence - it fixes the stated issue but leaves similar issues unaddressed
- The fix is technically correct and solves the immediate problem with namespace packages in
get_cuda_include_dirs(). However, the same pattern exists intransformer_engine/common/__init__.pywhich will still fail with namespace packages. Score of 3 reflects that this is a partial fix - the change works but doesn't address all instances of the problem in the codebase. - Consider reviewing
transformer_engine/common/__init__.pylines 249-252 for the same namespace package issue
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| build_tools/utils.py | 3/5 | Fixed get_cuda_include_dirs() to handle PEP 420 namespace packages using __path__, but missed fixing similar issue in transformer_engine/common/__init__.py and has minor code optimization opportunity |
Sequence Diagram
sequenceDiagram
participant Build as Build System
participant Utils as build_tools/utils.py
participant Nvidia as nvidia package
participant FS as File System
Build->>Utils: get_cuda_include_dirs()
alt CUDA Toolkit Installed
Utils->>Utils: cuda_toolkit_include_path()
Utils-->>Build: [toolkit_include_path]
else PyPI CUDA Packages
Utils->>Nvidia: import nvidia
alt ModuleNotFoundError
Nvidia-->>Utils: Error
Utils-->>Build: RuntimeError("CUDA not found.")
else Success
Nvidia-->>Utils: nvidia module
alt Has __path__ attribute (NEW)
Utils->>Nvidia: check hasattr(__path__) && __path__
alt __path__ exists and not empty
Utils->>Nvidia: nvidia.__path__[0]
Nvidia-->>Utils: First package path
Utils->>FS: iterate cuda_root subdirs
FS-->>Utils: subdirectories
Utils->>Utils: filter dirs with include/
Utils-->>Build: [subdir/include, ...]
else No __path__ or empty
Utils-->>Build: RuntimeError("Could not locate nvidia package directory.")
end
end
end
end
| # Handle namespace packages (PEP 420) which don't have __file__ | ||
| # The nvidia package from PyPI CUDA packages is a namespace package | ||
| if hasattr(nvidia, "__path__") and nvidia.__path__: | ||
| cuda_root = Path(list(nvidia.__path__)[0]) |
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.
Redundant list() conversion - nvidia.__path__ is already indexable, can use nvidia.__path__[0] directly
| cuda_root = Path(list(nvidia.__path__)[0]) | |
| cuda_root = Path(nvidia.__path__[0]) |
| # Handle namespace packages (PEP 420) which don't have __file__ | ||
| # The nvidia package from PyPI CUDA packages is a namespace package | ||
| if hasattr(nvidia, "__path__") and nvidia.__path__: | ||
| cuda_root = Path(list(nvidia.__path__)[0]) | ||
| else: | ||
| raise RuntimeError("Could not locate nvidia package directory.") |
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.
This fix correctly handles namespace packages here, but transformer_engine/common/__init__.py line 249-252 has the same issue with nvidia.__file__:
- Line 249 checks
if nvidia.__file__ is None, but namespace packages will raiseAttributeErrorwhen accessing__file__(the attribute doesn't exist, it doesn't return None) - Line 252 uses
Path(nvidia.__file__).parentwhich will also fail with namespace packages
Consider applying the same fix pattern (__path__) to _nvidia_cudart_include_dir() in that file. Should this PR also fix the similar nvidia.__file__ usage in transformer_engine/common/__init__.py line 249-252, or will that be addressed in a separate PR?
Description
The NCCL package from PyPI (e.g.,
nvidia-nccl-cu12/cu13) is a PEP 420 namespace package that doesn't have__file__attribute. This causedget_cuda_include_dirs()to fail with TypeError when building with only NCCL from PyPI.Use
nvidia.__path__[0]which works for both namespace packages and regular packages.Fixes: #2331
Type of change
Changes
Please list the changes introduced in this PR:
get_cuda_include_dirs()to handle namespace packages (which lack__file__)nvidiapackage directory cannot be locatedChecklist: