Wire with_remote_overrides to construct model families
#7621
Conversation
aibrahim-oai
changed the title
with_remote_overrides to construct model families
|
@codex review this |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| 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(); | ||
| per_turn_config.model_reasoning_effort = session_configuration.model_reasoning_effort; |
There was a problem hiding this comment.
I'm not super stoked we are keeping these. Is it possible to apply overrides to model but don't expose it on config an let everyone read random values?
There was a problem hiding this comment.
Non blocking, just ranting.
codex-rs/core/src/codex.rs
Outdated
| assert_eq!(expected, got); | ||
| } | ||
|
|
||
| fn construct_model_family_offline(model: &str, config: &Config) -> ModelFamily { |
There was a problem hiding this comment.
don't love how many helpers we have, I'd prefer if only methods on model_manager were public and we didn't expose find_family_for_model
codex-rs/core/src/codex.rs
Outdated
| session_source: SessionSource::Exec, | ||
| }; | ||
| let per_turn_config = Session::build_per_turn_config(&session_configuration); | ||
| let model_family = construct_model_family_offline(&per_turn_config.model, &per_turn_config); |
There was a problem hiding this comment.
we have models manager, can we toss a cfg(test)-only helper onto it?
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
Main feedback - model family on chat.handle_codex_event. Can it be on the instance?
with_remote_overridesand make theconstruct_model_familiesan async functionsyncsynchelper for model families