-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Added retries parameters to pipeline.draw() #9055
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
Pull Request Test Coverage Report for Build 14076804129Details
💛 - Coveralls |
fixed too many positional arguments error
This is good, and I agree with the PR, but in the future, we probably should:
My point is that running it for every PR might be a might too much charge for mermaid servers - alternatively, we can also host a Docker with a mermaid server and run against that server |
@davidsbatista @Amnah199 We could consider using haystack/haystack/utils/requests_utils.py Line 45 in dae8c7b
|
I had another look at the PR; I overlooked the fact that retry is being added to the code that actually calls the endpoint and draws the image. Initially, I thought this was only being added to the test. IMO this retry mechanism should only be added to test and not the code of the component. |
releasenotes/notes/add-retries-parameter-to-pipeline_draw-ac1a7bc76767c450.yaml
Outdated
Show resolved
Hide resolved
…7bc76767c450.yaml Co-authored-by: Amna Mubashar <amnahkhan.ak@gmail.com>
@nickprock Thanks a lot for this great contribution — we really appreciate your continued support and engagement with Haystack! 🙌 We had an internal discussion around this issue and, at the moment, we're unsure about the best way to proceed. Introducing retries in |
@nickprock here is follow up issue: #9107 |
Hi @nickprock, thank you again for your initiative; unfortunately, two things happened. Although not explicitly stated in #9045, the idea was to add the retry to tests only, not to the code itself. Plus, we are now skipping those tests since they tend to fail and block daily PRs. As pointed out, there's another issue, #9107, to tackle this. I want to thank you once again for your initiative and contribution and feel free to suggest any suggestions on the new issue I shared. |
Thanks @davidsbatista @Amnah199 🙏 |
Related Issues
Proposed Changes:
max_retries
andinitial_delay
parameters to control retry behavior and allow more customizationHow did you test it?
unit tests
Notes for the reviewer
@anakin87 check if it's ok, you are cited in the issue.
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
and added!
in case the PR includes breaking changes.