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

fake agent allowing timeouts or exceptions, #672

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Conversation

jamesbraza
Copy link
Collaborator

Easier to read diff: https://github.com/Future-House/paper-qa/compare/standard-rollouts?expand=1&w=1

This PR:

  • Creates a helper function run_with_timeout_failure that runs a rollout with AgentStatus.TRUNCATED (timeout) or AgentStatus.FAIL (Exception) support
  • Moves all three runners (fake, aviary, LDP) to use it
    • Now the fake agent supports timeouts or failures 🥳
  • Adds AgentStatus.TRUNCATED support for the LDP agent runner

@jamesbraza jamesbraza added the enhancement New feature or request label Nov 8, 2024
@jamesbraza jamesbraza self-assigned this Nov 8, 2024
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Nov 8, 2024
Comment on lines 362 to 364
if traj.steps[-1].truncated:
nonlocal status
status = AgentStatus.TRUNCATED
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is confusing - can you just return status in rollout? This modification is like three levels deep and I'm actually unclear on if it always works

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Believe it or not, locally I have this exact change in the works

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, done. Good to see we are both still thinking

Copy link
Collaborator

@whitead whitead left a comment

Choose a reason for hiding this comment

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

cool

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 8, 2024
@jamesbraza jamesbraza merged commit 79ee951 into main Nov 8, 2024
5 checks passed
@jamesbraza jamesbraza deleted the standard-rollouts branch November 8, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants