-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: The OpenAI interface supports the thought process #2392
Conversation
created=datetime.datetime.now().second, choices=[ | ||
Choice(delta=ChoiceDelta(content=content, reasoning_content=other_params.get('reasoning_content', ""), | ||
chat_id=chat_id), | ||
finish_reason='stop' if is_end else None, | ||
index=0)], | ||
usage=CompletionUsage(completion_tokens=completion_tokens, | ||
prompt_tokens=prompt_tokens, |
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.
Regularity Checks:
-
Initialization of
other_params
:- Both methods use
if other_params is None
to initialize it. This is good practice as it checks whether the parameter is provided and provides default values accordingly.
- Both methods use
-
Stream Response (
to_stream_chunk_response
):- The
choices
list remains unchanged, except for adding an additional field:reasoning_content
with a default value if not provided inother_params
.
- The
Potential Issues:
-
Type Hinting:
- Ensure that all type hints within the function definitions match their respective types. For instance,
_status
should be of typeint
. - Consider using more specific annotations like
Union[None, Dict[str, Any]]
instead of justDict[str, Any]
.
- Ensure that all type hints within the function definitions match their respective types. For instance,
-
Datetime Module Usage:
- In Python 3.x, you can import datetime directly from the module without specifying
datetime.
at the beginning. This aligns well with PEP 8 guidelines but could vary slightly based on Python versions used.
- In Python 3.x, you can import datetime directly from the module without specifying
Optimization Suggestions:
-
Use Type Annotations for Methods:
from typing import Set class OpenaiToResponse(BaseToResponse): def to_block_response( self, chat_id: str, chat_record_id: str, content: str, is_end: bool, completion_tokens: int, prompt_tokens: int, other_params: dict = None, # Default is an empty dict _status: int = status.HTTP_200_OK ) -> JsonResponse: pass def to_stream_chunk_response( self, chat_id: str, chat_record_id: str, node_id: str, up_node_id_list: Set[str], # Assuming this expects a set of strings content: str, is_end: bool, completion_tokens: int, prompt_tokens: int, other_params: dict = None ) -> None: pass
-
Consider Using Named Tuples or Data Classes:
from dataclasses import dataclass
@dataclass
class CompletionData:
id: str
model: str
object: str
created: float
choices: List[Choice]
usage: 'CompletionUsage'
@dataclass
class Choice:
delta: ChoiceDelta
finish_reason: Union[None, str]
@dataclass
class ChoiceDelta:
content: str
reasoning_content: str
@dataclass
class CompletionUsage:
completion_tokens: int
prompt_tokens: int
By implementing these suggestions, code will become more robust, maintainable, and efficient while adhering to best practices.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
feat: The OpenAI interface supports the thought process