-
Notifications
You must be signed in to change notification settings - Fork 258
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
refactor(2671): move retry logic and implement tokio_retry #2674
Changes from 14 commits
376d75c
463373c
a86526f
aaac082
dc811b8
f2f4aed
fc68ada
2a1e1d7
0feb36b
0be7933
5ee7b1a
49863dd
34a994b
d27ae73
e3d9235
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,46 @@ | ||
use derive_more::From; | ||
use strum_macros::Display; | ||
use reqwest::StatusCode; | ||
use thiserror::Error; | ||
|
||
#[derive(Debug, From, Display, thiserror::Error)] | ||
#[derive(Debug, Error)] | ||
pub enum WebcError { | ||
#[error("Response failed with status {status}: {body}")] | ||
ResponseFailedStatus { status: StatusCode, body: String }, | ||
#[error("Reqwest error: {0}")] | ||
Reqwest(#[from] reqwest::Error), | ||
} | ||
|
||
#[derive(Debug, Error)] | ||
pub enum Error { | ||
#[error("GenAI error: {0}")] | ||
GenAI(genai::Error), | ||
#[error("Webc error: {0}")] | ||
Webc(WebcError), | ||
#[error("Empty response")] | ||
EmptyResponse, | ||
Serde(serde_json::Error), | ||
#[error("Serde error: {0}")] | ||
Serde(#[from] serde_json::Error), | ||
} | ||
|
||
impl From<genai::Error> for Error { | ||
fn from(err: genai::Error) -> Self { | ||
if let genai::Error::WebModelCall { webc_error, .. } = &err { | ||
let error_str = webc_error.to_string(); | ||
if error_str.contains("ResponseFailedStatus") { | ||
// Extract status and body from the error message | ||
let parts: Vec<&str> = error_str.splitn(3, ": ").collect(); | ||
if parts.len() >= 3 { | ||
if let Ok(status) = parts[1].parse::<u16>() { | ||
return Error::Webc(WebcError::ResponseFailedStatus { | ||
status: StatusCode::from_u16(status) | ||
.unwrap_or(StatusCode::INTERNAL_SERVER_ERROR), | ||
body: parts[2].to_string(), | ||
}); | ||
} | ||
} | ||
} | ||
}; | ||
err.into() | ||
} | ||
} | ||
|
||
onyedikachi-david marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pub type Result<A> = std::result::Result<A, Error>; | ||
pub type Result<T> = std::result::Result<T, Error>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,6 +94,7 @@ | |
} | ||
|
||
pub async fn generate(&mut self, config: &Config) -> Result<HashMap<String, String>> { | ||
|
||
let mut new_name_mappings: HashMap<String, String> = HashMap::new(); | ||
// Filter out root operation types and types with non-auto-generated names | ||
let types_to_be_processed = config | ||
|
@@ -123,6 +124,7 @@ | |
.collect(), | ||
}; | ||
|
||
|
||
let mut delay = 3; | ||
loop { | ||
Comment on lines
128
to
129
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. loop is not needed anymore |
||
let answer = self.wizard.ask(question.clone()).await; | ||
|
@@ -137,32 +139,19 @@ | |
new_name_mappings.insert(type_name.to_owned(), name); | ||
break; | ||
} | ||
tracing::info!( | ||
"Suggestions for {}: [{}] - {}/{}", | ||
type_name, | ||
name, | ||
i + 1, | ||
total | ||
); | ||
|
||
// TODO: case where suggested names are already used, then extend the base | ||
// question with `suggest different names, we have already used following | ||
// names: [names list]` | ||
new_name_mappings.insert(name, type_name.to_owned()); | ||
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Formatter and Lint Check
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Formatter and Lint Check
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Check Examples
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-x64-musl
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on darwin-x64
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-x64-gnu
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-arm64-musl
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-ia32-gnu
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-arm64-gnu
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on darwin-arm64
Check failure on line 142 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on win32-x64-msvc
|
||
break; | ||
} | ||
Err(e) => { | ||
// TODO: log errors after certain number of retries. | ||
if let Error::GenAI(_) = e { | ||
// TODO: retry only when it's required. | ||
tracing::warn!( | ||
"Unable to retrieve a name for the type '{}'. Retrying in {}s", | ||
type_name, | ||
delay | ||
); | ||
tokio::time::sleep(tokio::time::Duration::from_secs(delay)).await; | ||
delay *= std::cmp::min(delay * 2, 60); | ||
} | ||
} | ||
tracing::info!( | ||
"Suggestions for {}: [{}] - {}/{}", | ||
type_name, | ||
name, | ||
i + 1, | ||
total | ||
); | ||
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Formatter and Lint Check
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Formatter and Lint Check
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Formatter and Lint Check
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Check Examples
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-x64-musl
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on darwin-x64
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-x64-gnu
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-arm64-musl
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-ia32-gnu
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-arm64-gnu
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on darwin-arm64
Check failure on line 151 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on win32-x64-msvc
|
||
} | ||
Err(e) => { | ||
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Formatter and Lint Check
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Formatter and Lint Check
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Formatter and Lint Check
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Check Examples
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-x64-musl
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on darwin-x64
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-x64-gnu
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-arm64-musl
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-ia32-gnu
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on linux-arm64-gnu
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on darwin-arm64
Check failure on line 153 in src/cli/llm/infer_type_name.rs GitHub Actions / Run Tests on win32-x64-msvc
|
||
tracing::error!("Failed to generate name for {}: {:?}", type_name, e); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
use super::error::{Error, Result, WebcError}; | ||
use derive_setters::Setters; | ||
use genai::adapter::AdapterKind; | ||
use genai::chat::{ChatOptions, ChatRequest, ChatResponse}; | ||
use genai::resolver::AuthResolver; | ||
use genai::Client; | ||
|
||
use super::Result; | ||
use reqwest::StatusCode; | ||
use tokio_retry::strategy::{jitter, ExponentialBackoff}; | ||
use tokio_retry::RetryIf; | ||
|
||
#[derive(Setters, Clone)] | ||
pub struct Wizard<Q, A> { | ||
|
@@ -40,13 +42,23 @@ impl<Q, A> Wizard<Q, A> { | |
|
||
pub async fn ask(&self, q: Q) -> Result<A> | ||
where | ||
Q: TryInto<ChatRequest, Error = super::Error>, | ||
A: TryFrom<ChatResponse, Error = super::Error>, | ||
Q: TryInto<ChatRequest, Error = Error> + Clone, | ||
A: TryFrom<ChatResponse, Error = Error>, | ||
{ | ||
let response = self | ||
.client | ||
.exec_chat(self.model.as_str(), q.try_into()?, None) | ||
.await?; | ||
A::try_from(response) | ||
let retry_strategy = ExponentialBackoff::from_millis(1000).map(jitter).take(5); | ||
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. the base for the strategy is too high, please, check the doc, it'll be Consider:
|
||
|
||
RetryIf::spawn( | ||
retry_strategy, | ||
|| async { | ||
let request = q.clone().try_into()?; | ||
self.client | ||
.exec_chat(self.model.as_str(), request, None) | ||
.await | ||
.map_err(Error::from) | ||
.and_then(A::try_from) | ||
}, | ||
|err: &Error| matches!(err, Error::Webc(WebcError::ResponseFailedStatus { status, .. }) if *status == StatusCode::TOO_MANY_REQUESTS) | ||
) | ||
.await | ||
} | ||
} |
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.
You might not need so much code since this is merged — jeremychone/rust-genai@736bbec
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.
Oh, great.
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.
@tusharmath Pls, why is this repo: https://github.dev/laststylebender14/rust-genai being used instead of the main genai repo: https://github.com/jeremychone/rust-genai
jeremychone's latest lib.rs:
laststylebender14's latest commit hash lib.rs:
So the webc is still private.
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.
@laststylebender14 can you update your Repo with the latest changes? Also can you transfer ownership to
tailcallhq
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.
@onyedikachi-david I have taken the latest changes here — https://github.com/tailcallhq/rust-genai
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.
Okay
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.
please apply discussed changes: