Restore gc and re imports dropped in the nvfuser_direct port - #6048
Open
adityasingh2400 wants to merge 1 commit into
Open
Restore gc and re imports dropped in the nvfuser_direct port#6048adityasingh2400 wants to merge 1 commit into
adityasingh2400 wants to merge 1 commit into
Conversation
Two stdlib imports were lost when code was copied from the legacy nvfuser package into nvfuser_direct, and both call sites now raise NameError. python/nvfuser_direct/pytorch_utils.py calls gc.collect() in retry_on_oom_or_skip_test but never imports gc. The legacy python/nvfuser/pytorch_utils.py had import gc. That decorator is applied to every collected python benchmark in benchmarks/python/conftest.py and to an opinfo test in tests/python/opinfo/test_direct_ops.py, so an OOM there currently dies with NameError: name 'gc' is not defined instead of clearing the cache, retrying, and skipping. The retry never runs at all. python/nvfuser_direct/__init__.py calls re.sub twice in repro_script_for but never imports re. The legacy python/nvfuser/__init__.py had import re. That code path is the non-tensor input branch, so any fd.repro_script_for([..., scalar]) or last_repro_script() after execute(save_repro_inputs=True) with a scalar input raises NameError instead of emitting a repro script. Neither was caught by lint because .flake8 ignores F821, and __init__.py also carries noqa F403 for its star import. Adds two regression tests in tests/python/direct/test_python_direct.py. Both need only the built extension, no GPU: torch.OutOfMemoryError is raised directly and torch.cuda.empty_cache() is a no-op when CUDA is not initialized, and repro_script_for does not require the inputs to match the fusion. Signed-off-by: Aditya Singh <adisin650@gmail.com>
Contributor
Greptile SummaryRestores the missing
Confidence Score: 5/5The PR appears safe to merge, with the restored imports directly satisfying existing reachable call sites and the regression tests covering both failures. The changes only restore standard-library bindings already required by existing code, and the added tests exercise the previously failing branches without introducing a concrete behavioral regression. Important Files Changed
Reviews (1): Last reviewed commit: "Restore gc and re imports dropped in the..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two stdlib imports were lost when code was copied from the legacy
nvfuserpackage intonvfuser_direct. Both call sites raiseNameErrortoday.gcinpython/nvfuser_direct/pytorch_utils.pyretry_on_oom_or_skip_testcallsgc.collect()at line 64, and the file's complete import list istorch,DataTypefrom._C_DIRECT,ctypes,typing, andfunctools. There is noimport gc. The import is not coming in sideways either, since the extension import is an explicitfrom ._C_DIRECT import DataTyperather than a star import.The legacy
python/nvfuser/pytorch_utils.pyhadimport gcon line 10, alongside the identicalgc.collect()call. It was dropped when the function was ported.The blast radius is the whole benchmark suite.
benchmarks/python/conftest.pyline 201 wraps every collected item with this decorator, andtests/python/opinfo/test_direct_ops.pyline 219 applies it to an op test. So today an OOM in any of them raisesNameError: name 'gc' is not definedinstead of clearing the cache, retrying, and skipping on a second OOM. The retry never runs at all, which means the decorator's entire purpose is defeated and the failure it produces points at the wrong thing.reinpython/nvfuser_direct/__init__.pyFusionDefinition.repro_script_forcallsre.subat lines 483 and 486 and the file never importsre. The legacypython/nvfuser/__init__.pyhadimport reon line 16 with the same two calls.That code is the non-tensor input branch, the one that rewrites
infandnanintofloat("inf")andfloat("nan")so the emitted script is valid Python. Anyfd.repro_script_for([..., scalar]), orfd.last_repro_script()afterfd.execute(inputs, save_repro_inputs=True)with a scalar input, raisesNameErrorinstead of returning a repro script. The existingtest_repro_script_forpasses only CUDA tensors plus a constant scalar, whichdefine_scalardoes not register as a fusion input, so this branch has no coverage.Why lint did not catch either
.flake8listsF821among its ignores, so undefined names are invisible tolintrunner.__init__.pyadditionally carries# noqa: F401,F403on itsfrom ._C_DIRECT import *line, andF403is precisely the code meaning flake8 can no longer detect undefined names in that file.I swept the whole Python tree with an AST pass that reports any stdlib module used as
mod.attrwith no matching binding in scope, which avoids the star-import false positives that make a rawF821run unusable here. Acrosspython/,tests/python/,benchmarks/python/, andtools/it reports exactly these two and nothing else, so this is the complete set.Tests
Two regression tests in
tests/python/direct/test_python_direct.py.test_retry_on_oom_or_skip_testraisestorch.OutOfMemoryErroron the first call and asserts the wrapped function is invoked a second time and its value returned.test_repro_script_for_non_tensor_inputspasses2.5,float("inf"),float("nan"), and-float("inf")torepro_script_forand asserts each is emitted in a form that is valid Python.Neither needs a GPU.
torch.OutOfMemoryErroris raised directly rather than provoked,torch.cuda.empty_cache()is a no-op when CUDA is not initialized,repro_script_fordoes not require itsinputsargument to match the fusion, and thetorch.cuda.device_count()loop at the top of that method simply iterates zero times. They do need the built extension, like the rest of the file.Verification
Both fixes were verified fail-before and pass-after on this machine with no GPU and no CUDA build, by loading the real source files through
importlibagainst a stub_C_DIRECTmodule. Withpython/nvfuser_direct/__init__.pyandpython/nvfuser_direct/pytorch_utils.pychecked out frommain, the decorator raisesNameError: name 'gc' is not definedafter exactly one call, andrepro_script_forraisesNameError: name 're' is not defined. With the patched files, the decorator retries and returns on the second attempt, andrepro_script_foremits all four non-tensor inputs withinfandnancorrectly rewritten.Formatting checked with
black23.3.0 andflake86.1.0 against.flake8, the versions pinned in.lintrunner.toml. Both are clean andblackleaves all three files unchanged. Re-runningflake8 --select=F821overpython/nvfuser_direct/now reports nothing.