-
Notifications
You must be signed in to change notification settings - Fork 43
feat: e2e dispatcher integration #458
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
Changes from all commits
1e0aa25
0da222f
bd37da6
21e186e
9b51646
640125b
7da991a
9b2a183
f092e6d
fa335e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,18 +230,6 @@ type BucketConfig struct { | |
| BucketCount int `yaml:"count"` | ||
| } | ||
|
|
||
| const asyncTenantID = "$batch" | ||
|
|
||
| // RequestQueueName returns the Redis sorted-set name for submitting async requests to the given pool. | ||
| func RequestQueueName(poolName string) string { | ||
| return "llm-d-async:requests:" + poolName | ||
| } | ||
|
|
||
| // ResultQueueName returns the Redis list name for collecting async results from the given pool. | ||
| func ResultQueueName(poolName string) string { | ||
| return "llm-d-async:results:" + poolName + ":" + asyncTenantID | ||
| } | ||
|
|
||
|
evacchi marked this conversation as resolved.
|
||
| // IsAsync returns true when the processor is configured for async dispatch. | ||
| func (c *ProcessorConfig) IsAsync() bool { | ||
| return c.DispatchMode == DispatchModeAsync | ||
|
|
@@ -386,14 +374,21 @@ func (c *ProcessorConfig) Validate() error { | |
| return fmt.Errorf("progress_ttl_seconds must be > 0") | ||
| } | ||
|
|
||
| if err := c.validateDispatchMode(); err != nil { | ||
| if err := c.validateGateways(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (c *ProcessorConfig) validateDispatchMode() error { | ||
| func (c *ProcessorConfig) validateGateways() error { | ||
| if c.GlobalInferenceGateway == nil && len(c.ModelGateways) == 0 { | ||
| return fmt.Errorf("either global_inference_gateway or model_gateways must be configured") | ||
| } | ||
| if c.GlobalInferenceGateway != nil && len(c.ModelGateways) > 0 { | ||
| return fmt.Errorf("global_inference_gateway and model_gateways are mutually exclusive") | ||
| } | ||
|
|
||
| switch c.DispatchMode { | ||
| case DispatchModeSync, DispatchMode(""): | ||
| c.DispatchMode = DispatchModeSync | ||
|
|
@@ -405,21 +400,7 @@ func (c *ProcessorConfig) validateDispatchMode() error { | |
| } | ||
| } | ||
|
|
||
| func (c *ProcessorConfig) validateGateways() error { | ||
| if c.GlobalInferenceGateway == nil && len(c.ModelGateways) == 0 { | ||
| return fmt.Errorf("either global_inference_gateway or model_gateways must be configured") | ||
| } | ||
| if c.GlobalInferenceGateway != nil && len(c.ModelGateways) > 0 { | ||
| return fmt.Errorf("global_inference_gateway and model_gateways are mutually exclusive") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (c *ProcessorConfig) validateSyncDispatchConfig() error { | ||
| if err := c.validateGateways(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if c.GlobalInferenceGateway != nil { | ||
| if err := validateGatewayConfig("global_inference_gateway", *c.GlobalInferenceGateway); err != nil { | ||
| return err | ||
|
|
@@ -435,15 +416,13 @@ func (c *ProcessorConfig) validateSyncDispatchConfig() error { | |
|
|
||
| func (c *ProcessorConfig) validateAsyncDispatchConfig() error { | ||
| if c.AsyncDispatchConfig.ResultPollTimeout <= 0 { | ||
| return fmt.Errorf("async.result_poll_timeout must be > 0") | ||
| } | ||
| if err := c.validateGateways(); err != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: don't you think all of this is still gateways validation? Maybe all the logic should be in that single function
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I folded the logic for sync/async+validateDispatchMode into validateGateways, I hope that's what you meant. |
||
| return err | ||
| return fmt.Errorf("async_dispatch.result_poll_timeout must be > 0") | ||
|
evacchi marked this conversation as resolved.
|
||
| } | ||
| if c.GlobalInferenceGateway != nil { | ||
| if c.GlobalInferenceGateway.InferencePoolName == "" { | ||
| return fmt.Errorf("global_inference_gateway.inference_pool_name must be set when dispatch_mode is %q", DispatchModeAsync) | ||
| } | ||
| return fmt.Errorf("global_inference_gateway is not supported with dispatch_mode %q; use model_gateways with inference_pool_name", DispatchModeAsync) | ||
| } | ||
| if len(c.ModelGateways) == 0 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't introduced by this PR but I think having
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, but this should be addressed separately, let's create an issue
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. created #526 |
||
| return fmt.Errorf("model_gateways must be configured when dispatch_mode is %q", DispatchModeAsync) | ||
| } | ||
| for model, gw := range c.ModelGateways { | ||
| if gw.InferencePoolName == "" { | ||
|
|
@@ -580,6 +559,7 @@ func toGatewayClientConfig(gw ModelGatewayConfig, apiKey string) inference.Gatew | |
| type ResolvedGateways struct { | ||
| Global *inference.GatewayClientConfig | ||
| PerModel map[string]inference.GatewayClientConfig | ||
| Async *inference.AsyncClientConfig | ||
| } | ||
|
|
||
| // ResolveModelGateways resolves API keys for all configured gateways and returns | ||
|
|
@@ -588,6 +568,17 @@ type ResolvedGateways struct { | |
| func ResolveModelGateways(cfg *ProcessorConfig) (*ResolvedGateways, error) { | ||
| result := &ResolvedGateways{} | ||
|
|
||
| if cfg.IsAsync() { | ||
| models := make(map[string]string, len(cfg.ModelGateways)) | ||
| for model, gw := range cfg.ModelGateways { | ||
| models[model] = gw.InferencePoolName | ||
| } | ||
| result.Async = &inference.AsyncClientConfig{ | ||
| Models: models, | ||
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| if cfg.GlobalInferenceGateway != nil { | ||
| apiKey, err := resolveGatewayAPIKey("global_inference_gateway", *cfg.GlobalInferenceGateway) | ||
| if err != nil { | ||
|
|
||
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.
do you need inference_pool_name for global_inference_gateway?
because i saw this in below
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.
no, global_inference_gateway doesn't need inference_pool_name. Async mode explicitly rejects global_inference_gateway, validation fails with that error message.
inference_pool_name is only needed on model_gateways entries, which should be where the template renders it. The error message is telling users to switch from global_inference_gateway to model_gateways when using async mode
we can do amendments to the config format in another PR anyway