Skip to content

Conversation

@leofang
Copy link
Member

@leofang leofang commented Oct 8, 2025

Description

Try to avoid cleanup errors like this

Traceback (most recent call last):
  File "cuda/core/experimental/_memory.pyx", line 138, in cuda.core.experimental._memory.Buffer.close
SystemError: Objects/dictobject.c:2395: bad argument to internal function
Traceback (most recent call last):
  File "cuda/core/experimental/_memory.pyx", line 751, in cuda.core.experimental._memory.DeviceMemoryResource.close
SystemError: Objects/dictobject.c:2395: bad argument to internal function
Exception ignored in: 'cuda.core.experimental._memory.DeviceMemoryResource.__dealloc__'

Checklist

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

@copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Oct 8, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@leofang leofang self-assigned this Oct 8, 2025
@leofang
Copy link
Member Author

leofang commented Oct 8, 2025

/ok to test 8a1ca77

@leofang leofang added bug Something isn't working P0 High priority - Must do! cuda.core Everything related to the cuda.core module labels Oct 8, 2025
@leofang leofang added this to the cuda.core beta 7 milestone Oct 8, 2025
@github-actions
Copy link

github-actions bot commented Oct 8, 2025

else:
s = <cyStream>stream
self._mr._deallocate(self._ptr, self._size, s)
self._ptr = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This was something I had mentioned elsewhere, that we shouldn't be accessing object properties in a __dealloc__ code path.

Why is _ptr still being touched here?

From the Cython docs on __dealloc__:

In particular, don’t call any other methods of the object or do anything which might cause the object to be resurrected. It’s best if you stick to just deallocating C data.

Why not just do the minimum C-API invocation required in __dealloc__ and duplicate whatever you need to from close?

Then we're not going against the documented behavior and causing a crashing knowing that we could have avoided it all along.

Copy link
Member Author

Choose a reason for hiding this comment

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

self._ptr is a cdef attribute. I think it might be a confusion with self._ptr_obj?

Regardless, I don't believe this PR is needed. I think #1104 and #1105 together should fix everything for us.

@leofang leofang closed this Oct 8, 2025
@leofang leofang deleted the fix_cleanup branch October 8, 2025 14:56
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
Removed preview folders for the following PRs:
- PR #1021
- PR #1034
- PR #1052
- PR #1059
- PR #1069
- PR #1086
- PR #1090
- PR #1096
- PR #1102
- PR #1103
- PR #1106
- PR #1107
- PR #1117
- PR #1133
- PR #1140
- PR #1166
- PR #1174
- PR #1185
- PR #1188
- PR #1191
... and 41 more
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 P0 High priority - Must do!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants