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

feat: model provider configuration validation #1019

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iwilltry42
Copy link
Contributor

@iwilltry42 iwilltry42 commented Dec 23, 2024

Important: This draft PR contains a couple of changes.

  1. Change to the handler to populate ToolReference.Status.Error with an error message if available models couldn't be fetched
    Screenshot-20241223111504-1128x61
  2. Change to the UI when calling /available-models/{model-provider-id} so it shows the error message to the user
    Screenshot 2024-12-23 at 10-45-22
  3. Change to the UI to call /validate and only if that succeeds, save the config and call /available-models
    • If it fails, don't save the config and display an appropriate error
  4. Add the /validate endpoint for model providers which calls the subtool validate from <model-provider-tool> to validate the config (if available)

Tested with the "reference implementation" which is the gemini-vertex provider: #986 / obot-platform/tools#297

Ref:

@iwilltry42 iwilltry42 marked this pull request as draft December 23, 2024 10:12
@iwilltry42 iwilltry42 requested a review from thedadams December 23, 2024 10:12
@iwilltry42 iwilltry42 force-pushed the feat/model-provider-validation branch from 6c75635 to b479edc Compare December 23, 2024 10:28
@iwilltry42 iwilltry42 force-pushed the feat/model-provider-validation branch from 194ab84 to 13ca8d7 Compare January 3, 2025 12:45
@iwilltry42 iwilltry42 changed the title feat: surface model configuration errors feat: model provider configuration validation Jan 3, 2025
@iwilltry42 iwilltry42 force-pushed the feat/model-provider-validation branch 3 times, most recently from 98a4d52 to e4bbfe1 Compare January 3, 2025 20:47
@iwilltry42 iwilltry42 marked this pull request as ready for review January 3, 2025 20:52
@iwilltry42 iwilltry42 force-pushed the feat/model-provider-validation branch from e4bbfe1 to fb1cb19 Compare January 3, 2025 20:56
pkg/api/handlers/modelprovider.go Show resolved Hide resolved
pkg/api/handlers/modelprovider.go Outdated Show resolved Hide resolved
},
}

if err := req.Get(thread, thread.Name); apierrors.IsNotFound(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? It looks like thread.Name will always be empty here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely not - leftover from when I didn't use GenerateName

pkg/api/handlers/modelprovider.go Outdated Show resolved Hide resolved
pkg/controller/handlers/toolreference/toolreference.go Outdated Show resolved Hide resolved
@iwilltry42 iwilltry42 requested a review from thedadams January 7, 2025 19:33
@iwilltry42 iwilltry42 force-pushed the feat/model-provider-validation branch from 384c9dc to f4f054d Compare January 8, 2025 16:56
@iwilltry42 iwilltry42 force-pushed the feat/model-provider-validation branch from f4f054d to 08da9a2 Compare January 10, 2025 14:52
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.

3 participants