fix(server): stop silently dropping duplicate target model ids - #180
fix(server): stop silently dropping duplicate target model ids#180elyasmnvidian wants to merge 1 commit into
Conversation
27ad0ce to
ec76568
Compare
WalkthroughChanges
Target configuration validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
| // Each llm client keys its models by id, so two targets that share an id and an | ||
| // llm client would silently overwrite each other (last one wins) instead of failing. | ||
| // The same id on two different clients is allowed (one model served by two providers). | ||
| let mut target_names_by_client_id = BTreeMap::new(); |
There was a problem hiding this comment.
Do you want HashSet instead?
There was a problem hiding this comment.
Anthropic models love the BTreeMap. Opus 4.8 tried using one for me earlier where it did not seem to make sense.
There was a problem hiding this comment.
I changed to HashMap
There was a problem hiding this comment.
I meant Set. Don't you only need to know if it exists already or not? i.e you only need Key, not Key/Value.
ec76568 to
8d679ec
Compare
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
8d679ec to
2444823
Compare
| validate_value(&format!("target {target_name} id"), &target.id)?; | ||
| if let Some((first_name, first_target)) = targets_by_client_id.insert( | ||
| (target.llm_client.as_str(), target.id.as_str()), | ||
| (target_name.as_str(), target), |
There was a problem hiding this comment.
The HashMap<(&str, &str), (&str, &TargetConfig)> with target four way is awkward. Can we clean that up?
Ideally we'd have a single HashSet that says whether we've seen this before or not.
Two targets accidentally share one model id on one llm client:
The server stores each client's models by id, so it keeps one target and drops the other. Both routes start, but one
service_tieris gone and nothing says so — every request toswitchyard/flexnow sendspriority:The fix
switchyard-servernow checks the targets on each llm client before it starts.extra_body— dropping one target loses real config, so the server won't start. It prints an error naming both targets and how to fix it.Evidence
The broken config above now stops at startup, and a fixed config still starts:
No perf table — this change is about which configs start, not speed.
Compatibility
A config with two same-id targets and different
extra_bodyon one client used to start and drop one silently; it now stops at startup. To fix it, give the targets different ids, or point both routes at one target. The same id across two clients, and same-id targets that are otherwise the same, keep starting as before.Honest trade-off: two same-id targets that are otherwise the same still start with a warning, so a typo that repeats a model id you already serve is reported but not blocked. Rejecting every same-client duplicate was the alternative — set aside so a deliberate "one model, two route names" setup keeps working without edits. The deeper limit: the client stores models by id alone, so it can't hold two
extra_bodyvalues under one id — serving two service tiers on one key isn't supported yet, and this change makes that stop loudly instead of dropping one silently.How tested
cargo test -p switchyard-serverpasses. Three config tests pin the behavior:rejects_duplicate_target_ids_with_different_extra_body— same id and client, differentextra_body; the server stops. Fails on the old code, so it pins the fix.accepts_duplicate_target_model_ids_on_one_client— two same-id targets, otherwise the same; the server warns and both routes still resolve.accepts_same_model_id_on_different_llm_clients— same id on two clients; starts with no warning.cargo clippy -p switchyard-server --all-targets -- -D warningsandcargo fmt --all --checkare clean.Reproducer: