-
Notifications
You must be signed in to change notification settings - Fork 2.5k
refactor: Refactor agent breakpoint logic #10222
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Pull Request Test Coverage Report for Build 20295604111Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
…ism of Pipeline._run_component
|
Hey @mpangrazzi and @davidsbatista I'd appreciate your review since this may impact https://github.com/deepset-ai/haystack-private/issues/236 |
|
@sjrl good job!
I did a first review and noticed essentially duplication of code (I can imagine you noticed as well but wanted to first see it works) - my comments are towards solutions for those duplication. |
|
Hey @mpangrazzi this is ready for another review! |
mpangrazzi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good! I left some minor comments
mpangrazzi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Related Issues
Proposed Changes:
The way we save a pipeline and agent snapshot when an AgentBreakpoint is triggered needs to be refactored to so that using an Agent in a Pipeline with the BreakpointConfirmationStrategy in haystack-experimental works as expected.
AgentBreakpointto thePipeline.run()method we no longer auto inject the parent pipeline snapshot into the underlyingAgents arguments. Instead when an Agent triggers aBreakpointException, that exception is caught by thePipeline.runand the final pipeline snapshot construction is done at the pipeline level. This is similar to how we handled creating a pipeline snapshot when aPipelineRuntimeErroroccurred.PipelineSnapshot._from_agent_snapshotto help reduce duplicate code._should_trigger_tool_invoker_breakpointwhich is used within Agent to determine whether theToolBreakpointshould be passed to thePipeline._run_componentmethod.break_pointtoBreakpointExceptionfor easier access to the break point that caused the exception.How did you test it?
Making sure existing tests pass.
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.