-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: Add back hardcoded default templates #4998
Conversation
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.
JFYI, the PR merged by @ZanSara renames prompt_text
to prompt
and removes name
in PromptTemplate
.
@TuanaCelik yep, I noticed. I'm fixing it right now. |
7619e0d
to
d2f4c69
Compare
Pull Request Test Coverage Report for Build 5059281109
💛 - Coveralls |
Ok, I see the value of these, but my concerns with this approach is: do we want to keep these local PromptTemplates forever, or not? I see there's no deprecation schedule or hint they will eventually be deprecated. I'm ok with keeping them, but if so, let's not call them "legacy" or similar names: we should find a stable place for them that makes sense. Also, I'd rather distinguish them strongly from the ones in the hub: If we don't want to support output parsers in the hub, then we either go head-in with this decision and deprecate these prompts, or backtrack and rethink the integration. And to be honest I believe output parsers could be supported in the hub (I'm volunteering for this task if necessary) so, if we need them so badly, we should consider this option too. In short, I see this solution here as valid only if it's a patch to ease the transition. Let's clarify what's our stand on this. |
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.
Let's clarify what we want the API to look like before moving forward
Ok after some discussion, let's add a deprecation notice to these templates. It should be enough for now. |
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.
This could be enough to clarify the situation imho.
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.
Overall this is probably good like this. We'll consider if deprecating the default templates or not later, depending on usage too. No need to block the hotfix for that 🙂
Proposed Changes:
Add the hardcoded
PromptTemplate
remove with PR #4879.How did you test it?
Add a new test that verifies the expected legacy template is returned when using legacy template names.
Notes for the reviewer
There's more information in the comment at the top of
legacy_default_templates.py
.