Skip to content

fix(cuda.core): preserve adjacency set semantics - #2893

Merged
mdboom merged 4 commits into
NVIDIA:mainfrom
rluo8:fix/adjacency-discard-annotation-typing
Sep 18, 2026
Merged

mdboom merged 4 commits into
NVIDIA:mainfrom
rluo8:fix/adjacency-discard-annotation-typing

Conversation

@rluo8

@rluo8 rluo8 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Description

This is found when adding coverage test cases.

AdjacencySetProxy.discard() is intended to follow MutableSet semantics and silently ignore values that are not present.

Because GraphNode is a Cython extension type, Cython interprets the value: GraphNode annotation as a runtime argument check. Non-GraphNode values therefore raise TypeError before the method body can reach its membership check.

This change disables Cython annotation typing for discard() only. The annotation remains available to static tooling and generated stubs, while runtime values can reach the existing value not in self check and return without modifying the graph.

add() remains unchanged and continues to reject non-GraphNode values with TypeError.

A regression test verifies that discarding str and int values is a no-op and preserves the existing adjacency set.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions Bot added the cuda.core Everything related to the cuda.core module label Sep 17, 2026
@rluo8 rluo8 self-assigned this Sep 17, 2026
@github-actions

This comment has been minimized.

@mdboom mdboom 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.

In isolation this looks good to me, and I'm fine with merging.

This does get to the larger question @LeoF raised about whether we should turn of annotation typing globally. I made this issue to track that: #2894

Also Cc @juenglin for viz.

@mdboom mdboom added this to the cuda.core 1.3.0 milestone Sep 17, 2026
@mdboom
mdboom self-requested a review September 17, 2026 16:19

@mdboom mdboom 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.

Just came back to say: This needs a release note mention (since it's technically a behavioral change). I think it's one we can live with, but we still need to note it.

Other than that, I still think this is good-to-go.

@mdboom mdboom added the bug Something isn't working label Sep 17, 2026
@rluo8

rluo8 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @mdboom. I've added a release note for this change.

@rluo8
rluo8 requested a review from mdboom September 18, 2026 02:03

@mdboom mdboom 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.

LGTM. Thanks.

@mdboom
mdboom merged commit 58c276e into NVIDIA:main Sep 18, 2026
115 checks passed
@github-actions

Copy link
Copy Markdown
Contributor
Doc Preview CI
Preview removed because the pull request was closed or merged.

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

Labels

bug Something isn't working cuda.core Everything related to the cuda.core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants