Uh oh!
There was an error while loading. Please reload this page.
fix(core): four LinkerOptions fields that don't behave as documented - #2582
Open
LeSingh1 wants to merge 1 commit into
Open
fix(core): four LinkerOptions fields that don't behave as documented#2582LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
1. time=False ENABLES timing.
if self.time is not None:
options.append("-time")
`-time` is a valueless nvJitLink switch, so the gate has to be truthiness.
Every other valueless flag in the same function gets that right (verbose,
-lto, -ptx, -g, -lineinfo, and `no_cache is True`); `is not None` is only
correct for the flags that emit a value, e.g. `-ftz=true|false`. And on the
driver backend the mirror-image gate raises "time option is not supported by
the driver API" for a user who explicitly DISABLED it.
2. optimize_unused_variables=False ENABLES the optimization.
Same shape, same valueless switch. This one silently changes the linked
binary: the linker drops device variables the caller asked to keep. The
driver path also emits a DeprecationWarning for the disabled value.
3. kernels_used / variables_used silently ignore the documented tuple form.
if isinstance(self.kernels_used, str): ...
elif isinstance(self.kernels_used, list): ...
Both are annotated (and documented) `str | tuple[str] | list[str]`. A tuple
matches neither branch, so no option is emitted and nothing is raised -- the
link keeps every kernel/variable the caller meant to filter out.
`ptxas_options`, eleven lines below, already uses `is_sequence()`.
tests/test_linker.py:83,86 already parametrize the tuple form, but only
assert that linking succeeds, which it does -- with no filtering.
4. LinkerOptions(name=None) raises AttributeError.
self._name = self.name.encode()
`name` is annotated `str | None`. ProgramOptions had the identical bug and
was fixed in NVIDIA#2517 by falling back to its documented default; do the same
here.
All four are error-path or option-emission changes; no configuration that
linked correctly before links differently now.Contributor
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 freeto 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.
Four
LinkerOptionsfields whose behaviour contradicts their own annotation and docstring. Grouped: one class, one file, and the first three are literally the same mistake in the same function.1.
time=Falseenables timing-timeis a valueless nvJitLink switch, so the gate must be truthiness. Every other valueless flag in_prepare_nvjitlink_optionsgets that right —if self.verbose:(:326),-lto(:328),-ptx(:330),-g(:334),-lineinfo(:336),if self.no_cache is True:(:370).is not Noneis only correct for the flags that emit an explicit value, e.g.-ftz=true|false(:338)._prepare_driver_optionshas the mirror image (:401): a user who explicitly disabled timing getsValueError: time option is not supported by the driver API.2.
optimize_unused_variables=Falseenables the optimizationSame shape (
:358), same valueless switch — and this one silently changes the linked binary: the linker drops device variables the caller asked it to keep. The driver path (:432) also emits aDeprecationWarningfor the disabled value.3.
kernels_used/variables_usedsilently ignore the documentedtupleformBoth are annotated and documented
str | tuple[str] | list[str]. A tuple matches neither branch, so no option is emitted and nothing is raised — the link keeps every kernel/variable the caller meant to filter out.ptxas_options, eleven lines below (:363), already usesis_sequence().Worth noting:
tests/test_linker.py:83,86already parametrize the tuple form — but only assert that linking succeeds, which it does, with no filtering applied. That is why this has gone unnoticed.4.
LinkerOptions(name=None)raisesAttributeErrorname: str | None = "<default linker>".ProgramOptionshad the identical bug and was fixed in #2517 by falling back to its documented default; this does the same.(Observation, deliberately not changed here to keep the diff a straight mirror of #2517:
LinkerOptions._nameis written on this line and never read —grep -n '\._name\b'overcuda_core/shows the only readers areProgramOptions._namein_program.pyx:778,810,978and the unrelatedObjectCode._name. Happy to drop the dead store in a follow-up if you want it gone.)Compatibility
All four are error-path or option-emission changes. No configuration that linked correctly before links differently now:
Truestill emits,Nonestill emits nothing,str/listare unchanged. The inputs whose behaviour changes are exactlytime=False,optimize_unused_variables=False, tuple sequences, andname=None— each of which is currently doing the opposite of, or nothing about, what the caller asked for.Tests
Five new cases in
cuda_core/tests/test_linker.py, next to the existing_prepare_driver_optionsunit tests:test_valueless_flags_are_gated_on_truthiness—Trueemits,Falseand unset do not (parametrised over both flags).test_sequence_options_accept_tuples— parametrised overlist/tuple×kernels_used/variables_used.test_linker_options_accepts_name_none.test_prepare_driver_options_ignores_disabled_flags— a disabled flag neither raises nor warns on the driver backend.The existing
test_prepare_driver_options_deprecated_warnings/_unsupported_raisesparametrisations all useTrueand are unchanged.What I ran
Environment: macOS, no CUDA driver and no CUDA toolkit, so
cuda.corecannot be built or imported here.test_linker.py—LinkerOptions.__post_init__calls_lazy_init(), which probes nvJitLink, so the class cannot even be constructed without a toolkit.__post_init__, verbatim, before and after:python -m py_compile,ruff check,ruff format --checkoncuda_core/tests/test_linker.py— clean, no new findings against amainbaseline. (ruffcannot parse.pyx, so_linker.pyxwas reviewed by hand.)is_sequenceisisinstance(obj, Sequence)(_utils/cuda_utils.pyx:292), sostrmatches it too — which is why theisinstance(..., str)branch must stay first, exactly asptxas_optionsalready orders it.is_sequencewas already imported and used in_linker.pyx.grepfortime=,optimize_unused_variables,kernels_used,variables_used,LinkerOptions(nameacrosscuda_core/tests/shows onlyTrue/str/list/tupleinputs andname="ABC".Refs #2517