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

add tests to trajectory eval chain #8909

Merged

Conversation

shibuiwilliam
Copy link
Contributor

What

  • add tests to trajectory eval chain

@vercel
Copy link

vercel bot commented Aug 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Aug 9, 2023 8:20am

@dosubot dosubot bot added Ɑ: agent Related to agents module 🤖:improvement Medium size change to existing code to handle new use-cases labels Aug 8, 2023
@shibuiwilliam shibuiwilliam marked this pull request as ready for review August 8, 2023 11:16
) # Scan for first digit

if not 1 <= int(score_str) <= 5:
# Use regex to extract the score
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Would be useful to document the intended behavior to help verify implementation.

Is the reason for the change to change missing score into a None rather than a 0 ?

If so, could we keep the previous logic and replace like so:

score_str = next(
            (char for char in score_str if char.isdigit()), None
)  # Scan for first digit

Copy link
Contributor Author

@shibuiwilliam shibuiwilliam Aug 9, 2023

Choose a reason for hiding this comment

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

Thanks!
This change is intended to take care of cases where LLM makes invalid score like 1.1 or 20.
next((char for char in score_str if char.isdigit()), None) will return "1" for 1.1 and "2" for 20 which is invalid score.
My change will validate these case as error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added explanation on the code comment.

@eyurtsev
Copy link
Collaborator

eyurtsev commented Aug 8, 2023

Thanks for the change and the tests. Do you mind including somewhere an explanation of what behavior this PR modifies?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ɑ: agent Related to agents module 🤖:improvement Medium size change to existing code to handle new use-cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants