Conversation
Greptile SummaryReplaces manual CPython API calls ( Confidence Score: 5/5Safe to merge; the refcount simplification is correct and the only finding is a P2 API narrowing that doesn't affect any current caller. The Cython simplification is semantically equivalent to the old manual PyAPI code and the reference counting for all callbacks is correct. The sole finding — strides/storage_offset being dropped from the malloc callback API — has no impact on the existing torch implementation since torch.empty only needs shape and dtype. No regressions are introduced. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as C_Runtime
participant CY as Cython_Callbacks
participant PY as Python_env_fns
C->>CY: create_context
CY->>PY: fn(ctx) returns TorchMemoryContext
C->>CY: malloc with tensor_desc and malloc_type
CY->>PY: fn(shape_tuple, dtype_int, malloc_type_int, mem_ctx, ctx)
PY-->>CY: int64 pointer
CY-->>C: void pointer
C->>CY: free with memory_context
CY->>PY: fn(mem_ctx, ctx)
C->>CY: destroy_context with memory_context
CY->>PY: fn(mem_ctx, ctx)
Reviews (4): Last reviewed commit: "Replace manual PyTuple_New/PyTuple_SetIt..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if global_context == NULL: | ||
| (<void **> memory_context)[0] = NULL | ||
| return |
There was a problem hiding this comment.
memory_context is dereferenced at line 428 without first checking for null. When global_context == NULL, the code writes to memory_context[0] without validating memory_context itself is non-null. Every other callback in this PR (destroy, malloc, free, output_malloc, output_free) guards against null memory_context before dereferencing. This should be consistent:
| if global_context == NULL: | |
| (<void **> memory_context)[0] = NULL | |
| return | |
| if global_context == NULL: | |
| if memory_context != NULL: | |
| (<void **> memory_context)[0] = NULL | |
| return |
|
/ok to test 64d68c3 |
|
/ok to test 18054d8 |
|
Looks like this did fix the first round of segfaults. I added a 3.14 test job for Currently: |
with Cython-native function call syntax in all env callback wrappers, and pass plain Python objects (tuple, int) instead of cdef class instances across the C→Python boundary.
linhu-nv
force-pushed
the
fix-append-unique
branch
from
18054d8 to
81d049a
Compare
|
/ok to test 81d049a |
alexbarghi-nv
added
bug
alexbarghi-nv
left a comment
alexbarghi-nv
left a comment
There was a problem hiding this comment.
Looks good, thanks for getting this fix in.
|
/merge |
3 participants
No description provided.