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

Address missing GenerateAnswer in trajectories, no answers after Complete tools, and better history #726

Merged
merged 5 commits into from
Nov 27, 2024

Conversation

mskarlin
Copy link
Collaborator

  • Moved the tool_history into session -- so it can be externally accessed for tests + debugging.
  • Added a history check for gen_answer in runner, it uses the history to ensure it's there.
  • Added a sentinel into complete tool, this will allow trajectories where this tool is used outside of the runner to have something populated in the answer.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Nov 27, 2024
paperqa/agents/tools.py Outdated Show resolved Hide resolved
paperqa/agents/tools.py Outdated Show resolved Hide resolved
paperqa/agents/main.py Show resolved Hide resolved
paperqa/agents/tools.py Outdated Show resolved Hide resolved
paperqa/agents/tools.py Outdated Show resolved Hide resolved
paperqa/agents/tools.py Outdated Show resolved Hide resolved
paperqa/agents/tools.py Show resolved Hide resolved
Copy link
Collaborator

@jamesbraza jamesbraza left a comment

Choose a reason for hiding this comment

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

LGTM, and a Thanksgiving approval

@@ -357,6 +354,8 @@ class Complete(NamedTool):
r" \| " + EnvironmentState.STATUS_SEARCH_REGEX_PATTERN
)

NO_ANSWER_PHRASE: ClassVar[str] = "No answer generated."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we move this to PQASession since that's where it will be used most likely? Imo housing this in the Complete tool makes it less accessible (since Complete tool is sort of an agent implementation detail)

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 27, 2024
@mskarlin
Copy link
Collaborator Author

Note I'm merging w.o CI passing because we seem to still be getting 502s from openAI

@mskarlin mskarlin merged commit fc710c5 into main Nov 27, 2024
3 of 5 checks passed
@mskarlin mskarlin deleted the no-success-answer branch November 27, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants