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

feat: time extension for ChatPromptBuilder #9001

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mathislucka
Copy link
Member

Related Issues

Proposed Changes:

How did you test it?

Notes for the reviewer

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

@mathislucka mathislucka requested a review from a team as a code owner March 7, 2025 16:17
@mathislucka mathislucka requested review from sjrl and removed request for a team March 7, 2025 16:17
@mathislucka mathislucka requested a review from a team as a code owner March 7, 2025 16:20
@mathislucka mathislucka requested review from dfokina and removed request for a team March 7, 2025 16:20
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 13724814033

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 89.921%

Files with Coverage Reduction New Missed Lines %
components/builders/chat_prompt_builder.py 4 93.51%
Totals Coverage Status
Change from base Build 13718432299: -0.02%
Covered Lines: 9689
Relevant Lines: 10775

💛 - Coveralls

self._env = SandboxedEnvironment()
try:
# The Jinja2TimeExtension needs an optional dependency to be installed.
# If it's not available we can do without it and use the PromptBuilder as is.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update doctoring to ChatPromptBuilder

features:
- |
Users can now work with date and time in the ChatPromptBuilder.
In the same way as the PromptBuilder, the ChatPromptBuilder now supports arrow to work with datetime.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dfokina I noticed that I don’t think we have in depth docs on how to use this especially around the expected format. Could we add them to the PromptBuilder and ChatPromptBuilder?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for tagging, let me look into it!

@sjrl
Copy link
Contributor

sjrl commented Mar 10, 2025

@mathislucka thanks for your work on this! I do realize this is a similar implementation as in PromptBuilder but I did want to ask what happens if arrow is not installed and a user still tries to request a date? Will they get an informative error message that they are missing the install?

@mathislucka
Copy link
Member Author

@mathislucka thanks for your work on this! I do realize this is a similar implementation as in PromptBuilder but I did want to ask what happens if arrow is not installed and a user still tries to request a date? Will they get an informative error message that they are missing the install?

I believe they would not get an error or warning but I'm not sure how that could be implemented. At init time we could log a warning when the time extension is not installed (instead of just silently swallowing the ImportError) but I don't think it would be practical to try and parse the template itself to determine if a user attempts to use date rendering or not.

Should I add just the warning? Or do you have a better idea?

@sjrl
Copy link
Contributor

sjrl commented Mar 10, 2025

So this is the type of error they would get jinja2.exceptions.TemplateSyntaxError: Encountered unknown tag 'now'. at runtime.

I'm not sure if a warning makes sense since it's not a true warning if they aren't using the time extension.

@mathislucka
Copy link
Member Author

No, I mean we could add a warning at init time where we say something like:

Install arrow if you want to use the time extension. (ideally with a link to the docs for what the time extension is / does)

@sjrl
Copy link
Contributor

sjrl commented Mar 10, 2025

No, I mean we could add a warning at init time where we say something like:

Install arrow if you want to use the time extension. (ideally with a link to the docs for what the time extension is / does)

Right I understand but I thought that might be an annoying warning to get as a user if they don’t plan on using the time extension. Although I think you’re right that it’s better to add that there so users are aware what to do to enable it. So I’d say go ahead and add it.

@sjrl sjrl self-requested a review March 10, 2025 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Jinja2TimeExtension to ChatPromptBuilder
4 participants