-
Notifications
You must be signed in to change notification settings - Fork 31
Breaking: Remove legacy hayhooks pipeline deploy
; introduce hayhooks pipeline deploy-yaml
to deploy YAML pipelines with required inputs/outputs fields
#161
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
I'll put here a question as a reminder. Considering this situation (coming from a complex Haystack pipeline): inputs:
query:
- bm25_retriever.query
- query_embedder.text
- ConditionalRouter.question
filters:
- bm25_retriever.filters
- embedding_retriever.filters It's always safe to assume that |
@mpangrazzi yes I'd say based on the provided mapping here we can assume they will all have the same input type. Technically you could inspect it but that would require creating the pipeline first. |
…yaml ; refactoring
…sing inputs/outputs
…t using old YAML deploy logic)
…threadpool when running it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments.
Some general notes:
- I would improve the PR title (for release notes) to make it clear that the change is breaking and that we are introducing a non-backward compatible way to deploy YAML pipelines.
- Related to the previous point: are you considering releasing a major version?
@sjrl @anakin87 I should have addressed all your very useful feedbacks, thanks! Let me know if you have more of them of course.
Makes sense since it became bigger than expected, but I don't have a clear idea here. If you have a suggestion @anakin87 feel free to update it!
Hmm maybe we can do it. We didn't strictly followed SemVer here, but maybe we can start doing it from e.g. |
Yeah I think we should start following SemVar and starting with v1.0.0 sounds good! |
""" | ||
Create a flat Pydantic request model from declared inputs resolved by yaml_utils. | ||
|
||
Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a general comment for a separate PR but I wonder if we should use the same style of do strings we use in Haystack. Is there an advantage to using this style here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google-style docstrings were already there when I started working on Hayhooks, I simply kept using that style. IMHO for consistency yes, we can maybe migrate them to reST-style in another PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I've left a few more minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
For the title, I'd suggest something like (maybe long)
Breaking: Remove
hayhooks deploy
; introducehayhooks deploy-yaml
to deploy YAML pipelines with required inputs/outputs fields
inputs
/ outputs
fieldshayhooks pipeline deploy
; introduce hayhooks pipeline deploy-yaml
to deploy YAML pipelines with required inputs/outputs fields
Fixes #156.
inputs
andoutputs
params / types from a Haystack pipeline YAML definition/deploy-yaml
API route for deploying YAML pipelineshayhooks pipeline deploy-yaml
CLI command/deploy
endpoint - NOTE: This was needed to avoid confusion between new and old logicNote: initially I wanted to remove old YAML logic in another PR, but it would end to be quite confusing. Better to remove it now and update README accordingly.