Skip to content

Avoid aggregate CUmemLocation initialization - #2673

Open
rwgk wants to merge 1 commit into
NVIDIA:mainfrom
rwgk:avoid_aggregate_cumemlocation_initialization
Open

Avoid aggregate CUmemLocation initialization#2673
rwgk wants to merge 1 commit into
NVIDIA:mainfrom
rwgk:avoid_aggregate_cumemlocation_initialization

Conversation

@rwgk

@rwgk rwgk commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Initialize CUmemLocation structures by declaring them and assigning the active type and id fields instead of using Cython aggregate constructors. Apply the same pattern when constructing the nested CUmemLocation inside CUmemAccessDesc.

This preserves the existing behavior while keeping the source compatible with both the older two-member generated declaration and the newer CUDA 13.4 declaration.

Rationale

This issue surfaced while building #2641. Public main's cuda.core source used aggregate initializers such as:

cydriver.CUmemLocation(type=..., id=...)

The older generated declaration exposed only type and id, making that initializer complete. CUDA 13.4 adds the localized arm to the anonymous location union, so the generated Cython declaration exposes type, id, and localized. With that declaration, Cython reports:

Not all members given for struct 'CUmemLocation'

All affected paths currently construct location kinds whose active payload is id: DEVICE, HOST, HOST_NUMA, or HOST_NUMA_CURRENT. They do not construct DEVICE_LOCALITY_DOMAIN, which uses the localized arm. Declaring the structure and assigning only its active fields therefore expresses the intended union use directly and compiles against both generated layouts.

This is a source-compatibility change only; it does not alter the selected location types, IDs, access flags, or runtime control flow.

Note for completeness

The aggregate initialization style entered through #2434 and was valid for the generated binding declaration available at that time. Public PR #2593 later centralized one of the location conversions in _memory/_location.pxd, so this patch updates that shared helper as well as the original call sites. This is a narrow compatibility follow-up that preserves the broader refactoring in those PRs.

@rwgk rwgk added this to the cuda.bindings 13.4.0 & 12.9.8 milestone Aug 20, 2026
@rwgk rwgk self-assigned this Aug 20, 2026
@rwgk rwgk added P0 High priority - Must do! cuda.core Everything related to the cuda.core module labels Aug 20, 2026
@rwgk
rwgk requested a review from juenglin August 20, 2026 03:25
@github-actions

Copy link
Copy Markdown

@juenglin

Copy link
Copy Markdown
Contributor

That's a bummer. I wish we wouldn't need to change code for this.

What if we suppressed these specific warnings (for CUmemLocation only) instead? Unfortunately cython has nor #pragma or similar for this purpose.

My agent came up with this workaround. What do you think?

@juenglin

Copy link
Copy Markdown
Contributor

I will leave the decision up to you.

@juenglin juenglin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't like it. Hope the workaround works.

@rwgk

rwgk commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

My agent came up with this workaround. What do you think?

codex gpt-5.6-sol reports:

The alternative has three concerns:

  • It monkey-patches private Cython internals in build_hooks.py. Meanwhile, cuda.core uses parallel cythonization, and Cython 3.2.5 uses ProcessPoolExecutor. The patch may be inherited with fork, but not reliably with spawn or forkserver, making its behavior platform and Python-version dependent.

  • It suppresses the warning for every incomplete CUmemLocation initializer. A future DEVICE_LOCALITY_DOMAIN call site that mistakenly omits localized could therefore hide a genuine bug.

  • It adds 45 lines dependent on Cython’s internal module layout and exact diagnostic text to avoid six straightforward source adaptations. That has a much higher maintenance cost.


@ajost, since you reviewed #2434 and #2593, could you weigh in on the two approaches here? Maybe there is a better 3rd alternative?

@juenglin

Copy link
Copy Markdown
Contributor

It suppresses the warning for every incomplete CUmemLocation initializer. A future DEVICE_LOCALITY_DOMAIN call site that mistakenly omits localized could therefore hide a genuine bug.

The same concern applies to the changes in this PR.

Another alternative is to isolate the 13.3/13.4 difference to the to_cumemlocation helper like in this PR. Even better if we could isolate it in cuda.bindings instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda.core Everything related to the cuda.core module P0 High priority - Must do!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants