-
Notifications
You must be signed in to change notification settings - Fork 190
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
batch processing as "mlserver infer ..." CLI #720
batch processing as "mlserver infer ..." CLI #720
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.
This is looking great @RafalSkolasinski !
Example input file
example output file:
Above can be for example use against mnist-svm (I believe I have exactly the same model there as one in the MLServer docs here) with
|
Currently only
Some notes from the in person code review with @adriangonz :
|
queue.task_done() | ||
|
||
|
||
async def process_batch( |
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.
One for the design doc but there may be some value for providing a process_batch_sync
function to simplify interaction if someone wants to call the function from a non-asyncio loop in python - otherwise they'll have to handle the asyncio logic themselves, but perhaps not as bad, maybe it could just be documentation that explains how people could do it if needed
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.
Actually, for now this is meant mostly as the CLI entrypoint, not to be yet used directly anywhere else. I am going to add the docstring.
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 added small docstrings just now
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.
Exposing it as from mlserver import ...
functionality is definitely a worth following up on though 👍
Closes #753