Introduce LocatedHeaderDir to report the header discovery method
#1536
Conversation
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
|
||
| @functools.cache | ||
| def find_nvidia_header_directory(libname: str) -> str | None: | ||
| def find_nvidia_header_directory(libname: str, include_info: bool = False) -> FoundHeader | str | None: |
There was a problem hiding this comment.
Cursor is telling me this is an anti-pattern, with a list of reasons, including "return type ambiguity makes static analysis harder".
I think a new function name will be best, e.g. this could become
def find_nvidia_header_dir(libname: str) -> FoundHeader | None
with something like this (untested):
def find_nvidia_header_directory(libname: str) -> str | None:
found = find_nvidia_header_dir(libname)
return found.abs_path if found else None
Another idea would be locate_nvidia_header_directory as the name for the new function. I almost like that better: currently we only have this one find_... function in the public API, and we don't have a locate_... function, so we could make locate the new naming convention and soft-deprecate (documentation only) the find function.
|
|
||
|
|
||
| @dataclass | ||
| class FoundHeader: |
There was a problem hiding this comment.
I think having Dir in the name is important, to clearly distinguish between a directory and file.
FoundHeaderDir or LocatedHeaderDir (see other comments)
| abs_path: str | None | ||
| found_via: str | ||
|
|
||
| def _normalize_path(self) -> FoundHeader: |
There was a problem hiding this comment.
You can use __post_init__ to do the normalization automatically when the dataclass is instantiated. This is the standard pattern for dataclasses when you need to transform fields after initialization:
@dataclass
class FoundHeaderDir:
abs_path: str | None
found_via: str
def __post_init__(self):
self.abs_path = _abs_norm(self.abs_path)With this approach, you can remove the _normalize_path() method entirely, and also remove all the ._normalize_path() calls since normalization happens automatically at construction time.
| for cdir in candidate_dirs: | ||
| for hdr_dir in sorted(glob.glob(cdir), reverse=True): | ||
| if _joined_isfile(hdr_dir, h_basename): | ||
| return _abs_norm(hdr_dir) |
There was a problem hiding this comment.
WDYT about
return FoundHeaderDir(abs_path=hdr_dir, found_via="supported_install_dir")
?
| assert os.path.isfile(os.path.join(hdr_dir, h_filename)) | ||
| if STRICTNESS == "all_must_work": | ||
| assert hdr_dir is not None | ||
|
|
There was a problem hiding this comment.
I recommend changing the existing tests to call the richer new function, and then add only one trivial/small extra test at the bottom to exercise the old API.
There was a problem hiding this comment.
I'm a little hesitant to mutate the existing tests rather than adding more - one goal of this PR is to make sure the old API remains unperturbed. What do you think of circling back to cleaning out old tests if/when we deprecate the old API?
There was a problem hiding this comment.
I'd rather not maintain two parallel test implementations, that's how inconsistencies and confusion creep in over time.
The changes are trivial and super easy to review: change the function name in the calls sites and add .abs_path checks where needed. I think the insertions for assertions on .found_via will also be very obvious. One small test for the existing API wrapper will be sufficient to fully cover backward compatibility.
| info_summary_append(f"{None if not found_header_dir else found_header_dir.abs_path=!r}") | ||
| if found_header_dir: | ||
| # old api | ||
| hdr_dir = find_nvidia_header_directory(libname) |
There was a problem hiding this comment.
Under the if found_header_dir condition this doesn't get full test coverage.
Maybe (untested):
found_header_dir = locate_nvidia_header_directory(libname)
hdr_dir = find_nvidia_header_directory(libname) # old api
assert hdr_dir == None if not found_header_dir else found_header_dir.abs_path
info_summary_append(f"{hdr_dir=!r}")
Then simply use hdr_dir everywhere below.
This is the only change I'd definitely make. Optional, only if you like it:
FoundHeaderDir → LocatedHeaderDir
I'd probably change most everything from "found" to "located" for full internal consistency, including _locate_under_site_packages etc.
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
One more tiny thing.
Could you please trigger running the tests, after looking into my suggestion?
| info_summary_append(f"{hdr_dir=!r}") | ||
| if hdr_dir: | ||
| assert isinstance(located_hdr_dir, LocatedHeaderDir) | ||
| assert located_hdr_dir.found_via in ("site-packages", "conda", "CUDA_HOME") |
There was a problem hiding this comment.
Possibly we'll get "supported_install_dir" here.
What do you think about adding a helper function (untested):
def _located_hdr_dir_asserts(located_hdr_dir):
assert isinstance(located_hdr_dir, LocatedHeaderDir)
assert located_hdr_dir.found_via in ("site-packages", "conda", "CUDA_HOME", "supported_install_dir")
Then call the helper from here, and right after the current line 102 below in test_locate_ctk_headers?
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
rwgk
changed the title
FoundHeader to report the header discovery methodLocatedHeaderDir to report the header discovery method
|
2 participants
Closes #1148
This PR implements
LocatedHeaderDirto report the discovery method of the cuda headers. Similarly toLoadedDL, we capture this info at the time the library is found and surface it through a metadata object.After some tinkering I felt the best way of approaching this was to expand the existing API with a kwarg rather than breaking the API or introducing a parallel API that returns the struct rather than a string, which would have to maintain a separate implementation. But I'm open to other ways of doing things if they seem better.cc @rwgk