-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Merge Modelfamily into modelinfo #8763
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
base: main
Are you sure you want to change the base?
Conversation
|
@codex review this and note any behavior change |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b80cd383a5
ℹ️ 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".
| upgrade: None, | ||
| base_instructions: BASE_INSTRUCTIONS.to_string(), | ||
| supports_reasoning_summaries: true, | ||
| support_verbosity: false, |
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.
Avoid enabling reasoning for unknown models by default
The fallback default_model_info now sets supports_reasoning_summaries: true, which is used whenever a model slug isn’t matched in find_model_info_for_slug and no remote metadata is available. In that case, ModelClient::stream_responses_api will include a reasoning block for custom/third‑party models that may not support it, leading to 400s or rejected requests. Previously unknown models defaulted to not sending reasoning, so this is a behavior change; consider keeping the default false or only enabling when remote metadata explicitly indicates support.
Useful? React with 👍 / 👎.
| }, | ||
| ReasoningEffortPreset { | ||
| effort: ReasoningEffort::High, | ||
| description: "Maximizes reasoning depth for complex or ambiguous problems".to_string(), |
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.
Would it be possible to change "Maximizes" to "Greater" like in supported_reasoning_level_low_medium_high_xhigh()?
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b80cd383a5
ℹ️ 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".
| pub supported_in_api: bool, | ||
| pub priority: i32, | ||
| pub upgrade: Option<String>, | ||
| pub base_instructions: Option<String>, | ||
| pub base_instructions: String, | ||
| pub supports_reasoning_summaries: bool, |
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.
Keep
base_instructions backward-compatible in ModelInfo
Because base_instructions is now a required String with no serde default, any cached models written by older clients (or older /models responses that omitted/returned null for this field) will fail to deserialize; ModelsClient::list_models and ModelsCache::load_cache treat this as a hard error, so remote models won’t load and offline users lose their cached list. Consider keeping this optional or adding a serde default to preserve backward compatibility.
Useful? React with 👍 / 👎.
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.
Agree with Codex - are we 100% guaranteed to have base_instructions and context_window present in the models_cache.json file?
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.
yeah it's required
| pub apply_patch_tool_type: Option<ApplyPatchToolType>, | ||
| pub truncation_policy: TruncationPolicyConfig, | ||
| pub supports_parallel_tool_calls: bool, | ||
| pub context_window: Option<i64>, | ||
| pub context_window: i64, | ||
| /// Token threshold for automatic compaction. When omitted, core derives it |
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.
Preserve parsing of cached models without
context_window
The new required context_window: i64 has the same compatibility problem: older cached entries or backend responses that used null will now fail to parse, causing refresh_available_models_with_cache to drop the cache and remote model discovery to fail when offline. Adding a default or keeping the field optional avoids breaking existing caches.
Useful? React with 👍 / 👎.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b80cd383a5
ℹ️ 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".
| } else if slug.starts_with("gpt-oss") || slug.starts_with("openai/gpt-oss") { | ||
| model.apply_patch_tool_type = Some(ApplyPatchToolType::Function); | ||
| model.context_window = 96_000; |
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.
Disable reasoning summaries for gpt-oss models
In find_model_info_for_slug, the gpt-oss branch no longer overrides supports_reasoning_summaries, so it inherits the new default_model_info value of true. That means ModelClient::stream_responses_api will now include a reasoning block for gpt-oss/openai/gpt-oss requests, whereas previously ModelFamily defaulted this to false. If those models don’t accept reasoning fields, Responses API calls will start failing (400s) whenever reasoning summaries are enabled. Consider explicitly setting supports_reasoning_summaries = false for gpt-oss or restoring the old default.
Useful? React with 👍 / 👎.
| let mut model = default_model_info(slug); | ||
|
|
||
| if slug.starts_with("o3") || slug.starts_with("o4-mini") { | ||
| model.base_instructions = BASE_INSTRUCTIONS_WITH_APPLY_PATCH.to_string(); |
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.
Why isn't this in existing model families?
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.
Can we reuse the same declarative way of defining models as we had before?
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.
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.
Can we reuse the same declarative way of defining models as we had before?
+1 to this point - purely declarative logic is going to be more maintainable than mutating a struct here. Can we use the same macro w/ defaults pattern?
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.
okay. I am planning to remove this and just use models.json so won't maintain it for long
dylan-hurd-oai
left a comment
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.
My only top-level question is whether we can split up this migration into a few smaller pieces, or whether we have sufficient testing on these code paths to be sure we're not missing any config changes.
| Self::find_family_for_model(model).with_config_overrides(config) | ||
| /// Offline helper that builds a `ModelInfo` without consulting remote state. | ||
| pub fn construct_model_info_offline(model: &str, config: &Config) -> ModelInfo { | ||
| model_info::with_config_overrides(Self::find_model_info_for_slug(model), config) |
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.
(out of scope for this PR) Is this helper function strictly necessary? If tests control the config, couldn't we just call construct_model_info with no remote settings enabled?
If we can remove it, would be great to simplify before or after this migration.
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.
it's mainly for unit tests not integration tests
| pub supported_in_api: bool, | ||
| pub priority: i32, | ||
| pub upgrade: Option<String>, | ||
| pub base_instructions: Option<String>, | ||
| pub base_instructions: String, | ||
| pub supports_reasoning_summaries: bool, |
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.
Agree with Codex - are we 100% guaranteed to have base_instructions and context_window present in the models_cache.json file?
|
@codex review this |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e9f830f72
ℹ️ 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".
dylan-hurd-oai
left a comment
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.
Thanks for cleaning up model_info - i think we need to address the codex comment, other than that LGTM
|
@codex review this |
|
@codex review this |
1 similar comment
|
@codex review this |
|
@codex list differences in behavior between declarative model info and previous model family |
This comment was marked as outdated.
This comment was marked as outdated.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f926231a4c
ℹ️ 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 list differences in behavior between declarative model info and previous model family |
This comment was marked as outdated.
This comment was marked as outdated.
a35c8d6 to
f926231
Compare
|
@codex list differences in behavior between declarative model info and previous model family |
This comment was marked as outdated.
This comment was marked as outdated.
|
@codex list differences in behavior between declarative model info and previous model family |
Behavioral differences between declarative
|
|
@codex list differences in behavior between declarative model info and previous model family |
This comment was marked as outdated.
This comment was marked as outdated.
|
@codex list differences in behavior between declarative model info and previous model family |
This comment was marked as outdated.
This comment was marked as outdated.
|
@codex list differences in behavior between declarative model info and previous model family |
This comment was marked as outdated.
This comment was marked as outdated.
|
@codex list differences in behavior between declarative model info and previous model family |
|
Below are the behavioral differences I can confirm between the new declarative 1) Apply‑patch instructions are no longer conditionally appendedBefore (ModelFamily):
Now (ModelInfo):
Impact: apply‑patch instructions are always present for those model entries now, even if the tool is available. 2) Remote model metadata is now “replace‑all” instead of “selective overlay”Before (ModelFamily):
Now (ModelInfo):
Impact: Remote metadata now fully replaces local defaults instead of partially overlaying them. 3) Verbosity warning now reports model slug instead of family nameBefore (ModelFamily):
Now (ModelInfo):
Impact: Log output changes (family → slug), which can affect log filtering/alerting. 4) Model info now always carries explicit reasoning‑preset metadata (affects presets/UI)Now (ModelInfo):
Before (ModelFamily):
Impact: Reasoning‑effort presets for UI/selection are now explicitly defined per model in metadata rather than inferred or absent. Commands used
If you want a model‑by‑model diff (e.g., |
|
@codex review this |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9ea8021ac
ℹ️ 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".
| pub priority: i32, | ||
| pub upgrade: Option<String>, | ||
| pub base_instructions: Option<String>, | ||
| pub base_instructions: String, | ||
| pub supports_reasoning_summaries: bool, |
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.
Handle missing base_instructions in /models responses
This field was made mandatory with no serde default, so any older /models responses or cached models_cache.json entries that omit base_instructions or send it as null will now fail to deserialize, causing list_models to error and remote model metadata to be skipped. That’s a compatibility regression for clients talking to older backends or reading existing caches. Consider keeping it optional or adding a serde default/fallback so older payloads still load.
Useful? React with 👍 / 👎.

ModelInfo