-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Order prompt schema and add title #3419
Conversation
@@ -67,16 +67,14 @@ def __init__(self): | |||
super().__init__(RetrievalConfig) | |||
|
|||
def _jsonschema_type_mapping(self): | |||
return schema_utils.unload_jsonschema_from_marshmallow_class(RetrievalConfig) | |||
return schema_utils.unload_jsonschema_from_marshmallow_class(RetrievalConfig, title="Retrieval") |
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.
@connor-mccorm what's the motivation to add 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.
Basically, in UI, when there is a nested JSON object - so think the Retrieval being a field under prompt - the schema processor looks to create a specific section for the nested object so that it can display all it's properties. Without a title however, it just displays a random UI divider bar which looks weird / doesn't make sense. particular line adds that title so it's clear that the user is adding in parameters for the retrieval section. Broadly speaking, the addition of a title param to this function allows us to add titles in for this reason in other parts of the schema when necessary.
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.
Got it! How does it work for other parts of the schema that don't explicitly set title like 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.
So if you don't explicitly set the title, then we just leave the json object without a title like we were doing before. Some of the objects get titles added in the FE which we want to avoid since it just adds extra processing time which makes the UI less snappy, so we should expect to see some more of those moving over and using this mechanism in the near future.
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.
Got it!
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.
@connor-mccorm could you add a comment to unload_jsonschema_from_marshmallow_class
? This seems like useful context for future readers of this code will have more context on when/why "title" is set here and why not other schemas.
Reasoning for adding a title param to
unload_jsonschema_from_marshmallow_class
: Basically, in UI, when there is a nested JSON object - so think the Retrieval being a field under prompt - the schema processor looks to create a specific section for the nested object so that it can display all it's properties. Without a title however, it just displays a random UI divider bar which looks weird / doesn't make sense. particular line adds that title so it's clear that the user is adding in parameters for the retrieval section. Broadly speaking, the addition of a title param to this function allows us to add titles in for this reason in other parts of the schema when necessary.