Skip to content

feat: added on_agent_final_answer-support to Agent callback_manager#5736

Merged
anakin87 merged 14 commits intodeepset-ai:mainfrom
davidberenstein1957:main
Oct 9, 2023
Merged

feat: added on_agent_final_answer-support to Agent callback_manager#5736
anakin87 merged 14 commits intodeepset-ai:mainfrom
davidberenstein1957:main

Conversation

@davidberenstein1957
Copy link
Contributor

@davidberenstein1957 davidberenstein1957 commented Sep 7, 2023

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
Loading

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

davidberenstein1957 commented Sep 11, 2023

@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
Comment on lines +295 to +297
def on_agent_final_answer(final_answer: Dict[str, Any], **kwargs: Any) -> None:
pass

Copy link
Contributor

@ZanSara ZanSara Sep 13, 2023

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

@davidberenstein1957 davidberenstein1957 Sep 18, 2023

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

davidberenstein1957 commented Sep 18, 2023

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
Loading

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

anakin87 commented Sep 27, 2023

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

davidberenstein1957 commented Sep 27, 2023

@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

anakin87 commented Sep 27, 2023

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
Loading

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anakin87 anakin87 anakin87 approved these changes

@dfokina dfokina Awaiting requested review from dfokina dfokina was automatically assigned from deepset-ai/documentation

+1 more reviewer

@ZanSara ZanSara ZanSara approved these changes

Reviewers whose approvals may not affect merge requirements

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