-
Notifications
You must be signed in to change notification settings - Fork 837
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(operator): Included LLM spec to CRD #6234
feat(operator): Included LLM spec to CRD #6234
Conversation
6b0e24a
to
1ac617b
Compare
1ac617b
to
a698f39
Compare
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.
In general it looks good, I left some comments for consideration.
@@ -96,6 +96,9 @@ func TestAsModelDetails(t *testing.T) { | |||
Type: "anchor_tabular", | |||
ModelRef: &incomeModel, | |||
}, | |||
Llm: &LlmSpec{ |
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.
nit: I think that we should have a test case specific to LLM spec and not have it with explainer. In fact the user should not be able to set the two together and this can should be marked as invalid perhaps.
Co-authored-by: Sherif Akoush <[email protected]>
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.
LGTM!
e006dc0
to
579ab9e
Compare
What this PR does / why we need it:
This PR adds support for the
llm
tag in the model CRD. Similarly to theexplainer
tag, thellm
tag allows to reference an inference model by the name. This specification is used in thePromptRuntime
for the LLM Module.Example usage:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: