Skip to content

fix: Error in obtaining variables for character settings #1948

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.

Sign up for GitHub

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

Merged
shaohuzhang1 merged 1 commit into from
Dec 30, 2024
+2 −2
Merged

fix: Error in obtaining variables for character settings #1948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/application/flow/step_node/ai_chat_step_node/impl/base_chat_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ def execute(self, model_id, system, prompt, dialogue_number, history_chat_record
question = self.generate_prompt_question(prompt)
self.context['question'] = question.content
system = self.workflow_manage.generate_prompt(system)
self.context['system'] = question.content
self.context['system'] = system
message_list = self.generate_message_list(system, prompt, history_message)
self.context['message_list'] = message_list
if stream:
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided code snippet is generally clean and concise. However, there's one small issue that could be improved:

# Potential issue: In the line where 'self.context['system']' is being overwritten,
# it might be more appropriate to set it based on the current context rather than always overwriting it with `question.content`.

# Optimization suggestion: Consider handling potential exceptions when trying to access or modify context variables.

This comment suggests modifying the last assignment to use the updated system content instead of directly setting self.context['system'] to question.content, which can prevent unexpected behavior if the context contains other relevant information that needs to be preserved. Additionally, adding error handling for accessing/modifying context variables can make the code more robust.

Expand Down
2 changes: 1 addition & 1 deletion apps/application/flow/step_node/question_node/impl/base_question_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def execute(self, model_id, system, prompt, dialogue_number, history_chat_record
question = self.generate_prompt_question(prompt)
self.context['question'] = question.content
system = self.workflow_manage.generate_prompt(system)
self.context['system'] = question.content
self.context['system'] = system
message_list = self.generate_message_list(system, prompt, history_message)
self.context['message_list'] = message_list
if stream:
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no apparent issue with this code. However, there are a couple of small improvements that can be made for readability and maintainability:

  1. Consistent Use of content: Since you're already referring to the system.content, it would make sense to consistently use .content throughout.

  2. Use of Variables For Clarity: While current approach works, using variables can enhance clarity:

      question = self.generate_prompt_question(prompt)
  self.context['question'] = question.content

  # Using 'self.system' to avoid redundancy
  system_content = self.workflow_manage.generate_prompt(system).content
  self.context['system'] = system_content

  message_list = self.generate_message_list(system, prompt, history_message)
  self.context.update({'message_list': message_list})

These changes improve consistency within your codebase and potentially reduce potential errors or misunderstandings down the line.

Expand Down