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

Fix bug with Agents for the enable_trace events #254

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

jdbaker01
Copy link
Collaborator

@jdbaker01 jdbaker01 commented Oct 24, 2024

The trace_log was either always empty or only showed the last one because the events come through in succession and the last one was being returned only. Changed the trace log variable to a list and append new events then at the end of of the loop serialize the array to a string.

Added new unit tests for our different output types and to check the existence of the trace log.

Fixes bug introduced in #244.

…riting the log output.

Changed to an array from a string and moved json string conversation to outside the loop.
Add prerequisite instructions to the Jupyter notebook example.
Add unit tests.
Fix lint issues.
@rsgrewal-aws
Copy link

rsgrewal-aws commented Oct 24, 2024

@3coins could you please review this PR

@3coins
Copy link
Collaborator

3coins commented Oct 24, 2024

@jdbaker01
Thanks for this fix. Code looks good, a few minor things.

  1. Can you fix the lint error. Running make format and make lint should fix them.
    tests/unit_tests/agents/test_bedrock_agents.py:49: error: X | Y syntax for unions requires Python 3.10  [syntax]
    tests/unit_tests/agents/test_bedrock_agents.py:57: error: X | Y syntax for unions requires Python 3.10  [syntax]
  2. langchain-aws and langchain repos use pytest as the test runner, there is extra setup main and run_tests that are redundant if you convert these tests to pytest format with just functions. Use fixtures instead of the setup function. Let me know if you need help with this.

Copy link
Collaborator

@3coins 3coins left a comment

Choose a reason for hiding this comment

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

LGTM!

@3coins 3coins merged commit fd3c820 into langchain-ai:main Oct 24, 2024
12 checks passed
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.

4 participants