Skip to content

Add retry on eager #393

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

Merged
merged 9 commits into from
Jun 3, 2025

Conversation

gkumbhat
Copy link
Collaborator

@gkumbhat gkumbhat commented May 8, 2025

Changes

@gkumbhat gkumbhat marked this pull request as ready for review May 13, 2025 20:46
@@ -73,7 +73,7 @@ tokio-rustls = { version = "0.26.1", features = ["ring"] }
tokio-stream = { version = "0.1.17", features = ["sync"] }
tonic = { version = "0.12.3", features = [
"tls",
"tls-roots",
"tls-native-roots",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tls-roots is being deprecated

@mdevino mdevino mentioned this pull request Jun 2, 2025
Copy link
Collaborator

@declark1 declark1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs rebase and left a couple of nit comments, but I think this is fine until we find a better client-level solution

src/clients.rs Outdated
@@ -70,6 +70,9 @@ pub mod openai;
const DEFAULT_CONNECT_TIMEOUT_SEC: u64 = 60;
const DEFAULT_REQUEST_TIMEOUT_SEC: u64 = 600;
const DEFAULT_GRPC_PROBE_INTERVAL_SEC: u64 = 10;
const DEFAULT_RES_STRATEGY_INTERVAL_SEC: u64 = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a timeout and not an interval, rename to DEFAULT_RES_STRATEGY_TIMEOUT_SEC and update "interval" to "timeout" accordingly in the config field names

src/clients.rs Outdated
Comment on lines 284 to 298
let resolution_strategy =
if let Some(resolution_strategy_override) = &service_config.resolution_strategy {
match resolution_strategy_override.as_str() {
"eager" => ResolutionStrategy::Eager {
timeout: (Duration::from_secs(
service_config
.resolution_strategy_interval
.unwrap_or(DEFAULT_RES_STRATEGY_INTERVAL_SEC),
)),
},
_ => ResolutionStrategy::Lazy,
}
} else {
ResolutionStrategy::Lazy
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: recommend decoupling the timeout and simplifying the match, e.g.

    let resolution_strategy_timeout = Duration::from_secs(
        service_config
            .resolution_strategy_timeout
            .unwrap_or(DEFAULT_RES_STRATEGY_TIMEOUT_SEC),
    );
    let resolution_strategy = match &service_config.resolution_strategy {
        Some(name) if name == "eager" => ResolutionStrategy::Eager {
            timeout: resolution_strategy_timeout,
        },
        _ => ResolutionStrategy::Lazy,
    };

@gkumbhat
Copy link
Collaborator Author

gkumbhat commented Jun 3, 2025

Thanks for the review @declark1 . I have made both of the suggested changes. I'll rebase mine after your PR #391 gets merged and then go with merging this one.

@gkumbhat gkumbhat merged commit a30f1a7 into foundation-model-stack:main Jun 3, 2025
2 checks passed
@gkumbhat gkumbhat deleted the add_retry_on_eager branch June 3, 2025 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants