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.

Sign up for GitHub

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

Jump to bottom

feat: added on_agent_final_answer-support to Agent callback_manager #5736

Merged
merged 14 commits into from
Oct 9, 2023
Merged

feat: added on_agent_final_answer-support to Agent callback_manager #5736

merged 14 commits into from
Oct 9, 2023

Conversation

davidberenstein1957
Copy link
Contributor

Related Issues

Proposed Changes:

added on_agent_final_answer-support to Agent callback_manager

How did you test it?

N.A.

Notes for the reviewer

@anakin87

Checklist

@davidberenstein1957 davidberenstein1957 requested a review from a team as a code owner September 7, 2023 09:03
@davidberenstein1957 davidberenstein1957 requested review from ZanSara and removed request for a team September 7, 2023 09:03
@CLAassistant
Copy link

CLAassistant commented Sep 7, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@anakin87
Copy link
Member

anakin87 commented Sep 7, 2023

Hello, @davidberenstein1957 and welcome!

  • Black is set with a line length of 120 characters. You can let pre-commit fix the style for you.
  • You should add a (release) note to this PR. You can find more information here.

@davidberenstein1957 davidberenstein1957 requested a review from a team as a code owner September 11, 2023 19:44
@davidberenstein1957 davidberenstein1957 requested review from dfokina and removed request for a team September 11, 2023 19:44
@davidberenstein1957
Copy link
Contributor Author

@anakin87 I ran the required instructions

ZanSara
ZanSara suggested changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Left a couple of nitpicks, otherwise this PR only lacks tests. Could you add some?

haystack/agents/base.py Outdated Show resolved Hide resolved
haystack/agents/base.py
Comment on lines +295 to +297
def on_agent_final_answer(final_answer: Dict[str, Any], **kwargs: Any) -> None:
pass

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's provide a default implementation like for all the others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZanSara what do you suggest? I did not want to do this because it might mess with the current use-cases of people.

@github-actions github-actions bot added the type:documentation Improvements on the docs label Sep 18, 2023
@davidberenstein1957
Copy link
Contributor Author

Left a couple of nitpicks, otherwise this PR only lacks tests. Could you add some?

@ZanSara I wanted to implement this in a more sustainable way but was urged to wrap this up as a quick and dirty fix. Also, there don't seem to be any other tests for the callback_manager yet so I think these can be skipped for now. What do you think @anakin87. If not, I would instead implement a proper class as proposed here.

@coveralls
Copy link
Collaborator

coveralls commented Sep 18, 2023 •
edited

Pull Request Test Coverage Report for Build 6458327639

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 50.341%
Files with Coverage Reduction New Missed Lines %
agents/base.py 5 95.93%
Totals Coverage Status
Change from base Build 6457280690: 0.02%
Covered Lines: 12629
Relevant Lines: 25087
💛 - Coveralls

@anakin87
Copy link
Member

Hey @davidberenstein1957, sorry to keep you waiting...

Although untested, this PR looks very simple and good to me.

To make sure we don't break things, could you reproduce this tutorial using your branch?

It would be nice if you could attach the resulting notebook to this PR...

@davidberenstein1957
Copy link
Contributor Author

@anakin87, thanks for the guidance. I will make sure to test it and add the resulting notebook to this PR. Do you want me to update the notebook along with generated outputs or is a reference via Colab fine?

@anakin87
Copy link
Member

Thanks!

A reference via Colab is fine.

@anakin87 anakin87 mentioned this pull request Sep 28, 2023
Merged
@davidberenstein1957
Copy link
Contributor Author

davidberenstein1957 commented Oct 9, 2023 •
edited

@anakin87, I finally managed to find some time to work on this.
https://colab.research.google.com/drive/15Q-7z6ZANrLrywjjDDZe0cYTof_v26kM?usp=sharing

@anakin87 anakin87 self-requested a review October 9, 2023 15:50
anakin87
anakin87 approved these changes Oct 9, 2023
View reviewed changes
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Thanks @davidberenstein1957!

@anakin87 anakin87 requested a review from ZanSara October 9, 2023 15:51
@anakin87 anakin87 merged commit 13fb7c5 into deepset-ai:main Oct 9, 2023
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZanSara ZanSara ZanSara approved these changes

@anakin87 anakin87 anakin87 approved these changes

@dfokina dfokina Awaiting requested review from dfokina dfokina is a code owner automatically assigned from deepset-ai/documentation

Assignees
No one assigned
Labels
topic:agent type:documentation Improvements on the docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

create Argilla integration for haystacknodes
5 participants
@davidberenstein1957 @CLAassistant @anakin87 @coveralls @ZanSara