refactor: restructure async config with explicit queue names (#526) - #629
Conversation
|
Unsigned commits detected! Please sign your commits. For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation. |
0f7047a to
b8519d5
Compare
evacchi
left a comment
There was a problem hiding this comment.
looks fine to me, the async dispatcher is a new component so if we land this soon we shouldn't break anything.
FYI @vishbhat @j-mok-dev
let @pazqo know if you think we should a backwards-compat path too
i think |
Nice catch, adding it now |
|
I'd prefer keeping modelGateways for async mode, it already holds both inferencePoolName and inferenceObjective, and the current PR breaks InferenceObjectiveFor() by leaving inferenceObjective orphaned. If we do want to move async model config into asyncDispatch.models, then consider to go further: use explicit queue names instead of having the code concatenate |
|
@yizhaodev this originated by the comment at #458 (comment) I am not particularly opinionated about this, but I feel like the config would indeed need some restructuring. You can feel free to propose an alternative! |
fbb4b54 to
b4409dd
Compare
Async dispatch mode reused ModelGateways (a sync-oriented struct with HTTP/TLS/retry fields) but only read InferencePoolName. Move async model-to-pool mappings into a dedicated AsyncDispatchConfig.Models field with a purpose-built AsyncModelConfig struct. InferencePoolName on ModelGatewayConfig is marked as deprecated. Signed-off-by: Stefano Pascolutti <spascolu@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Stefano Pascolutti <spascolu@redhat.com> Signed-off-by: Stefano Pascolutti <pascoluttistefano@gmail.com>
b4409dd to
2fef3a9
Compare
Signed-off-by: Stefano Pascolutti <pascoluttistefano@gmail.com>
2fef3a9 to
2f86867
Compare
|
@yizhaodev I added inferenceObjective, so that part should be fine. |
|
Maybe we can allow the user to specify the queue name, but we need to keep this auto-generate version as well until all the rest of the configs are migrated. |
@lioraron @j-mok-dev |
|
I’m leaning toward doing the fuller cleanup in this PR rather than stopping at a partial move.
@lioraron what do you think? |
|
I think keeping the old naming as a fallback is important in during this refactoring, at least to document the previous way of naming queues. We can also use comments, but the fallback (deprecated) is probably more robust. |
Add RequestQueueName and ResultQueueName fields to AsyncModelConfig so that async queue names are owned end-to-end by the async config section. When omitted, names are derived from InferencePoolName as before (deprecated fallback). Validation rejects partial config (only one of the two queue names set). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Stefano Pascolutti <pascoluttistefano@gmail.com>
|
@yizhaodev @j-mok-dev @gkneighb I added explicit queue names as first-class async config fields and kept the fallback from InferencePoolName (marked as deprecated). Please let me know if you think there is more work expected on this PR or if we can move forward, thanks! |
yizhaodev
left a comment
There was a problem hiding this comment.
it looks good to me overral
but i think you need to update benchmarks, examples/deploy-demo, scripts/dev-deploy-dispatcher.sh and operator CRD in the following
InferenceObjectiveFor checked AsyncDispatchConfig.Models regardless of dispatch mode, so leftover async config would shadow sync gateway objectives after switching modes. Gate the async lookup behind IsAsync(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Stefano Pascolutti <pascoluttistefano@gmail.com>
|
with this change i assume we will need some sort of release notes to cover the changes, even we keep old still valid and probably do a full cleanup in 2-3 release once deprecation preiod is done |
Async dispatch mode reused ModelGateways (a sync-oriented struct with HTTP/TLS/retry fields) but only read InferencePoolName. Move async model-to-pool mappings into a dedicated AsyncDispatchConfig.Models field with a purpose-built AsyncModelConfig struct.
InferencePoolName on ModelGatewayConfig is marked as deprecated.
Why is this PR needed?
Async dispatch mode reuses the sync-oriented ModelGateways config section, but only reads InferencePoolName — the other ~12 fields (URL, TLS, retries, API keys) are silently ignored. This is confusing for users configuring async, who can't tell which fields matter. This PR gives async its own config section (async_dispatch.models) where every field is meaningful, making the config self-documenting per dispatch mode.
What does this PR do?
Introduces AsyncModelConfig (with a single InferencePoolName field) and adds a Models map to AsyncDispatchConfig. In async mode, validation and resolution now read from async_dispatch.models instead of model_gateways. InferencePoolName on ModelGatewayConfig is marked as deprecated.
Additionally, adds InferenceObjective to AsyncModelConfig so the async config owns objective routing end-to-end (previously only available via the sync-oriented ModelGatewayConfig).
Adds explicit RequestQueueName and ResultQueueName fields to AsyncModelConfig, making queue naming a first-class config concern rather than a hidden convention. When omitted, queue names are still derived from InferencePoolName as a deprecated fallback. Validation rejects partial config (only one of the two set).
The Helm chart, e2e, and benchmark values files are updated to use the new config structure. Sync mode is unaffected.
How was this tested?
Checklist
git commit -s) per DCOmake ci)make test-e2e) (also async)Related Issues
This PR addresses #526, originated from a comment to #458.