feat: Non streaming response answers_list

[shaohuzhang1]

feat: Non streaming response answers_list

[f2c-ci-robot]

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.

Details

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.

[f2c-ci-robot]

[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.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shaohuzhang1]

The provided code appears to be part of a language model's execution logic where it processes answers and streams responses. Here are some observations and suggestions:

1. Empty String Check: The answer_text variable is initialized as an empty string, but this might not be intended if you expect it to hold the final response content.

2. Reduce Function Overhead: Using the reduce function with a lambda expression seems unnecessary here since join() can be more efficient for concatenating lists into strings.

3. Multiple Join Calls: You have multiple calls to join(), which could lead to redundancy and performance issues. Consider combining these joins into a single call.

4. Default Argument Types: The type hints for parameters like _status suggest they should be expected to accept integers rather than enums/types that match Django's HTTP status codes (e.g., http.HttpResponse). Ensure correct typing.

Here’s an optimized version of the code based on these points:

@@ -337,26 +337,28 @@ def run_block(self, language='zh'):
         # Combine all answers into a single list without using reduce
         answer_texts = [a.get('content') for sublist in answer_text_list for a in sublist]
         
         # Join all parts into one continuous string separated by two newlines
         answer_text = '\n\n'.join(answer_texts)
         
         # Initialize other required variables (assumed present elsewhere in the code)
         self.work_flow_post_handler.handler(
             params['chat_id'],
             params['chat_record_id'],
-            answer_text,
+            answer_texts,
             self
         )
        
        return self.base_to_response.to_block_response(
            params['chat_id'],
            params['chat_record_id'],
            answer_text,
            True,
            message_tokens,
            answer_tokens,
-            _status=status.HTTP_200_OK if self.status == 200 else status.HTTP_500_INTERNAL_SERVER_ERROR
+            _status=status.HTTP_200_OK if self.status == 200 else status.HTTP_500_INTERNAL_SERVER_ERROR,
+            other_params={'answer_list': answer_texts}
        )

     def run_stream(self, current_node, node_result_future, language='zh'):
         """

Notes:

• Replace params[...] with actual context-dependent parameter names from your application.
• Ensure _status is correctly passed along if necessary, possibly by modifying self.params before passing to to_block_response.

[shaohuzhang1]

The provided code has a few minor issues:

1. Indentation: The except block's indentation is inconsistent with the rest of the method.

2. Variable Naming: The variable all_text should likely be named more clearly, especially if it is intended to store detailed error messages rather than just exception strings.

3. String Formatting: When logging the exception text, using 'Exception:' + str(e) can lead to errors if the exception object does not support string formatting. It would be safer to use f-strings (f'Exception: {str(e)}') if available.

4. Return Type: The function signature specifies returning Union[MessageBlockResponse, BaseMessage], but there seems to be a logical mismatch between this specification and the actual return value being returned by the function (a Dict[str, Any]). Ensure that the return type matches the expected output.

Here's a revised version of the code with some addressing these issues:

from typing import List, Dict, Union, NoReturn

class ChatHandler:
    def execute_block(self, message_list: List[BaseMessage],
                          chat_id: str,
                          chat_record_id: int) -> Union[MessageBlockResponse, BaseMessage]:
        try:
            response_dict = manage.get_base_to_response().to_block_response(
                str(chat_id), str(chat_record_id),
                content, True,
                request_token, response_token,
                {'reasoning_content': reasoning_content}
            )
            # Assuming response_dict is expected to be used elsewhere in your application
            return response_dict
        except Exception as e:
            all_text = f"Error: {e}"
            write_context(self, manage, 0, 0, all_text)

Explanation:

• Corrected Indentation: Ensured consistent indentation throughout the execute_block method.
• Detailed Error Logging: Used an f-string for logging to improve readability and safety.
• Consistent Return Value: Kept the return statement aligned with the specified union type, assuming response_dict follows that pattern.
• Type Hinting: Added explicit type hints where appropriate.

These changes enhance code clarity and maintainability while ensuring correct behavior.

[shaohuzhang1]

There doesn't appear to be any significant errors in the code provided. However, there are some minor optimizations for readability and performance:

1. Reduce Function Optimization: The reduce function is not necessary here because you are already working with lists of answers directly. You can simplify the line where answer_list is created.

2. Code Style Improvements: Ensure consistent spacing around operators and keywords (e.g., using spaces after commas and colons).

Here's an optimized version of the code based on these improvements:

def run_block(self, language='zh'):
    answer_text = '\n\n'.join('\n\n'.join(a.get('content') for a in answer) for answer in answer_text_list)
    self.work_flow_post_handler.handler(
        self.params['chat_id'],
        self.params['chat_record_id'],
        answer_text,
        self
    )
    # Return response without modifying the original text
    return self.base_to_response.to_block_response(
        self.params['chat_id'],
        self.params['chat_record_id'],
        answer_text,
        True,
        message_tokens,
        answer_tokens,
        _status=status.HTTP_200_OK if self.status == 200 else status.HTTP_500_INTERNAL_SERVER_ERROR,
        other_params={'answer_list': answer_text_list}
    )

def run_stream(self, current_node, node_result_future, language='zh'):
    """

These changes improve both readability and maintainability while removing unnecessary steps. Let me know if you need further adjustments!

[shaohuzhang1]

The provided code is mostly clean and functional. However, it can be optimized for readability and potential improvements based on future needs:

1. Indentation: Ensure consistent indentation levels. The line return ... should be aligned with the preceding colon.

2. Comments: Adding comments where necessary to explain complex parts of the function might help clarify its purpose and functionality.

3. Dictionary Structure: While the structure doesn't have significant issues at present, ensuring data consistency could make debugging easier in the future. Consistency in how keys are formatted (e.g., camelCase vs. snake_case) may also improve maintainability.

Here's a revised version with some minor adjustments:

def execute_block(self, message_list: List[BaseMessage],
                     chat_id: str = None,
                     chat_record_id: int = None):
    try:
        if chat_id and chat_record_id:
            result = manage.get_base_to_response().to_block_response(
                chat_id=chat_id,
                record_id=str(chat_record_id),
                content=message_list,
                is_system=True,
                request_token="",
                response_token="",
                context={'reasoning_content': '', 'answer_list': [{'content': content, 'reasoning_content': reasoning_content}]}
            )
        return result
    except Exception as e:
        all_text = f'Exception:{str(e)}'
        write_context(self, manage, 0, 0, all_text)

Optimization Suggestion:
To enhance performance or add more features later, consider organizing your code into smaller functions that each handle specific tasks, rather than having a single method doing too much. This approach improves modularity, makes the code easier to read, and maintains cleaner separation between concerns.

[shaohuzhang1]

There is an issue with the line self.answer_list = reduce(lambda pre, _n: [*pre, *_n], answer_text_list, []). This will raise a TypeError because you can't use unpacking (*) directly on strings in Python. A more appropriate approach would be to join the list of answers into a single string before reducing it. Here's the corrected version:

answer_list = ','.join(answer.get('content') for answer in answer_text_list)

# Then, proceed with the rest of the code...

If you need to maintain the list format after joining, consider converting it back to a list before using reduce:

answer_list = [answer.get('content') for answer in answer_text_list]

[shaohuzhang1]

from django.core.cache import cache

# alg使用的算法
HEADER = {'type': 'JWP', 'alg': 'default'}
TOKEN_KEY = 'solomon_world_token'
TOKEN_SALT = 'solomonwanc@gmail.com'
TIME_OUT = 30 * 60

The provided code appears to be correctly formatted and free of syntax errors. Here are a few minor suggestions for improvement:

1. Consistency: Ensure that TYPE instead of type is used for the key 'type', which might prevent conflicts with other data types.

HEADER = {
    'typ': 'JWP',
    'alg': 'default'
}

2. Variable Naming:
   • Use consistent naming conventions (e.g., all uppercase or all lowercase) across related variables like TOKEN_KEY.

HEADER = {'typ': 'JWP', 'alg': 'default'}
TOKEN_KEY = 'SOLOMON_WORLD_TOKEN'  # Consistent capitalization
TOKEN_SALT = 'soLomonWANc@gmail.com'  # Lowercase email addresses are common practice for keys/salts
TIME_OUT = 30 * 60

Additionally, consider if there's any specific context where using 'solomonwanc@gmail.com' as the token salt is necessary. If not, you can use a more secure string, preferably one generated randomly and stored securely elsewhere.

These changes will improve consistency and security in your codebase.

[shaohuzhang1]

The code is mostly correct, but there are a couple of potential issues:

1. Handling Exceptions: The exception handling does not specify which exceptions should be caught or handled differently. It might be beneficial to wrap specific types of exceptions that are expected during execution.

2. Response Structure: Adding a list of answer dictionaries within manage.get_base_to_response() seems redundant if you're already including it in the final dictionary being returned. This could be simplified by removing one of the instances.

Here's the modified version of the function with these considerations:

def execute_block(self, message_list: List[BaseMessage],
                     chat_id: int,
                     chat_record_id: int,
                     content: str,
                     request_token: str,
                     response_token: Optional[str] = None) -> ResponseBlockData:
    try:
        # Construct the data payload for managing responses
        base_data_payload = {
            'chat_id': chat_id,
            'chat_record_id': chat_record_id,
            'content': content,
            'request_token': request_token,
            'response_token': response_token,
            'reasoning_content': reasoning_content,
            'answer_list': [{'content': content, 'reasoning_content': reasoning_content}]}
        
        # Get the block response using the constructed data
        return manage.get_base_to_response().to_block_response(str(chat_id), str(chat_record_id),
                                                            base_data_payload['content'],
                                                            True,
                                                            request_token, response_token,
                                                            {'reasoning_content': reasoning_content})
    
    except Exception as e:
        all_text = f'Exception:{str(e)}'
        write_context(self, manage, 0, 0, all_text)

Key Changes Made:

• Combined the logic inside the try block to minimize redundancy.
• Removed unnecessary indentation from the call to write_context.
• Used string formatting instead of concatenation for better readability and performance on long strings.

[shaohuzhang1]

This code looks mostly correct, but there are a few minor points that can potentially be improved:

1. String Formatting: Consider using an f-string for more readable formatting of the answer_text variable.

   answer_text = '\n\n'.join(
       '\n\n'.join([a.get('content') for a in answer]) for answer in answer_text_list).rstrip()

2. Variable Naming Consistency: The name _n is used in the list comprehension inside the call to reduce, which suggests that you plan to use it as a counter. It's good practice to avoid using underscores at the start of variable names unless they are part of a specific naming convention (e.g., loop variables).

3. Return Statement Clarity: Ensure that the return statement includes all required parameters even if some are optional or default values.

Here’s the updated code with these improvements:

@@ -337,13 +337,15 @@ def run_block(self, language='zh'):
         answer_text = '\n\n'.join(
             '\n\n'.join(a.get('content') for a in answer) for answer in answer_text_list).rstrip()
+
+        # Using enumerate to get both index and value for better readability
+        answer_list = [item for item in answer_text_list]
         
         self.work_flow_post_handler.handler(self.params['chat_id'], self.params['chat_record_id'],
                                             answer_text,
                                             self)
         return (
             self.base_to_response.to_block_response(self.params['chat_id'],
                                                     self.params['chat_record_id'], answer_text, True,
-                                                    message_tokens, answer_tokens,
-                                                    _status=status.HTTP_200_OK if self.status == 200 else status.HTTP_500_INTERNAL_SERVER_ERROR))
+                                                    message_tokens, answer_tokens,
+                                                    _status=status.HTTP_200_OK if self.status == 200 else status.HTTP_500_INTERNAL_SERVER_ERROR,
+                                                    other_params={'answer_list': answer_list})
 
     async def run_stream(self, current_node, node_result_future, language='zh'):
         """

These changes will improve the legibility, consistency, and possibly performance slightly, especially when dealing with larger data sets. However, for production-level applications, additional optimizations like caching or parallel processing might be considered depending on the workload.

[shaohuzhang1]

The code you provided has one issue that needs to be addressed:

• Typo correction: In the HEADER dictionary, there is a typo with the key 'type' instead of 'typ'. This will not affect your application but should be corrected.

In addition, here are some minor improvements or considerations for future maintenance and readability:

1. Consistent naming conventions: While both typ and type work correctly, using the standard Python convention (lowercase_with_underscore) is preferred.

   Corrected code:

   HEADER = {'type': 'JWP', 'algorithm': 'default'}

2. Code formatting consistency:

   Ensure consistent indentation throughout the file when adding new lines like setting TOKEN_KEY.

3. Docstring comments (if applicable):

   If this class contains methods related to tokens or JWTs, consider adding docstrings explaining what each method does for better understanding and documentation purposes.

4. Keep up-to-date cache configurations:

   Depending on your Django project's caching system settings in settings.py, ensure the TTL (Time To Live) used by the cache matches TIME_OUT.

Overall, the code looks functional and follows good practices regarding naming and structure. The mentioned typo can safely be resolved without affecting its functionality.