Conversation
dmjoy
left a comment
dmjoy
left a comment
There was a problem hiding this comment.
Looking good so far, and good questions. Let me know if anything is still unclear. I'm assuming that the extra requirements needed for the openai dependency are fairly light? Assuming the answer is yes we can go with it for now, some part of me likes the idea of just using requests or some other generic HTTP request library to truly decouple, but that's probably not worth it.
|
|
||
| self.responses_kwargs = { | ||
| "model": self.model, | ||
| "reasoning": {"effort": "medium", "summary": "auto"}, |
There was a problem hiding this comment.
These might make more sense to go in the init args for the class, but fine to leave here for this first version.
|
@dmjoy I think this is ready for final review. Thoughts? |
dmjoy
left a comment
dmjoy
left a comment
There was a problem hiding this comment.
In general this all looks fine though I did have a few questions and minor (I think) suggestions. Also since this is an auxiliary component of the system and not necessarily / always critical path I'm not going to put too much scrutiny on it.
I would normally argue strongly for making the uv switch a separate PR, but since you need to add dependencies anyway, and I do also consider uv a strict upgrade to poetry I think it's fine to include here
| return json.loads(text_content) | ||
| else: | ||
| # Return as plain text for unstructured outputs | ||
| return {"response": text_content} No newline at end of file |
There was a problem hiding this comment.
I believe how the outlines inference engine works with the run_inference_unstructured call is it just returns strings (not dictionaries) unless this extra structure is needed here by the downstream response parsing code.
There was a problem hiding this comment.
def run_inference_unstructured(self, prompts: Union[str, list[str]]) -> Union[Dict, List[Dict]]:
So I am returning a dictionary or lis of dictionaries. Is that not right?
There was a problem hiding this comment.
Oh i see. I added the return type hint. I'll fix it
There was a problem hiding this comment.
actually for the structured output should i be returning Dialog | DialogElement objects instead of `Union[Dict, List[Dict]]?
There was a problem hiding this comment.
So you were able to successfully run against a local vLLM instance with this config (and the openai api inference code?)
I agree in principle. If you would like I can preempt this PR with another migrating to |
2 participants
Code & Config to utilize OpenAI API