Skip to content

Conversation

@srini047
Copy link
Contributor

@srini047 srini047 commented Oct 24, 2025

Related Issues

Proposed Changes:

Previously we checked only the undeclared variables and left the set variables.
Now, we find all the variables assigned using set in the template and push it to the variables list.

This behaviour would be seen in multiple components namely:

  • PromptBuilder
  • ChatPromptBuilder
  • LLMMetadataExtractor
  • LLMDocumentContentExtractor

How did you test it?

Run the exisiting suite and added test case for the this scenario.

Notes for the reviewer

  • Is this logic for addressing the problem is fine.
  • Then, do let me know if there's any other component (other than ones mentioned above) as well that needs to be addressed with this logic.

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added unit tests and updated the docstrings
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test: and added ! in case the PR includes breaking changes.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@vercel
Copy link

vercel bot commented Oct 24, 2025

@srini047 is attempting to deploy a commit to the deepset Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Oct 24, 2025
@coveralls
Copy link
Collaborator

coveralls commented Oct 24, 2025

Pull Request Test Coverage Report for Build 19114177728

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 22 unchanged lines in 7 files lost coverage.
  • Overall coverage remained the same at 92.248%

Files with Coverage Reduction New Missed Lines %
components/embedders/sentence_transformers_sparse_document_embedder.py 1 98.41%
components/embedders/sentence_transformers_sparse_text_embedder.py 1 98.08%
components/embedders/sentence_transformers_document_embedder.py 2 97.01%
components/embedders/sentence_transformers_text_embedder.py 2 96.49%
dataclasses/chat_message.py 2 99.33%
dataclasses/streaming_chunk.py 3 95.77%
tools/toolset.py 11 84.71%
Totals Coverage Status
Change from base Build 19078717113: 0.0%
Covered Lines: 13531
Relevant Lines: 14668

💛 - Coveralls

@srini047 srini047 marked this pull request as ready for review October 24, 2025 05:41
@srini047 srini047 requested a review from a team as a code owner October 24, 2025 05:41
@srini047 srini047 requested review from mpangrazzi and removed request for a team October 24, 2025 05:41
@sjrl sjrl requested review from sjrl and removed request for mpangrazzi October 24, 2025 08:08
@sjrl
Copy link
Contributor

sjrl commented Oct 27, 2025

With the above listed changes I think we could then extend this to the other components you mention. It may be wise to add this as a utility method in haystack/utils/ either in misc.py or a new file called jinja2.py

@sjrl
Copy link
Contributor

sjrl commented Nov 3, 2025

hey @srini047 do you think you'll have time to work on this, this week?

@srini047
Copy link
Contributor Author

srini047 commented Nov 3, 2025

hey @srini047 do you think you'll have time to work on this, this week?

Yes @sjrl will continue on this today.

@srini047 srini047 force-pushed the jinja2-set-vars-incorrect branch 3 times, most recently from 6835a8a to a305317 Compare November 4, 2025 20:00
@srini047 srini047 force-pushed the jinja2-set-vars-incorrect branch from 624a201 to de715ff Compare November 4, 2025 20:01
@srini047 srini047 requested a review from sjrl November 4, 2025 20:01
@srini047 srini047 force-pushed the jinja2-set-vars-incorrect branch from d016bd3 to 25e7416 Compare November 4, 2025 20:02
@srini047 srini047 force-pushed the jinja2-set-vars-incorrect branch from 25e7416 to 1f24f61 Compare November 4, 2025 20:08
@sjrl
Copy link
Contributor

sjrl commented Nov 6, 2025

Hey @srini047 could you add a release note for your PR following the instructions here?

Also we still want to bring this change to ChatPromptBuilder.

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.

PromptBuilder incorrectly marks internally set Jinja2 variables as required inputs when using `required_variables='*'

3 participants