-
Notifications
You must be signed in to change notification settings - Fork 1
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
Use a single GPU for proof generation #9
base: main
Are you sure you want to change the base?
Conversation
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.
Looks okay to me, but I don't have much context on this!
Left some comments on possible refactorings and some remaining TODO comments.
@@ -167,8 +174,13 @@ impl ProofActor { | |||
}; | |||
} | |||
} | |||
let mut tasks = tasks.lock().await; | |||
tasks.remove(&key); | |||
// TODO think |
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.
What is this TODO about?
{ | ||
let mut gpus = gpus.lock().await; | ||
gpus.push_back(proof_request.gpu_number.unwrap()); | ||
let mut tasks = tasks.lock().await; | ||
tasks.remove(&key); | ||
} |
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.
Maybe better to do this instead? Not an issue here but usually bad to unnecessarily nest mutex locks.
{ | |
let mut gpus = gpus.lock().await; | |
gpus.push_back(proof_request.gpu_number.unwrap()); | |
let mut tasks = tasks.lock().await; | |
tasks.remove(&key); | |
} | |
{ | |
let mut gpus = gpus.lock().await; | |
gpus.push_back(proof_request.gpu_number.unwrap()); | |
} | |
{ | |
let mut tasks = tasks.lock().await; | |
tasks.remove(&key); | |
} |
@@ -274,6 +296,7 @@ impl ProofActor { | |||
Message::TaskComplete(req) => { | |||
// pop up pending task if any task complete | |||
debug!("Message::TaskComplete({req:?})"); | |||
// TODO fix |
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.
Fix what?
// Set gpu number for task | ||
if proof_request.gpu_number.is_some() { panic!("GPU number is already set"); } | ||
let mut available_gpus = self.available_gpus.lock().await; | ||
if let Some(gpu_number) = available_gpus.pop_front() { | ||
proof_request.set_gpu_number(Some(gpu_number)); | ||
} else { | ||
panic!("No available GPU"); | ||
} | ||
drop(available_gpus); | ||
|
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.
Maybe this? It uses a closure so no need for an explicit drop too. Not a Rust expert though, so must be taken with caution!
// Set gpu number for task | |
if proof_request.gpu_number.is_some() { panic!("GPU number is already set"); } | |
let mut available_gpus = self.available_gpus.lock().await; | |
if let Some(gpu_number) = available_gpus.pop_front() { | |
proof_request.set_gpu_number(Some(gpu_number)); | |
} else { | |
panic!("No available GPU"); | |
} | |
drop(available_gpus); | |
proof_request.set_gpu_number(Some({ | |
proof_request.gpu_number | |
.as_ref() | |
.inspect(|_| panic!("GPU number is already set")); | |
self.available_gpus | |
.lock() | |
.await | |
.pop_front() | |
.ok_or_else(|| panic!("No available GPU")) | |
})); |
@@ -287,11 +293,18 @@ impl Prover for Sp1Prover { | |||
} | |||
} | |||
|
|||
// TODO it is None now |
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.
What is None?
No description provided.