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

[llm] Parameter metadata #3404

Merged
merged 5 commits into from
May 17, 2023
Merged

[llm] Parameter metadata #3404

merged 5 commits into from
May 17, 2023

Conversation

tgaddair
Copy link
Collaborator

No description provided.

@tgaddair tgaddair requested a review from martindavis May 16, 2023 22:06
@github-actions
Copy link

github-actions bot commented May 16, 2023

Unit Test Results

  6 files  ±0    6 suites  ±0   1h 20m 13s ⏱️ + 7m 16s
33 tests ±0  29 ✔️ ±0    4 💤 ±0  0 ±0 
99 runs  ±0  87 ✔️ ±0  12 💤 ±0  0 ±0 

Results for commit 3866b31. ± Comparison against base commit b9bece4.

♻️ This comment has been updated with latest results.

template:
ui_display_name: Template
expected_impact: 3
component_type: textarea
Copy link
Collaborator

@ksbrar ksbrar May 17, 2023

Choose a reason for hiding this comment

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

So based on a separate conversation with @martindavis and I had earlier today, I would have expected the new component_type property to appear at the schema level, not in the parameter metadata.

By schema level I mean in the json_schema_type_mapping for PromptConfigField. So it would become a "sibling" property of "parameter_metadata" rather than one of the latter's child props.

We can plumb it through either way, but by putting it in parameter metadata the correct thing to do would be to define a default for this prop in the ParameterMetadata class as well, which I suppose could be null. But doing so would also imply we're establishing a general standard of tying parameters to their renderable HTML elements.

Up to you guys, I'm fine going either way. I kind of lean in this direction since we are already putting other "UI" focused metadata in their like the "ui_display_name". But maybe we don't want to more strongly tie (quite light) business logic in like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I went ahead and added ui_component_type to ParameterMetadata, which I feel is consistent with how we handle the display name, etc.

@tgaddair tgaddair merged commit 640206b into master May 17, 2023
14 of 15 checks passed
@tgaddair tgaddair deleted the llm-meta branch May 17, 2023 16:32
@tgaddair tgaddair mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants