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

All input parameters in PromptBuilder are optional #7441

Closed
1 task done
LastRemote opened this issue Mar 30, 2024 · 7 comments
Closed
1 task done

All input parameters in PromptBuilder are optional #7441

LastRemote opened this issue Mar 30, 2024 · 7 comments
Assignees
Labels
2.x Related to Haystack v2.0 topic:promptnode

Comments

@LastRemote
Copy link
Contributor

LastRemote commented Mar 30, 2024

Describe the bug
All input parameters in PromptBuilder have a default empty-string value and are thus marked as optional. I guess this is not the intended behavior according to the API documentations (https://docs.haystack.deepset.ai/reference/builders-api#promptbuilder). This does not have a big impact in terms of execution, but it does mess up the auto-generated pipeline graphs so I decided to report and fix it.

Error message
N/A

Expected behavior
{'is_mandatory': True} for all PromptBuilder inputs

Additional context
An one-line fix should be enough: https://github.com/deepset-ai/haystack/blob/main/haystack/components/builders/prompt_builder.py#L33

To Reproduce
Follow the 2.0 RAG pipeline recipe from https://docs.haystack.deepset.ai/docs/creating-pipelines

FAQ Check

System:

  • OS: not relevent
  • GPU/CPU: not relevent
  • Haystack version (commit or version number): haystack-ai==2.0.0
  • DocumentStore: not relevent
  • Reader: not relevent
  • Retriever: not relevent
@silvanocerza
Copy link
Contributor

This is actually intended.

We made the decision to make all PromptBuilder inputs as optional to support some use cases like self correcting loops in a Pipeline. If the inputs are not optional you would not be able to create loops in the Pipeline using a PromptBuilder that easily.

Also this is a pretty big breaking change, it fundamentally changes how you can connect a PromptBuilder in a Pipeline.

In the end I wouldn't do this, though I would like to know more about your use case if you need all inputs to be mandatory. If it's a valid use case we can find a way to make it work if we're missing something.

@LastRemote
Copy link
Contributor Author

LastRemote commented Apr 7, 2024

This is actually intended.

We made the decision to make all PromptBuilder inputs as optional to support some use cases like self correcting loops in a Pipeline. If the inputs are not optional you would not be able to create loops in the Pipeline using a PromptBuilder that easily.

Also this is a pretty big breaking change, it fundamentally changes how you can connect a PromptBuilder in a Pipeline.

In the end I wouldn't do this, though I would like to know more about your use case if you need all inputs to be mandatory. If it's a valid use case we can find a way to make it work if we're missing something.

Thanks for the reply and sharing the context. I think it does make sense either way, but the code behavior does not seem to match its documentation so I got a bit confused.

I would prefer that we can have an optional parameter during PromptBuilder initialization to tell whether to make (some of) the jinja inputs mandatory though (this also avoids breaking change since you could make the default behavior optional). As for my use case, I was trying out a customized RAG pipeline as a showcase to my colleagues, and I would prefer to do a sanity check to see if some key inputs are not provided, say, if "user question" is somehow missing when I use a LLM to generate a search query. It does feel weird if I would need to put this check elsewhere, and I would like to make this statement clear by showing pipeline.inputs().

Moreover, the graph does look a bit off when I run pipeline.draw(). For example, when I run 2.0 RAG Pipeline and here is what I get for pipeline.draw():
example
On the contrary, this is the graph shown on the documentation page, which I think looks much better (ignore the AnswerBuilder part for now):
original_draw
I think in this case prompt_builder expects a question input, and this information is much more clearly illustrated in the second graph.

@silvanocerza
Copy link
Contributor

I would prefer that we can have an optional parameter during PromptBuilder initialization to tell whether to make (some of) the jinja inputs mandatory though (this also avoids breaking change since you could make the default behavior optional).

This is a valid requests I'd say, I would keep the current behaviour and add a flag that makes the inputs non optional. Feel free to update the PR you already opened or create a new one from scratch if you want to take care of this.

The documentation is indeed wrong.
We heavily refactored the Pipeline.run() method before the 2.0.0 release and we had to change the PromptBuilder accordingly to support certain use cases. It was also one of the first Components we created when work on Haystack 2 started, the documentation was written at the time too. Same thing for the Pipeline graph.

We probably forgot to update the docs after those changes, I'll open a PR to update the docstring and update the documentation.

Thanks for the insights, really helpful. 🙏

@LastRemote
Copy link
Contributor Author

LastRemote commented Apr 10, 2024

This is a valid requests I'd say, I would keep the current behaviour and add a flag that makes the inputs non optional. Feel free to update the PR you already opened or create a new one from scratch if you want to take care of this.

Thank you for acknowledging the validity of the request. Sure, I can update the PR with the necessary modifications. You may expect these updates by early next week.

Thanks for the quick docstring fix and all the insightful discussions too! I'm glad to contribute to the improvement of the project, it's my fav framework for building LLM apps right now :)

@silvanocerza
Copy link
Contributor

Awesome! I'll be on the lookout for the updated PR, feel free to ping me directly if you want. 💪

@LastRemote
Copy link
Contributor Author

I decided to open a new PR since it is no longer a fix: #7553

@silvanocerza
Copy link
Contributor

Closing as fixed by #7553.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to Haystack v2.0 topic:promptnode
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants