-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Remove custom models options from cloud runs #11219
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -454,10 +454,24 @@ pub fn populate_model_picker_for_harness<A: OrchestrationControlAction, V: View> | |
| let harness = Harness::parse_orchestration_harness(&harness_type); | ||
| match harness { | ||
| Some(Harness::Oz) | None => { | ||
| // Oz / unset: current behavior — Warp LLM catalog. | ||
| // Oz / unset: Warp LLM catalog. Custom models excluded for | ||
| // cloud runs (not supported by remote workers). | ||
| // Order: auto models first, then custom models, then other models. | ||
| let llm_prefs = LLMPreferences::as_ref(ctx_dropdown); | ||
| let choices: Vec<_> = llm_prefs | ||
| let all_choices: Vec<_> = llm_prefs | ||
| .get_base_llm_choices_for_agent_mode(ctx_dropdown) | ||
| .filter(|llm| is_local || llm_prefs.custom_llm_info_for_id(&llm.id).is_none()) | ||
| .collect(); | ||
| let (auto_models, rest): (Vec<_>, Vec<_>) = all_choices | ||
| .into_iter() | ||
| .partition(|llm| llm.id.as_str().starts_with("auto")); | ||
| let (custom_models, other_models): (Vec<_>, Vec<_>) = rest | ||
| .into_iter() | ||
| .partition(|llm| llm_prefs.custom_llm_info_for_id(&llm.id).is_some()); | ||
| let choices: Vec<_> = auto_models | ||
| .into_iter() | ||
| .chain(custom_models) | ||
| .chain(other_models) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic here might be clearer if we do something like this to exclude custom models. I think it's a little confusing to filter out the custom models at the |
||
| .collect(); | ||
| let selected_display_name = choices | ||
| .iter() | ||
|
|
@@ -548,6 +562,7 @@ pub fn is_model_in_filtered_choices<V: View>( | |
| let llm_prefs = LLMPreferences::as_ref(ctx); | ||
| llm_prefs | ||
| .get_base_llm_choices_for_agent_mode(ctx) | ||
| .filter(|llm| is_local || llm_prefs.custom_llm_info_for_id(&llm.id).is_none()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the logic here rely on having the same model list as I wonder if we should include a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, added a common helper |
||
| .any(|llm| llm.id.to_string() == model_id) | ||
| } | ||
| Some(Harness::Codex) if is_local => model_id.is_empty(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -468,41 +468,36 @@ impl ModelSelector { | |
| .clone(); | ||
|
|
||
| let mut auto_choices = Vec::new(); | ||
| let mut custom_choices = Vec::new(); | ||
| let mut other_choices = Vec::new(); | ||
| for llm in llm_preferences.get_base_llm_choices_for_agent_mode(ctx) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use the common |
||
| if llm_preferences.custom_llm_info_for_id(&llm.id).is_some() { | ||
| continue; | ||
| } | ||
|
|
||
| let display_name = llm.menu_display_name(); | ||
| if !query.is_empty() && !display_name.to_lowercase().contains(query) { | ||
| continue; | ||
| } | ||
| if is_auto(llm) { | ||
| auto_choices.push(llm); | ||
| } else if llm_preferences.custom_llm_info_for_id(&llm.id).is_some() { | ||
| custom_choices.push(llm); | ||
| } else { | ||
| other_choices.push(llm); | ||
| } | ||
| } | ||
|
|
||
| let items: Vec<MenuItem<ModelSelectorAction>> = auto_choices | ||
| .into_iter() | ||
| .chain(custom_choices) | ||
| .chain(other_choices) | ||
| .map(|llm| { | ||
| let display_name = llm.menu_display_name(); | ||
| let is_custom = llm_preferences.custom_llm_info_for_id(&llm.id).is_some(); | ||
| let mut fields = MenuItemFields::new(display_name) | ||
| let fields = MenuItemFields::new(display_name) | ||
| .with_icon(llm.provider.icon().unwrap_or(Icon::Oz)) | ||
| .with_icon_size_override(ITEM_ICON_SIZE) | ||
| .with_font_size_override(ITEM_FONT_SIZE) | ||
| .with_padding_override(ITEM_VERTICAL_PADDING, MENU_HORIZONTAL_PADDING) | ||
| .with_override_hover_background_color(hover_background) | ||
| .with_on_select_action(ModelSelectorAction::SelectModel(llm.id.clone())) | ||
| .with_disabled(llm.disable_reason.is_some()); | ||
| if is_custom { | ||
| fields = fields.with_right_side_icon(Icon::Key); | ||
| } else { | ||
| fields = fields.with_icon(llm.provider.icon().unwrap_or(Icon::Oz)); | ||
| } | ||
| MenuItem::Item(fields) | ||
| }) | ||
| .collect(); | ||
|
|
||
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.
Do we need to collect into a
Vechere? That's a memory allocation that I don't think we need here, since the only time we useall_choiceswe immediately transform it back into an iterator usinginto_iter().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.
nope, not needed.
removed