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

fix: Improve robustness of get_task HF pipeline invocations #5284

Merged
merged 8 commits into from
Jul 6, 2023

Conversation

MichelBartels
Copy link
Contributor

@MichelBartels MichelBartels commented Jul 6, 2023

Related Issues

Proposed Changes:

Before this fix, the HF invocation layer was tested early which caused the process not to reach the correct invocation layer check if the HF servers were unresponsive. This PR changes the invocation order so HF is checked later and adds a timeout of 1s so it can't get stuck waiting for a response from the HF servers.

How did you test it?

I added a unit test that checks that the timeout is handled correctly. There is also a new test checking that the huggingface checks are in the last five invocation layer checks.

Notes for the reviewer

The timeout is set to 1s and can't be changed by the user. This could potentially lead to issues. I am also not sure about whether the test for the invocation layer order should be different because it does not make sure the methods are actually called in that order.

I have discussed these changes with @vblagoje who also created the original issue. I am not sure if it might make sense for him to review the PR.

Checklist

@MichelBartels MichelBartels requested a review from a team as a code owner July 6, 2023 09:46
@MichelBartels MichelBartels requested review from anakin87 and removed request for a team July 6, 2023 09:46
@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Jul 6, 2023
@MichelBartels MichelBartels requested review from anakin87 and removed request for anakin87 July 6, 2023 09:46
@coveralls
Copy link
Collaborator

coveralls commented Jul 6, 2023

Pull Request Test Coverage Report for Build 5476358114

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 44.668%

Files with Coverage Reduction New Missed Lines %
nodes/prompt/invocation_layer/hugging_face.py 10 88.06%
Totals Coverage Status
Change from base Build 5474864964: 0.01%
Covered Lines: 10283
Relevant Lines: 23021

💛 - Coveralls

@vblagoje
Copy link
Member

vblagoje commented Jul 6, 2023

All makes sense to me, except 1s timeout seems too aggressive. I'd set it to 2-3 sec. @anakin87

@vblagoje
Copy link
Member

vblagoje commented Jul 6, 2023

@MichelBartels you'll likely need to rebase to main (or merge main) as #5285 fixed the test issue.

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.

@MichelBartels, it seems like a nice hotfix to me!

I left two minor comments to improve things...

haystack/nodes/prompt/invocation_layer/hugging_face.py Outdated Show resolved Hide resolved
test/prompt/invocation_layer/test_hugging_face.py Outdated Show resolved Hide resolved
@MichelBartels MichelBartels merged commit 08f1865 into main Jul 6, 2023
47 checks passed
@MichelBartels MichelBartels deleted the fix-invocation-layer-hf-timeout branch July 6, 2023 14:33
@vblagoje
Copy link
Member

vblagoje commented Jul 6, 2023

Nice work 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve robustness of get_task HF pipeline invocations
4 participants