Skip to content
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

make defOutputProcessor more flexible #634

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 20, 2024

  • A change in the way errors are logged was made in packages/core/src/trace.ts file 💼

    • The renderError function now always details errors, a change from the previous value of false.
    • The error message string now incorporates both message & emsg as a single string separated by a comma, unlike previously, it was one or the other.
  • Adjustments were implemented in packages/core/src/types/prompt_template.d.ts 🧾

    • Introduced the possibility of the PromptOutputProcessorHandler type to be void or Promise<void>. This is potentially user-facing as it alters the public API.
  • A substantial change was made to packages/sample/genaisrc/output.genai.js 🔄

    • Added a model field and updated the files and tests fields.
    • Changed defOutputProcessor function to be async.
    • Added another defOutputProcessor function block that logs out a text.

By the look of it, the main objective of these changes appears to be the enhancement of error logging and improvements in the tasks undertaken by output processors.

generated by pr-describe

Copy link

The changes in this pull request involve two primary modifications:

  1. The renderError method in trace.ts now shows details of the error by setting the details property to true. This can be beneficial in providing more comprehensive error diagnostics.
  2. The message in the error handling logic is now a combination of message and emsg, separated by a comma. This could improve the clarity of the error messages as it accumulates all relevant information.
  3. The PromptOutputProcessorHandler type in prompt_template.d.ts now includes void and Promise<void>. This could potentially introduce new behaviors or handle additional cases.

Overall, these changes seem reasonable but there's a potential concern:

  • Including both message and emsg in the error message could potentially lead to redundant or confusing information if they often contain similar details.

Based on the given diff, it's hard to ascertain the full impact of these changes without additional context such as related functions or usage scenarios. However, as long as the above concern is addressed and the changes have been properly tested, it should be good to go.

Response: "Looks generally good, but please ensure that combining message and emsg doesn't lead to redundancy or confusion in the error messages. ⚠️"

generated by pr-review

@pelikhan pelikhan merged commit aeabd7a into main Aug 20, 2024
10 checks passed
@pelikhan pelikhan deleted the outputfileprocessorvoid branch August 20, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant