mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Avoid cloning the full catalog for model metadata lookups (#55265)
## Why `get_model_info` clones the entire model catalog before selecting metadata for a single model. ## What changed Look up metadata directly in the active catalog for both remote and static model managers. Share catalog selection across remote catalog accessors and metadata lookup, preserving auth identity checks, API-key discovery policy, provider matching rules, and configuration overrides. ## Testing Extend coverage for custom catalog configuration overrides, metadata fallback after disabling API-key discovery, and fallback after an auth identity change. GitOrigin-RevId: 7d0bc4c1ccd2b56e16c7a17c1dbdca05a5c14a7b
This commit is contained in:
@@ -762,6 +762,12 @@ mod tests {
|
||||
let bundled = codex_models_manager::bundled_models_response().unwrap();
|
||||
assert_eq!(manager.get_remote_models().await, bundled.models);
|
||||
assert_eq!(manager.try_get_remote_models().unwrap(), bundled.models);
|
||||
assert_eq!(
|
||||
manager
|
||||
.get_model_info(&model.slug, &Default::default())
|
||||
.await,
|
||||
model_info_from_slug(&model.slug)
|
||||
);
|
||||
assert_eq!(
|
||||
manager
|
||||
.raw_model_catalog(
|
||||
|
||||
@@ -42,6 +42,7 @@ async fn api_key_discovery_flag_controls_fetches_and_cached_catalogs() {
|
||||
let manager =
|
||||
OpenAiModelsManager::new(home.path().into(), endpoint.clone(), Some(auth.clone()));
|
||||
let bundled = load_remote_models_from_file().unwrap();
|
||||
let config = ModelsManagerConfig::default();
|
||||
|
||||
for strategy in [
|
||||
RefreshStrategy::Online,
|
||||
@@ -69,6 +70,7 @@ async fn api_key_discovery_flag_controls_fetches_and_cached_catalogs() {
|
||||
models
|
||||
);
|
||||
assert_eq!(endpoint.fetch_count(), 1);
|
||||
assert_eq!(manager.get_model_info("dynamic", &config).await, models[0]);
|
||||
|
||||
// A runtime rollback must also suppress the already-loaded catalog.
|
||||
manager.set_api_key_model_discovery_enabled(/*enabled*/ false);
|
||||
@@ -80,6 +82,12 @@ async fn api_key_discovery_flag_controls_fetches_and_cached_catalogs() {
|
||||
bundled
|
||||
);
|
||||
assert_eq!(manager.try_get_remote_models().unwrap(), bundled);
|
||||
assert!(
|
||||
manager
|
||||
.get_model_info("dynamic", &config)
|
||||
.await
|
||||
.used_fallback_model_metadata
|
||||
);
|
||||
assert_eq!(endpoint.fetch_count(), 1);
|
||||
|
||||
// A new session must honor its startup flag even when a matching disk cache exists.
|
||||
|
||||
@@ -15,6 +15,7 @@ use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::openai_models::ModelPreset;
|
||||
use codex_protocol::openai_models::ModelVisibility;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use std::borrow::Cow;
|
||||
use std::fmt;
|
||||
use std::future::Future;
|
||||
use std::path::PathBuf;
|
||||
@@ -430,53 +431,38 @@ impl ModelsManager for OpenAiModelsManager {
|
||||
fn get_remote_models(&self) -> ModelsManagerFuture<'_, Vec<ModelInfo>> {
|
||||
Box::pin(async move {
|
||||
let entry = self.remote_models.read().await;
|
||||
if !self.api_key_discovery_disabled()
|
||||
&& entry.identity.is_some()
|
||||
&& entry.identity == self.endpoint_client.identity()
|
||||
{
|
||||
entry.models.clone()
|
||||
} else {
|
||||
self.catalog_source.fallback_models().unwrap_or_default()
|
||||
}
|
||||
self.models_for_entry(&entry).into_owned()
|
||||
})
|
||||
}
|
||||
|
||||
fn try_get_remote_models(&self) -> Result<Vec<ModelInfo>, TryLockError> {
|
||||
let entry = self.remote_models.try_read()?;
|
||||
Ok(
|
||||
if !self.api_key_discovery_disabled()
|
||||
&& entry.identity.is_some()
|
||||
&& entry.identity == self.endpoint_client.identity()
|
||||
{
|
||||
entry.models.clone()
|
||||
} else {
|
||||
self.catalog_source.fallback_models().unwrap_or_default()
|
||||
},
|
||||
)
|
||||
Ok(self.models_for_entry(&entry).into_owned())
|
||||
}
|
||||
|
||||
fn auth_manager(&self) -> Option<&AuthManager> {
|
||||
self.auth_manager.as_deref()
|
||||
}
|
||||
|
||||
#[tracing::instrument(skip_all, fields(model))]
|
||||
#[tracing::instrument(skip_all, fields(model = model))]
|
||||
fn get_model_info<'a>(
|
||||
&'a self,
|
||||
model: &'a str,
|
||||
config: &'a ModelsManagerConfig,
|
||||
) -> ModelsManagerFuture<'a, ModelInfo> {
|
||||
Box::pin(async move {
|
||||
let models = self.get_remote_models().await;
|
||||
match &self.catalog_source {
|
||||
CatalogSource::Default => {
|
||||
construct_model_info_from_candidates(model, &models, config)
|
||||
let remote = {
|
||||
let entry = self.remote_models.read().await;
|
||||
let models = self.models_for_entry(&entry);
|
||||
match &self.catalog_source {
|
||||
CatalogSource::Default => find_model_from_candidates(model, &models),
|
||||
CatalogSource::ExplicitProvider(_) => models
|
||||
.iter()
|
||||
.find(|candidate| candidate.slug == model)
|
||||
.cloned(),
|
||||
}
|
||||
CatalogSource::ExplicitProvider(_) => construct_model_info(
|
||||
model,
|
||||
models.into_iter().find(|candidate| candidate.slug == model),
|
||||
config,
|
||||
),
|
||||
}
|
||||
};
|
||||
construct_model_info(model, remote, config)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -498,6 +484,18 @@ impl ModelsManager for OpenAiModelsManager {
|
||||
}
|
||||
|
||||
impl OpenAiModelsManager {
|
||||
/// Selects the catalog available under the current provider and auth identity.
|
||||
fn models_for_entry<'a>(&self, entry: &'a ModelsCacheEntry) -> Cow<'a, [ModelInfo]> {
|
||||
if !self.api_key_discovery_disabled()
|
||||
&& entry.identity.is_some()
|
||||
&& entry.identity == self.endpoint_client.identity()
|
||||
{
|
||||
Cow::Borrowed(&entry.models)
|
||||
} else {
|
||||
Cow::Owned(self.catalog_source.fallback_models().unwrap_or_default())
|
||||
}
|
||||
}
|
||||
|
||||
async fn raw_model_catalog(
|
||||
&self,
|
||||
refresh_strategy: RefreshStrategy,
|
||||
@@ -766,6 +764,17 @@ impl OpenAiModelsManager {
|
||||
}
|
||||
|
||||
impl ModelsManager for StaticModelsManager {
|
||||
#[tracing::instrument(skip_all, fields(model = model))]
|
||||
fn get_model_info<'a>(
|
||||
&'a self,
|
||||
model: &'a str,
|
||||
config: &'a ModelsManagerConfig,
|
||||
) -> ModelsManagerFuture<'a, ModelInfo> {
|
||||
Box::pin(
|
||||
async move { construct_model_info_from_candidates(model, &self.remote_models, config) },
|
||||
)
|
||||
}
|
||||
|
||||
fn get_default_model<'a>(
|
||||
&'a self,
|
||||
model: &'a Option<String>,
|
||||
@@ -900,15 +909,20 @@ fn find_model_by_namespaced_suffix(model: &str, candidates: &[ModelInfo]) -> Opt
|
||||
find_model_by_longest_prefix(suffix, candidates)
|
||||
}
|
||||
|
||||
/// Selects owned model metadata without applying configuration overrides.
|
||||
fn find_model_from_candidates(model: &str, candidates: &[ModelInfo]) -> Option<ModelInfo> {
|
||||
// First use the normal longest-prefix match. If that misses, allow a narrowly scoped
|
||||
// retry for namespaced slugs like `custom/gpt-5.3-codex`.
|
||||
find_model_by_longest_prefix(model, candidates)
|
||||
.or_else(|| find_model_by_namespaced_suffix(model, candidates))
|
||||
}
|
||||
|
||||
pub(crate) fn construct_model_info_from_candidates(
|
||||
model: &str,
|
||||
candidates: &[ModelInfo],
|
||||
config: &ModelsManagerConfig,
|
||||
) -> ModelInfo {
|
||||
// First use the normal longest-prefix match. If that misses, allow a narrowly scoped
|
||||
// retry for namespaced slugs like `custom/gpt-5.3-codex`.
|
||||
let remote = find_model_by_longest_prefix(model, candidates)
|
||||
.or_else(|| find_model_by_namespaced_suffix(model, candidates));
|
||||
let remote = find_model_from_candidates(model, candidates);
|
||||
construct_model_info(model, remote, config)
|
||||
}
|
||||
|
||||
|
||||
@@ -811,6 +811,21 @@ async fn get_model_info_uses_custom_catalog() {
|
||||
assert_eq!(model_info.context_window, Some(272_000));
|
||||
assert!(model_info.supports_image_detail_original);
|
||||
assert!(!model_info.used_fallback_model_metadata);
|
||||
|
||||
let config = ModelsManagerConfig {
|
||||
model_context_window: Some(128_000),
|
||||
..Default::default()
|
||||
};
|
||||
let expected = ModelInfo {
|
||||
context_window: Some(128_000),
|
||||
..model_info
|
||||
};
|
||||
assert_eq!(
|
||||
manager
|
||||
.get_model_info("gpt-overlay-experiment", &config)
|
||||
.await,
|
||||
expected
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
Reference in New Issue
Block a user