Remove custom models options from cloud runs#11219
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
dagmfactory
changed the title
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR removes custom endpoint models from cloud-run model selection surfaces and adds a helper for identifying custom endpoint LLM entries.
Concerns
- For this user-facing model picker change, please include screenshots or a screen recording demonstrating the updated selector behavior end to end.
- The new custom-endpoint helper relies on a display-description prefix, which is brittle; see the inline suggestion.
Verdict
Found: 0 critical, 1 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| pub fn is_custom_endpoint(&self) -> bool { | ||
| self.description | ||
| .as_ref() | ||
| .is_some_and(|d| d.starts_with("Custom · ")) |
There was a problem hiding this comment.
💡 [SUGGESTION] This makes custom-endpoint detection depend on a user-facing description prefix. Prefer filtering via LLMPreferences::custom_llm_info_for_id(&llm.id).is_some() at the call sites so copy changes or a server model with the same prefix cannot change cloud model eligibility.
| 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) |
There was a problem hiding this comment.
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 all_choices level but still include custom_models in the chain.
let mut choices: Vec<_> = auto_models.into_iter();
if is_local {
choices = choices.chain(custom_models);
}
choices = choices.chain(other_models);
| 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(); |
There was a problem hiding this comment.
Do we need to collect into a Vec here? That's a memory allocation that I don't think we need here, since the only time we use all_choices we immediately transform it back into an iterator using into_iter().
There was a problem hiding this comment.
nope, not needed.
removed
| 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()) |
There was a problem hiding this comment.
Does the logic here rely on having the same model list as populate_model_picker_for_harness? We might want to extract the logic into a common method to avoid drift.
I wonder if we should include a include_custom_model or is_local parameter on get_base_llm_choices_for_agent_mode so that callers are forced to make a decision on whether or not they want custom models.
There was a problem hiding this comment.
good point, added a common helper
| 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) { |
There was a problem hiding this comment.
can we use the common get_base_model_choices method here as well?
2 participants
This change removes the ability to select custom models when running cloud runs via cloud run on terminal or via orchestrate.
Custom endpoints remain available for non-cloud agent conversations and local orchestration agents.
https://www.loom.com/share/2e2fa19213b9430fa7fbae763203b5c3