-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 13724814033Details
💛 - 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. |
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.
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. |
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.
@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?
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.
Yes, thanks for tagging, let me look into it!
@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? |
So this is the type of error they would get I'm not sure if a warning makes sense since it's not a true warning if they aren't using the time extension. |
No, I mean we could add a warning at init time where we say something like:
|
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. |
Related Issues
Proposed Changes:
How did you test it?
Notes for the reviewer
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
and added!
in case the PR includes breaking changes.