-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: The tag based thinking process does not output #2968
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
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[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 |
additional_kwargs: dict = {'reasoning_content': reasoning_content} | ||
additional_kwargs: dict = {} | ||
if 'reasoning_content' in _dict: | ||
additional_kwargs['reasoning_content'] = _dict.get('reasoning_content') | ||
if _dict.get("function_call"): | ||
function_call = dict(_dict["function_call"]) | ||
if "name" in function_call and function_call["name"] is None: |
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.
The provided code snippet seems to be part of a larger function _convert_delta_to_message_chunk
responsible for converting various types into BaseMessageChunk
objects. Here's an analysis along with some suggestions:
-
Variable Naming:
- The variable named
additional_kwargs
might be confusing, since it could potentially include other keyword arguments related to different message chunks types.
- The variable named
-
Conditional Check:
- The condition
'reasoning_content' in _dict:
checks if the key is present before attempting to access the value. This is good practice as it prevents accessing keys that do not exist.
- The condition
-
Default Keyword Argument:
- Adding
"default_class=type(BaseMessageChunk)
as an argument to the function does no harm but might obscure intent slightly. If this line was removed, clarity would increase slightly without changing behavior.
- Adding
-
Dictionary Initialization:
- Initializing
additional_kwargs as {}
at the top ensures that dictionary creation occurs only once, which is preferable for performance reasons when dealing with large collections.
- Initializing
-
Function Call Handling:
- Ensuring that the
function_call.name
is either None or properly handled should ideally be done more robustly within the broader logic context where messages are being constructed.
- Ensuring that the
-
Overall Clarity:
- The use of explicit casts (
cast(str,)
) and inline comments can help maintain clarity on data conversion and handling.
- The use of explicit casts (
Here’s a revised version of the code incorporating these suggestions:
@@ -24,10 +24,12 @@ def _convert_delta_to_message_chunk(
base_cls: type[BaseMessageChunk],
) -> BaseMessageChunk:
"""Converts delta-dictionary data structures into corresponding MessageChunk instances."""
id_ = _dict.get("id", None) # Default value set to None instead of "" for clarity
role = cast(str, _dict.get("role"))
content = cast(str, _dict.get("content", ""))
addition_kwargs: dict = {}
# Only add reasoning_content if it exists in the dictionary
if "reasoning_content" in _dict:
addition_kwargs["reasoning_content"] = _dict.get("reasoning_content")
if "function_call" in _dict:
function_call_data = _dict["function_call"].copy() # Copy to avoid mutating original
function_name = function_call_data.pop("name", None)
# Additional processing or validation here based on requirements
function_call = {"name": function_name} # Create new dictionary containing name
addition_kwargs.update(function_call)
chunk_type = "regular" if not function_call else "function"
try:
return base_cls(id=id_, role=role, content=content, **addition_kwargs)
except TypeError as e:
raise ValueError(f"Failed to create {chunk_type} message due to invalid parameters.") from e
This revision improves readability, adds missing documentation where necessary, fixes unnecessary initializations, and clarifies certain details such as handling default values and creating new dictionaries.
fix: The tag based thinking process does not output