Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage reportClick to see where and how coverage changed
The report is truncated to 25 files out of 26. To see the full report, please visit the workflow summary page. This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sjrl
left a comment
There was a problem hiding this comment.
Looks good! Would be nice if @mpangrazzi could also give it a lookover before merging
mpangrazzi
left a comment
There was a problem hiding this comment.
Looks good! I've left a minor comment, your choice if to handle it or not.
| """ | ||
| Warms up the SuperComponent by warming up the wrapped pipeline on the serving event loop. | ||
| """ | ||
| if not self._warmed_up: |
There was a problem hiding this comment.
Minor thing, but I was thinking that warm_up_async and warm_up are relying on a single self._warmed_up flag.
One could expect that warm_up warms up sync resources (ie sync client), and warm_up_async the async ones. So this single flag could be a bit inconsistent with other components behaviour. WDYT?
There was a problem hiding this comment.
Good point. I'll fix this.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Related Issues
Proposed Changes:
Discussed in https://github.com/deepset-ai/haystack-private/issues/384#issuecomment-4740431488
How did you test it?
Notes for the reviewer
This PR is big. I created a single one to test everything.
Once we agree on the general approach, I'd be happy to split it into smaller parts if you prefer.
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.