Skip to content
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

Assign specific jobs to dedicated workers #564

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Conversation

temaniarpit27
Copy link
Contributor

@github-actions github-actions bot added the crate: zero_bin Anything related to the zero-bin subcrates. label Aug 29, 2024
Cargo.toml Outdated Show resolved Hide resolved
zero_bin/README.md Outdated Show resolved Hide resolved
@temaniarpit27 temaniarpit27 changed the title separate workers and jobs Assign specific jobs to dedicated workers Aug 29, 2024
zero_bin/tools/prove_rpc.sh Outdated Show resolved Hide resolved
zero_bin/tools/prove_rpc.sh Outdated Show resolved Hide resolved
zero_bin/leader/src/client.rs Outdated Show resolved Hide resolved
zero_bin/leader/src/http.rs Outdated Show resolved Hide resolved
@temaniarpit27 temaniarpit27 marked this pull request as ready for review September 3, 2024 13:14
zero_bin/leader/src/cli.rs Outdated Show resolved Hide resolved
zero_bin/leader/src/main.rs Outdated Show resolved Hide resolved
zero_bin/leader/src/main.rs Outdated Show resolved Hide resolved
zero_bin/leader/src/main.rs Outdated Show resolved Hide resolved
Copy link
Member

@muursh muursh left a comment

Choose a reason for hiding this comment

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

How have we tested this? Have we tested edge cases? Error cases?

@temaniarpit27
Copy link
Contributor Author

How have we tested this? Have we tested edge cases? Error cases?

Tested this on single and multi node machine. Tried to cover all the cases. Not sure if you have anything specific in mind
Started multiple workers on multiple nodes. Let me know if you have anything in mind and see if i have covered that

@temaniarpit27
Copy link
Contributor Author

@muursh One thing we might to check is when queue messages expire.

@muursh
Copy link
Member

muursh commented Sep 5, 2024

@muursh One thing we might to check is when queue messages expire.

Yeah for sure. We should also open issues for the bits we discussed.

@temaniarpit27
Copy link
Contributor Author

@muursh One thing we might to check is when queue messages expire.

Yeah for sure. We should also open issues for the bits we discussed.

Have asked leo to also confirm on a few test cases

Copy link
Member

@atanmarko atanmarko left a comment

Choose a reason for hiding this comment

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

Lots of changes in the develop zero_bin project structure, will take a look one more time after rebase of this pr on top of develop

zero_bin/leader/src/main.rs Outdated Show resolved Hide resolved
zero_bin/leader/src/main.rs Outdated Show resolved Hide resolved
@@ -78,7 +78,7 @@ num-bigint = "0.4.5"
num-traits = "0.2.19"
nunny = "0.2.1"
once_cell = "1.19.0"
paladin-core = "0.4.2"
paladin-core = { git = "https://github.com/0xPolygonZero/paladin.git", branch = "main" }
Copy link
Member

Choose a reason for hiding this comment

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

@muursh Are we going to release new paladin version or we are targeting github head?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaik we will be releasing a new version

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we should release a new version. @temaniarpit27 is everything ready to go for a new release?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we get actual benchmark measurements of this approach?

Copy link
Contributor Author

@temaniarpit27 temaniarpit27 Sep 19, 2024

Choose a reason for hiding this comment

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

no we haven't benchmaked it yet

zero_bin/common/src/proof_runtime.rs Outdated Show resolved Hide resolved
zero/src/bin/leader.rs Outdated Show resolved Hide resolved
@0xaatif 0xaatif mentioned this pull request Sep 26, 2024
@temaniarpit27
Copy link
Contributor Author

@Nashtare final review from your side pls

Comment on lines +29 to +30
// Mode to use for worker for setup (affinity or default)
#[arg(long = "worker-run-mode", help_heading = WORKER_HELP_HEADING, value_enum, default_value = "default")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's default here? unified?

Comment on lines +36 to +37
pub light_proof_runtime: Runtime,
pub heavy_proof_runtime: Runtime,
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor, but I'd remove _runtime from the field names, as this is already implied by the parent

@@ -28,6 +32,11 @@ use crate::fs::generate_block_proof_file_name;
use crate::ops;
use crate::proof_types::GeneratedBlockProof;

pub struct ProofRuntime {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be nice to have some high-level explanation of this struct, i.e. its purpose to distinguish base STARK proof runtimes vs aggregation ones.

Comment on lines +35 to 38
pub enum WorkerRunMode {
Affinity,
Default,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing doc, we don't really know what this does just looking at it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Status: Ready to Review
Development

Successfully merging this pull request may close these issues.

5 participants