-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fixes and improvements to get custom providers working in the UI #4557
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
Fixes and improvements to get custom providers working in the UI #4557
Conversation
… improving the providers edit box exposing more fields, and the grid for UX experience, similar to extensions Signed-off-by: Carine Bruyndoncx <bruyndoncx@gmail.com>
|
@angiejones providers looks a bit like extensions now ... |
|
@zanesq the display of the labels has 2 ways to generate the label, i have not touched that. It is possible to see when it is an environment variable, but the label removes the underscore (placeholder has a space i think), so it is not truely obvious what environment variable it should be. |
|
@alexhancock replied on Discord, but commenting here that he will take a look today! |
|
@alexhancock gentle reminder |
|
Let me tag other team members across goose dev as well since there has been recent issues and items to review! @DOsinga @michaelneale @zanesq @jamadeo |
jamadeo
left a comment
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.
I really like the direction and the attention to custom provider config -- thank you @cbruyndoncx ! I think we could get this merged more quickly if you are able to break up the changes, though. Maybe one for the format/streaming changes, one for the configuration changes at least?
Also, for the format changes: can you share some context for which providers need this? It would be good to test them out or at least be generally aware of the variations on openai's format we see in the wild.
| } | ||
| } | ||
| } | ||
|
|
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 was slightly confusing to me -- where do we have a secret with a boolean and value and why should it be skipped?
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.
i think this is because our API returns true when getting a secret value or false if there is no value. however, I don't think we should block setting secret values to true - it is not much a of a value to keep secret. we should just not do this write. is there a particular case where this is happening?
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 the windows ui the true value seemed to cause keyring issues. I have tried a detailed split , but that failed. Retry with a subset, to get the custom provider bugs fixed first.
| .await; | ||
| // also send a visible assistant message so the UI shows it inline | ||
| let assistant_msg = Message::assistant().with_text(format!("Provider error: {}", err_text)); | ||
| let _ = stream_event(MessageEvent::Message { message: assistant_msg }, &task_tx, &cancel_token).await; |
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.
if we want to show the error (I thought we already did?) I think it would be better to just render the Error event instead of stream it as a Message
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.
when does this happen?
| // include the error text so renderers can style it as an error. | ||
| let msg_text = format!("LLM streaming error encountered. See details below:\n{}", details); | ||
|
|
||
| let message = Message::assistant().with_content(MessageContent::text(msg_text)); |
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.
If we do need to show errors while keeping the stream alive (and I could see how that makes sense) we should have instead a non-terminating error type where we can return these without putting them in a Message
| console.debug('API client get failed, falling back to fetch', e); | ||
| } | ||
|
|
||
| // Fallback to a direct fetch |
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.
why the fallback? is it because client wasn't initialized with the right endpoint? that should I think be working better now as of #4338 and we shouldn't need this
| path: { id: currentProvider.name }, | ||
| headers: { 'X-Secret-Key': secretKey }, | ||
| }); | ||
| const body = |
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 should use the requests in sdk.gen.ts
DOsinga
left a comment
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.
I like this a lot, makes things right that weren't the first time.
I left some comments, but there's two things that stand out. I don't know why the custom providers should interact with their own custom settings, that seems confusing.
also can you look at the client code; it looks a little vibe coded.
| } | ||
| } | ||
| } | ||
|
|
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.
i think this is because our API returns true when getting a secret value or false if there is no value. however, I don't think we should block setting secret values to true - it is not much a of a value to keep secret. we should just not do this write. is there a particular case where this is happening?
| (status = 500, description = "Internal server error") | ||
| ) | ||
| )] | ||
| pub async fn update_custom_provider( |
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.
can you remove the over eager LLM comments here?
| axum::extract::Path(id): axum::extract::Path<String>, | ||
| Json(request): Json<UpdateCustomProviderRequest>, | ||
| ) -> Result<Json<String>, StatusCode> { | ||
| verify_secret_key(&headers, &state)?; |
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.
we got rid of this, but presumably syncing to main will tell you
| .await; | ||
| // also send a visible assistant message so the UI shows it inline | ||
| let assistant_msg = Message::assistant().with_text(format!("Provider error: {}", err_text)); | ||
| let _ = stream_event(MessageEvent::Message { message: assistant_msg }, &task_tx, &cancel_token).await; |
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.
when does this happen?
| .filter(|s| s.starts_with("LLM streaming error encountered")) | ||
| { | ||
| // Send a short error string (avoid flooding the SSE with huge payloads) | ||
| let short = if first_text.len() > 1024 { |
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.
don't use .len or [..] for text since that breaks CJK. we have a function to this, safe_truncate
| !configValues[parameter.name] | ||
| ) { | ||
| newValues[parameter.name] = String(parameter.default); | ||
| if (parameter.default !== undefined && parameter.default !== null) { |
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.
what is the scenario where this is the right thing to do?
| if check_context_length_exceeded(&payload_str) { | ||
| ProviderError::ContextLengthExceeded(payload_str) | ||
| } else { | ||
| // Try multiple ways to extract a useful message |
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 not use "Unknown error" as a way to flag if we have already handled this case (for one thing if the error is really unknown, we might now return the payload_str). but also, what are we trying to cover here? which providers have these specific behaviors we're trying to rescue from?
| // Some providers put a 'name' (tool name) here instead of in tool_calls | ||
| name: Option<String>, | ||
| // OpenAI/variants may include a 'refusal' field with refusal details | ||
| refusal: Option<Value>, |
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.
here and elsewhere, we talk about "some providers". I think we need to explicitly mention which ones we are talking about. in an ideal world we would just have a dialect flag but I can see how that is hard from a custom provider perspective
| // global config/keyring. Previously this used the constant | ||
| // "CUSTOM_PROVIDER_BASE_URL" for every provider which caused different | ||
| // providers to read/write the same key and mix up values stored in the | ||
| // keyring or config file. |
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.
previously? the reader doesn't care how this previously worked
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.
Previously if you just had a single custom provider it worked, but failed with the second one
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.
I understand the comment, but comments should help the reader understand the current code. Either way, custom providers should not use environment variables where the values are defined for the custom provider itself.
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.
yes, that i understand now, trying to figure out when/where they are introduced. I feel like i am on a treasure hunt in the code ...
I thought environment variables were always allowed, but i understand it slightly better now how this new design is supposed to work.
| ) -> Result<()> { | ||
| let configs = load_custom_providers(dir)?; | ||
|
|
||
| // Detect legacy shared key usage in keyring/config that could override |
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.
I'm not sure I follow this. I don't think custom providers should support custom proivder base urls beyond what they specify in the json. why do we look at the config here?
in general I don't think custom providers should access the config at all. they are already custom
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.
Understood now - things become clearer
|
Thanks for the review. Yes, Goose vibed with me. I have merged v1.8.0 locally, and creating detailed PRs for the different pieces. @DOsinga I don't understand your remark |
|
sorry for the slow reply. what I mean is that a custom provider doesn't need an environment variable to override its base url; it's a custom provider, it already has a custom base url. etc. does that make sense? |
Pull Request Description
Description
Key changes
Backend / core
UI (Electron / React)
Motivation
User-visible changes
Tests completed:
Known issues:
Screenshots
Provider grid
Edit custom provider configuration
Exposing all fields from the json custom providers config

Edit default providers configuration
Exposing all environment vars fields in the UI
