-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate the memory requested for the infer request is not out of bounds #7083
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! apart from things @Tabrizian pointed out
…rence-server/server into kyang-validate-shm-byte-size
@@ -295,7 +317,7 @@ def test_too_big_shm(self): | |||
def test_mixed_raw_shm(self): | |||
# Mix of shared memory and RAW inputs | |||
error_msg = [] | |||
shm_handles = self._configure_sever() | |||
shm_handles = self._configure_sever(shm_byte_size=256) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My PR will have some conflicts with this PR, but you should be able to use the create_byte_size
and register_byte_size
args in the same way. Let me know if you're good with it: #7093
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Francesco's PR will also likely have some merge conflicts: #7048
@@ -292,7 +328,7 @@ def test_too_big_shm(self): | |||
def test_mixed_raw_shm(self): | |||
# Mix of shared memory and RAW inputs | |||
error_msg = [] | |||
shm_handles = self._configure_sever(shm_byte_size=256) | |||
shm_handles = self._configure_server() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this supposed to have a value of 256 passed to it? or doesn't matter for this test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was only needed because I had a bug so I removed it..
…nds (#7083) Validate that memory offset and byte size requested is not out of bounds of registered memory. Previously in #6914 we checked out of bounds offset for shared memory requests. This PR also adds more testing to verify the block of memory is in fact in bounds. Client change: triton-inference-server/client#565
…nds (#7083) (#7111) Validate that memory offset and byte size requested is not out of bounds of registered memory. Previously in #6914 we checked out of bounds offset for shared memory requests. This PR also adds more testing to verify the block of memory is in fact in bounds. Client change: triton-inference-server/client#565
Fixes windows build issues caused by: #7083
Previously in #6914 we checked out of bounds
offset
for shared memory requests. This PR updates to a more comprehensive test of verifying that the block of memory is in fact in bounds.Client change: triton-inference-server/client#565