-
Notifications
You must be signed in to change notification settings - Fork 25
feat: improve get local kernel importing #129
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
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
Nice, haven't checked the details but looks good conceptually (and the example scripts looks quite complete).
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.
LGTM thanks for adding this 🤗
| # Presume we were given the top level path of the kernel repository. | ||
| for base_path in [repo_path, repo_path / "build"]: | ||
| # Prefer the universal variant if it exists. | ||
| for v in [universal_variant, variant]: | ||
| package_path = base_path / v / package_name / "__init__.py" | ||
| if package_path.exists(): | ||
| return import_from_path(package_name, package_path) | ||
|
|
||
| # If we didn't find the package in the repo we may have a explicit | ||
| # package path. | ||
| package_path = repo_path / package_name / "__init__.py" | ||
| if package_path.exists(): | ||
| return import_from_path(package_name, package_path) |
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.
Could you add test cases for these? Not for universal vs. non-universal, but for the three different types of paths that are supported?
Probably best to change the fixture here:
Line 13 in 1caa4c1
| def local_kernel(): |
to return the path and not the loaded kernel. Then the existing test could be extended to test these.
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.
yea no problem, totally agree that we should have tests for this. Just opened a PR here #134
This PR update the
get_local_kernelfunction to handle more paths and infer the variant if needed.These changes handle specifying the top level of the kernel repo or a specific variant directory as shown in the example below