remove model_family from `config
#7571
Conversation
| pub model_reasoning_summary: ReasoningSummary, | ||
|
|
||
| /// Optional override to force-enable reasoning summaries for the configured model. | ||
| pub model_supports_reasoning_summaries: Option<bool>, |
There was a problem hiding this comment.
Why add these now? Was there a way to override these before that you are removing?
There was a problem hiding this comment.
no there wasn't. my guess is there was at some point before introducing model family but it was a chaos so we weren't respecting them anymore.
There was a problem hiding this comment.
sorry. yes it was updating model family in load_from_base_config_with_overrides which I thought it's only for tests per it's doc string but it's used to construct config
codex/codex-rs/core/src/config/mod.rs
Line 307 in cee37a3
codex-rs/tui/src/history_cell.rs
Outdated
| let cell = new_reasoning_summary_block( | ||
| "**High level reasoning without closing".to_string(), | ||
| &config, | ||
| &models_manager, |
There was a problem hiding this comment.
Do you need to be passing this in?
There was a problem hiding this comment.
We can just pass model_family.reasoning_summary_format but I am not sure what is best here actually.
codex-rs/core/src/client.rs
Outdated
| pub struct ModelClient { | ||
| config: Arc<Config>, | ||
| auth_manager: Option<Arc<AuthManager>>, | ||
| models_manager: Arc<ModelsManager>, |
There was a problem hiding this comment.
this seems wrong, why are we not passing model family here? Client lifetime is equal to turn lifetime as the model doesn't change.
| parent_otel.with_model(config.model.as_str(), config.model_family.slug.as_str()); | ||
| let child_otel = parent_otel.with_model( | ||
| config.model.as_str(), | ||
| models_manager |
There was a problem hiding this comment.
I don't like how many places we are calling construct_model_family in. Ideally we'd be passing model_family around.
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
Can we pass model_manager around less and model_family more?
model_familyfromconfigmodel_familylike supporting reasoning