Minimal edit to enable Deepseek prefilling #96
Conversation
| api_func = self.aclient.chat.completions.create | ||
| if model_id in {"deepseek-chat", "deepseek-reasoner"}: | ||
| if prompt.is_last_message_assistant(): | ||
| self.aclient.base_url = "https://api.deepseek.com/beta" |
There was a problem hiding this comment.
should you have an else here to swap back to the other version or swap back to non beta after the call is successful or something?
|
|
||
| original_base_url = self.aclient.base_url | ||
| try: | ||
| if model_id in {"deepseek-chat", "deepseek-reasoner"}: |
There was a problem hiding this comment.
Do we have a DEEPSEEK_MODELS dict somewhere?
There was a problem hiding this comment.
We have one in safety-tooling/safetytooling/apis/inference/api.py, but that would result in a circular import. We can create a constants file somewhere maybe?
| finally: | ||
| # Always revert the base_url after the call | ||
| self.aclient.base_url = original_base_url |
There was a problem hiding this comment.
one thought - could this have strange async race conditions :/
There was a problem hiding this comment.
hmm, maybe the base url should be passed directly to the api_func on a call-wise basis (rather than setting it as an attribute of the entire class). Since the class itself could be handling many requests with different models (and even different providers if it was set up differently).
There was a problem hiding this comment.
ah that's true, should we have an asyncio lock maybe?
There was a problem hiding this comment.
base url should be passed directly to the api_func on a call-wise basis
The api_func doesn't accept base url unfortunately. And i guess locking would harm concurrency..
Another (naive) approach is to instantiate the api_func again and again (instantiate openai.AsyncClient).
Would you be against locking?
There was a problem hiding this comment.
I think locking would mess things up in terms of throughput since it would lock until the async call is complete which would be bad. Perhaps you can override the URL via "extra_headers"?
There was a problem hiding this comment.
I'm still a little worried about this. Can we just use "https://api.deepseek.com/beta" always? Then we can set in api.py and remove all this logic internally of needing to swap between
There was a problem hiding this comment.
what ended up happening here?
Summary
Reincorporating the edits made in #75 to allow prefilling deepseek models.
This was not handled in the latest update and if I were to run a request with a prefilling, the code would throw an error asking to use
is_prefix=True. Once I set that parameter, the code would ask me to use the beta url.Changes Introduced
safetytooling/apis/inference/openai/chat.pydeepseek-reasonerordeepseek-chat, use theprompt.deepseek_formatfunction to prepare the prompt.