Uh oh!
There was an error while loading. Please reload this page.
fix(core): accept the documented str symbol_mapping in ObjectCode.get_kernel - #2585
Open
LeSingh1 wants to merge 1 commit into
Open
fix(core): accept the documented str symbol_mapping in ObjectCode.get_kernel#2585LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
…_kernel
Every `ObjectCode.from_*` constructor annotates and documents
`symbol_mapping: dict[str, str] | None`. `get_kernel` encodes the name only
on the mapping MISS path:
try:
name = self._sym_map[name]
except KeyError:
if isinstance(name, str):
name = name.encode()
cdef KernelHandle h_kernel = create_kernel_handle(self._h_library, <const char*>name)
On a HIT, `name` is rebound to the mapped value and handed straight to
`<const char*>`. cuda_core sets no `c_string_type`/`c_string_encoding`
directive (build_hooks.py passes only embedsignature, warn.deprecated.IF and
freethreading_compatible), so Cython's default applies and a `str` raises
"TypeError: expected bytes, str found".
So the documented form fails for exactly the names it is supposed to
translate:
ObjectCode.from_cubin(cubin, symbol_mapping={"saxpy<double>": mangled_str})
.get_kernel("saxpy<double>") # TypeError
.get_kernel("not_in_the_map") # fine
The reason this has gone unnoticed: `Program.compile` fills the mapping from
`nvrtcGetLoweredName`, whose `const char*` Cython converts to `bytes`, and
every existing test round-trips `mod.symbol_mapping` straight back into a
`from_*` constructor -- so only bytes values are ever exercised.
Move the encode past the lookup so both value types work. Compile-produced
mappings are unaffected.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.
Problem
Every
ObjectCode.from_*constructor annotates and documentssymbol_mapping: dict[str, str] | None— "a dictionary specifying how the unmangled symbol names (as keys) should be mapped to the mangled names".get_kernelencodes the name only on the mapping miss path (_module.pyx:802-808):On a hit,
nameis rebound to the mapped value and handed straight to<const char*>.cuda_coresets noc_string_type/c_string_encodingdirective —build_hooks.py:224passes onlyembedsignature,warn.deprecated.IFandfreethreading_compatible— so Cython's default applies and astrraisesTypeError: expected bytes, str found.The documented form therefore fails for exactly the names it exists to translate:
Why this has gone unnoticed:
Program.compilefills the mapping fromnvrtcGetLoweredName(_program.pyx:905-906), whoseconst char*Cython converts tobytes. Every existing test round-tripsmod.symbol_mappingstraight back into afrom_*constructor (test_module.py:340, 354, 367, 380, 395, 412, 422, 435), so onlybytesvalues are ever exercised.ObjectCode.symbol_mappingis likewise annotateddict[str, str]while returningbytesvalues.Fix
Move the encode past the lookup so both value types work. Compile-produced mappings are byte-for-byte unaffected.
Tests
test_object_code_symbol_mapping_accepts_str_values, modelled on the adjacenttest_object_code_load_cubin: decode the compile-produced mapping todict[str, str], rebuild theObjectCodefrom it, andget_kernelthrough a mapped name.What I ran
Environment: macOS, no CUDA driver and no CUDA toolkit, so
cuda.corecannot be built or imported here.test_module.py— they need a builtcuda.coreand a GPU.language_level=3, no string directives — matchingbuild_hooks.py) and drove it with both value types:So the change fixes the documented case and leaves the other three byte-identical.
python -m py_compile,ruff check,ruff format --checkoncuda_core/tests/test_module.py— clean, no new findings against amainbaseline.grep -rn "c_string_type\|c_string_encoding" cuda_core/finds nothing, so Cython's defaultstr→char*conversion (which rejectsstr) is what applies.ObjectCode.symbol_mapping's-> dict[str, str]annotation still describesbytesvalues for compile-produced mappings. Correcting that annotation is a separate, wider question (it would need to becomedict[str, str | bytes], or the compile path would need to decode), so I left it rather than widen this PR.