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

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

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

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
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

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.

@davidberenstein1957
Copy link
Contributor Author

davidberenstein1957 commented Oct 9, 2023

@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
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.

@anakin87 anakin87 requested a review from ZanSara October 9, 2023 15:51
@anakin87 anakin87 merged commit 13fb7c5 into deepset-ai:main Oct 9, 2023
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:agent type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create Argilla integration for haystacknodes
5 participants