refactor: Remove C shared memory shim. Refine shared memory utils behavior. Add unit test for shared memory #797
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Summarizing the changes, it seems like the idea is to more concretely delineate handling when shared memory keys collide? IIUC, the first to register the shared memory controls the size of the region. Subsequent shared memory handles can register with the same key, but must be As a result, we must now account for scenarios where the initial registrant has been deleted. In such a case, other handles registered with the same key will encounter an unlink error/will not exist in the shm region map. |
Yes, the goal is to have unit test to enforce the shared memory utils behavior, whereas previously there is no detailed documentation on it, and I would like to restrict the behavior and document it. i.e. you may call
Yes, I think I would want to do more implementation change on the shared memory utils to have more internal tracking and only unlink the region when the destroy call on the last handle. One other change I am going to make is that make create API default to be "allow create or reuse" to avoid breaking existing usage, some of our tests fail due to current change of raising error on reuse by default (ref). Even though it ends up being the test is not cleaning up shared memory properly between test cases, but I am going to avoid this backward-incompatible change, and instead provide option |
|
test cases and implementation ready for review |
GuanLuo
changed the title
| Whether a shared memory region must be created. If False and | ||
| a shared memory region of the same name exists, a handle to that | ||
| shared memory region will be returned and user must be aware that | ||
| the shared memory size can be different from the size requested. |
There was a problem hiding this comment.
| the shared memory size can be different from the size requested. | |
| the previously allocated shared memory size can be larger than the size requested. |
Nit. Current language seems to imply the user can request a size that is larger than previously allocated. This was valid behavior before, but is not with your changes now. I think we want to make that clear here.
There was a problem hiding this comment.
Applied the change but still refer to that the size can be different instead of larger. This part is actually a bit fuzzy. In the previous implementation you can actually write / read from the mapped memory even if it is smaller / larger than the actual region size, which is obviously wrong. Current implementation at least make sure that the created region will be reused without modification, and provide basic region boundary check. So it is now an error if you requested a larger size and write / read larger data, but if you requested a smaller size, then it is fine as long as your following actions act on the requested (smaller) size.
src/python/library/tritonclient/utils/shared_memory/__init__.py
Outdated
Show resolved
Hide resolved
| shm_tensor_view = np.ndarray( | ||
| input_value.shape, | ||
| input_value.dtype, | ||
| buffer=shm_handle._mpsm_handle.buf[offset:], | ||
| ) | ||
| ) | ||
| offset_current += byte_size | ||
| return | ||
| shm_tensor_view[:] = input_value[:] |
There was a problem hiding this comment.
Doesn't buffer=shm_handle._mpsm_handle.buf[offset:], fill the array?
Are we immediately overwriting the np array contents with shm_tensor_view[:] = input_value[:]? Or using np array underlying buffer as a mechanism to write to the mpsm_handle.buf?
https://numpy.org/devdocs/reference/generated/numpy.ndarray.html
buffer : object exposing buffer interface, optional
Used to fill the array with data.
There was a problem hiding this comment.
The passed buffer is the underlying buffer of the numpy array, this method is used in the shared memroy example
What does the PR do?
Add unit test to constraint the behavior of shared memory utilities
Checklist
<commit_type>: <Title>Commit Type:
Check the conventional commit type
box here and add the label to the github PR.
Related PRs:
Where should the reviewer start?
test_shared_memory.pyhas inline comment on change in behaviorTest plan:
L0_python_client_unit_tests
19586188
Caveats:
This PR is marked as draft to collect feedback on the test cases and API level changes, the PR will be updated with new implementation which uses Python's native shared memory support
Background
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)