Add tool completion to batch inference#461
Conversation
Georgepu1
left a comment
There was a problem hiding this comment.
overall lgtm and it's nice that tool_completion currently mirrors the latest version, just a couple of nits. thanks for adding this feature in for batch complete calls, we'll add it into our inference pipeline
model-engine/model_engine_server/inference/batch_inference/generate_tool_sample_data.py
Show resolved
Hide resolved
model-engine/model_engine_server/inference/batch_inference/sample_data_tool.json
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/batch_inference/vllm_batch.py
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/batch_inference/vllm_batch.py
Show resolved
Hide resolved
|
|
||
|
|
||
| TOOL_MAP = { | ||
| Tools.CODE_EVALUATOR: CodeBlockEvaluator, |
There was a problem hiding this comment.
nit: move each tool to own file for future if there are more tools
| self.evaluate = self.evaluate_code_in_docker | ||
| except docker.errors.DockerException: | ||
| # If docker is not available, use the python interpreter | ||
| self.evaluate = self.evaluate_code_using_exec |
There was a problem hiding this comment.
is this safe for the batch job? seems that potentially harmful generations could break this
There was a problem hiding this comment.
(synced offline) known risk - for now since main use is internal, risk should be low and this is okay. will have follow-ups to mitigate issues
There was a problem hiding this comment.
that's true. please take a look how we think about this problem: https://docs.google.com/document/d/1zKfhizd0FpwoupF_jYmWkz4naTxkInI0g-rS6xzNWso/edit#heading=h.2hej6pszhyrd
There was a problem hiding this comment.
i think for batch jobs the risk is more manageable
Pull Request Summary
for notekeeping, some things to fix:
Test Plan and Usage Guide
some local test
tested running jobs in training cluster
added unit tests
vllm batch image deployed