Skip to content
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

[V1] Various updates #28

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

njhill
Copy link
Collaborator

@njhill njhill commented Nov 6, 2024

See inline comments

Comment on lines +47 to +48
omit_defaults=True,
gc=False):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These also have performance benefit

Comment on lines 145 to 148
def _finish_stream(self, request_id: str):
stream = self.request_streams.pop(request_id)
if stream is not None:
stream.finish()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to remove the stream if it finishes due to a finished request output or due to client cancellation.

await self.engine_core.abort_requests_async(request_ids)
self.detokenizer.abort_requests(request_ids)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This shouldn't be needed because the detokenizer removes finished requests itself.

Comment on lines +324 to +325
encoder.encode_into(outputs, buffer)
socket.send_multipart((buffer, ),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will reuse the buffer rather than allocating one each time.

@neuralmagic neuralmagic deleted a comment from github-actions bot Nov 6, 2024
@robertgshaw2-redhat robertgshaw2-redhat merged commit 1591996 into neuralmagic:rework-rs-proto Nov 6, 2024
@njhill njhill deleted the v1/updates branch November 6, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants