-
Notifications
You must be signed in to change notification settings - Fork 284
refactor(tests): replace exec() with importlib in example runner #1628
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?
Changes from 1 commit
534aa0a
f21199d
6ae7f46
0a1aee7
90f1505
8a931d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,21 +3,22 @@ | |
|
|
||
| # If we have subcategories of examples in the future, this file can be split along those lines | ||
|
|
||
| import glob | ||
| import os | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| from cuda.core import Device | ||
|
|
||
| from .utils import run_example | ||
|
|
||
| samples_path = os.path.join(os.path.dirname(__file__), "..", "..", "examples") | ||
| sample_files = glob.glob(samples_path + "**/*.py", recursive=True) | ||
| # not dividing, but navigating into the "examples" directory. | ||
| EXAMPLES_DIR = Path(__file__).resolve().parent.parent.parent / "examples" | ||
|
|
||
| # recursively glob for test files in examples directory, sort for deterministic | ||
| # test runs. Relative paths offer cleaner output when tests fail. | ||
| SAMPLE_FILES = sorted([str(p.relative_to(EXAMPLES_DIR)) for p in EXAMPLES_DIR.glob("**/*.py")]) | ||
|
|
||
| @pytest.mark.parametrize("example", sample_files) | ||
|
|
||
| @pytest.mark.parametrize("example_rel_path", SAMPLE_FILES) | ||
| class TestExamples: | ||
| def test_example(self, example, deinit_cuda): | ||
| run_example(samples_path, example) | ||
| if Device().device_id != 0: | ||
| Device(0).set_current() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should kick off CI and check if removing this line is OK. IIRC on a multi-GPU system it would fail without this.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I think I get this, let me know if I mess up:
So, in a multi-GPU example, the driver recruits n devices using a given thread, then runs the kernel and calls The ProblemThe driver doesn't update the A Possible SolutionRedundantly set def test_example(self, example_rel_path: str, deinit_cuda) -> None:
from cuda.core import Device
Device(0).set_current()
run_example(str(EXAMPLES_DIR), example_rel_path) |
||
| # deinit_cuda is defined in conftest.py and pops the cuda context automatically. | ||
| def test_example(self, example_rel_path: str, deinit_cuda) -> None: | ||
| run_example(str(EXAMPLES_DIR), example_rel_path) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,9 @@ | |
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import gc | ||
| import os | ||
| import importlib.util | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
|
|
@@ -12,24 +13,38 @@ class SampleTestError(Exception): | |
| pass | ||
|
|
||
|
|
||
| def parse_python_script(filepath): | ||
| if not filepath.endswith(".py"): | ||
| raise ValueError(f"{filepath} not supported") | ||
| with open(filepath, encoding="utf-8") as f: | ||
| script = f.read() | ||
| return script | ||
| def run_example(parent_dir: str, rel_path_to_example: str, env=None) -> None: | ||
| fullpath = Path(parent_dir) / rel_path_to_example | ||
| module_name = fullpath.stem | ||
|
|
||
| old_sys_path = sys.path.copy() | ||
| old_argv = sys.argv | ||
|
|
||
| def run_example(samples_path, filename, env=None): | ||
| fullpath = os.path.join(samples_path, filename) | ||
| script = parse_python_script(fullpath) | ||
| try: | ||
| old_argv = sys.argv | ||
| sys.argv = [fullpath] | ||
| old_sys_path = sys.path.copy() | ||
| sys.path.append(samples_path) | ||
| # TODO: Refactor the examples to give them a common callable `main()` to avoid needing to use exec here? | ||
| exec(script, env if env else {}) # noqa: S102 | ||
| sys.path.append(parent_dir) | ||
| sys.argv = [str(fullpath)] | ||
|
|
||
| # Collect metadata for file 'module_name' located at 'fullpath'. | ||
| # CASE: file does not exist -> spec is none. | ||
| # CASE: file is not .py -> spec is none. | ||
| # CASE: file does not have proper loader (module.spec.__loader__) -> spec.loader is none. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we necessarily need to duplicate information from the Python stdlib docs here, but don't feel strongly either way.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, my ruff linter was flagging it as a potential point of runtime failure, the if-none check fixed these issues though I figured I'd leave an explanation to help people understand the need for the check, in case there are any future revisions.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a bit more complicated than necessary...? Assuming we always have main (#1664), can we simplify the logic here after merging the other PR?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, technically all we need once the examples are refactored to Python modules is the following spec = importlib.util.spec_from_file_location(module_name, fullpath)
module = importlib.util.module_from_spec(spec)
sys.modules[module_name] = module
spec.loader.exec_module(module)
module.main()There are 2 possible ways example(s) code may be invokedvia a user: In this case, via a script: In this case |
||
| spec = importlib.util.spec_from_file_location(module_name, fullpath) | ||
|
|
||
| if spec is None or spec.loader is None: | ||
| raise ImportError(f"Failed to load spec for {rel_path_to_example}") | ||
|
|
||
| # Otherwise convert the spec to a module, then run the module. | ||
| module = importlib.util.module_from_spec(spec) | ||
| sys.modules[module_name] = module | ||
|
|
||
| # This runs top-level code. | ||
| # CASE: exec() -> top-level code is implicitly run. | ||
| spec.loader.exec_module(module) | ||
|
|
||
| # CASE: main() -> we find main and call it below. | ||
| if hasattr(module, "main"): | ||
| module.main() | ||
|
|
||
| except ImportError as e: | ||
| # for samples requiring any of optional dependencies | ||
| for m in ("cupy", "torch"): | ||
|
|
@@ -40,14 +55,16 @@ def run_example(samples_path, filename, env=None): | |
| raise | ||
| except SystemExit: | ||
| # for samples that early return due to any missing requirements | ||
| pytest.skip(f"skip {filename}") | ||
| pytest.skip(f"skip {rel_path_to_example}") | ||
| except Exception as e: | ||
| msg = "\n" | ||
| msg += f"Got error ({filename}):\n" | ||
| msg += f"Got error ({rel_path_to_example}):\n" | ||
| msg += str(e) | ||
| raise SampleTestError(msg) from e | ||
| finally: | ||
| sys.path = old_sys_path | ||
| sys.argv = old_argv | ||
|
|
||
| # further reduce the memory watermark | ||
| sys.modules.pop(module_name, None) | ||
| gc.collect() | ||
Uh oh!
There was an error while loading. Please reload this page.