make model optional in config
#7769
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn build_per_turn_config(session_configuration: &SessionConfiguration) -> Config { | ||
| let config = session_configuration.original_config_do_not_use.clone(); | ||
| let mut per_turn_config = (*config).clone(); | ||
| per_turn_config.model = session_configuration.model.clone(); |
There was a problem hiding this comment.
are we confident nobody is reading this?
There was a problem hiding this comment.
Supposedly yeah. I replaced it with turn context
codex-rs/core/src/codex.rs
Outdated
|
|
||
| let model_family = models_manager | ||
| .construct_model_family(&config.model, &config) | ||
| .construct_model_family(&session_configuration.model, &config) |
There was a problem hiding this comment.
I'd love if everything model-related happend through model_family and we didn't flow raw string model anywhere.
codex-rs/core/tests/suite/review.rs
Outdated
|
|
||
| // Assert the request body model equals the configured review model | ||
| // Assert the review request currently uses the primary chat model. | ||
| // TODO: switch back to asserting the custom review model once the runtime plumbs it through. |
There was a problem hiding this comment.
Is there a way we don't merge broken default review model?
codex-rs/tui/src/status/tests.rs
Outdated
| Some(&rate_display), | ||
| None, | ||
| captured_at, | ||
| config_model(&config), |
There was a problem hiding this comment.
very roundabout way to pass the model into this method. Does it have to be set on config for some other reason?
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
I think having a separate default_model method on models manager is a footgun.
Models manager should have 3 methods:
async list_models
async get_model(requested_model: Optional<...>)
both of which should async block on manager waiting for models to get loaded.
we can use async list_models as a way to trigger warm up of model manager on start.
I'm also a bit worried about all the callsites that read model override fields from config directly and us having no systematic way of getting rid of them.
| pub fn new(auth_manager: Arc<AuthManager>) -> Self { | ||
| pub fn new( | ||
| auth_manager: Arc<AuthManager>, | ||
| provider_override: Option<ModelProviderInfo>, |
There was a problem hiding this comment.
It is strange to have to pass this in and then also have a fallback.
There was a problem hiding this comment.
That's for tests. Have been fighting it for a bit. lmk if there is an easy way to override the provider
There was a problem hiding this comment.
Model provider is based on the config.
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| codex_home, | ||
| cache_ttl: DEFAULT_MODEL_CACHE_TTL, | ||
| provider: ModelProviderInfo::get_chatgpt_provider().ok(), |
There was a problem hiding this comment.
ModelsManager now captures a provider at construction and defaults it to the built‑in ChatGPT provider, but ConversationManager only ever calls ModelsManager::new so this field is never replaced with the user’s Config.model_provider. refresh_available_models relies on this stored provider, so any setup that points at a different endpoint (API key mode, custom base_url/proxy, Azure, OSS providers) will still fetch /models from ChatGPT and ignore the configured provider, breaking remote-model discovery and default-model selection for non-ChatGPT providers. This regresses the previous behavior where the refresh call used the config’s provider directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That's intentional
There was a problem hiding this comment.
This is not wrong. Should we be checking that provider == openai before fetching models?
codex-rs/core/src/codex.rs
Outdated
| approval_policy, | ||
| sandbox_policy, | ||
| model, | ||
| model_family, |
There was a problem hiding this comment.
I know I keep saying I want model family everywhere but is it the right choice for SessionSettingsUpdate? Or should we keep the field as string and process the turn string into ModelInfo where SessionSettingsUpdate is applied and avoid carrying config around? Don't feel strongly, just got triggered by having to pull config through.
There was a problem hiding this comment.
We will need to pass config in the place where we update model session configuration, what do you think?
There was a problem hiding this comment.
Yeah, we'll have to keep model on SessionConfiguration and resolve when creating TurnContext. Feels okayish
codex-rs/tui/src/app.rs
Outdated
| resolved_model: self.current_model.clone(), | ||
| }; | ||
| self.chat_widget = ChatWidget::new(init, self.server.clone()); | ||
| self.current_model = self.chat_widget.model_slug().to_string(); |
There was a problem hiding this comment.
why do we pull current_model from chat_widget?
There was a problem hiding this comment.
when we just passed it in resolved_model: self.current_model
codex-rs/tui/src/chatwidget.rs
Outdated
| pub(crate) skills: Option<Vec<SkillMetadata>>, | ||
| pub(crate) is_first_run: bool, | ||
| pub(crate) model_family: ModelFamily, | ||
| pub(crate) resolved_model: String, |
There was a problem hiding this comment.
isn't this the same data as model_family?
codex-rs/tui/src/chatwidget.rs
Outdated
| self.config.model = Some(self.model.clone()); | ||
| self.add_to_history(history_cell::new_session_info( | ||
| &self.config, | ||
| &requested_model, |
There was a problem hiding this comment.
why are we using requested and not actual?
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review this. something different than Remote model refresh ignores configured provider |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review this. something different than Remote model refresh ignores configured provider |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
aibrahim-oai
force-pushed
the
config-model
branch
from
f1476fe to
545cd5a
Compare
aibrahim-oai
force-pushed
the
config-model
branch
from
545cd5a to
2bfb7ef
Compare
modelonce incoreandtuifrom config. Then store the state of it on other structs.