Fix #1995: Use StrEnum for enum-like strings#2016
Conversation
This comment has been minimized.
This comment has been minimized.
PR 2016 Agent ReviewReviewer: Opus 4.7 1M (Max), 2026-05-04 With a few small manual edits. I read through the entire review but only checked a few of the many details. ContextThis PR is the second half of the cuda.core v1.0.0 enum hardening that PR #2014 started. Where 2014 wrapped NVML enums for
It uses Blocking IssuesThese should be addressed in this PR. 1.
|
This is to address @leofang's concern offline about "class explosion" from using enums. By documenting them as linked from functions that use the enums, and not putting them at the top-level table of contents, I think this helps quite a bit with that concern. If anything the
Let's separate that out. Not required for 1.0
That suggestion does not work with our current Sphinx setup, and it is also not as ergonomic at the terminal or Jupyter. Sync testsAs with #2014, I'll experiment with what sync testing would look like, but it is not critical for 1.0 |
rwgk
left a comment
There was a problem hiding this comment.
Only two tiny fixes. The rest looks good to me.
| CodeTypeT = bytes | bytearray | str | ||
|
|
||
| cdef tuple _supported_code_type = ("cubin", "ptx", "ltoir", "fatbin", "object", "library") | ||
| cdef tuple _supported_code_type = tuple(CodeType.__members__.values()) |
There was a problem hiding this comment.
Ah, when reading this side-by-side it is easy to catch: We have both CodeTypeT and CodeType meaning two very different things, we probably should fix this
There was a problem hiding this comment.
Would CodeFormat (for the enum with "cubin", "ptx" etc.) make sense?
There was a problem hiding this comment.
Maybe ObjectCodeFormatT? The ObjectCode class can be constructed from any supported format.
Another naming convention discussion: Our type annotations have been made a bit C++-ish (but consistently) by appending a suffix T: IsStreamT, CodeTypeT, .... Maybe a good time to decide a convention and stick to it. My only concern (as raised elsewhere) is we might have both actual classes and associated type annotations refer to overlapping or orthogonal concepts. Would be nice to avoid ambiguity.
There was a problem hiding this comment.
In think in this case, enums are not type annotations (they have members and runtime semantics and such), so shouldn't have the T. I know we are putting them in typing.
|
This got a pretty significant rework based on @leofang's feedback. All the new enums are defined in |
leofang
left a comment
There was a problem hiding this comment.
LGTM overall! Left a bunch of minor comments mainly for consistency.
Ok. I renamed everything so it has a consistent |
leofang
left a comment
There was a problem hiding this comment.
We will need a release note entry, feel free to add it in this PR or the next one. Approving.
|
See discussion in #1995.