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

chore(blockifier_reexecution): upload and download reexecution files #2229

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aner-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.159 ms 34.203 ms 34.250 ms]
change: [-4.0284% -2.5403% -1.2213%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mild

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)


crates/blockifier_reexecution/Cargo.toml line 18 at r2 (raw file):

clap = { workspace = true, features = ["cargo", "derive"] }
flate2.workspace = true
google-cloud-storage = "0.22.1"

pending approval

Code quote:

google-cloud-storage = "0.22.1"

crates/blockifier_reexecution/src/main.rs line 245 at r2 (raw file):

        Command::UploadFiles { block_numbers, directory_path } => {
            let directory_path =
                directory_path.unwrap_or("./crates/blockifier_reexecution/resources".to_string());

this string appears several times in this module; please make a named constant

Code quote:

"./crates/blockifier_reexecution/resources"

crates/blockifier_reexecution/src/main.rs line 251 at r2 (raw file):

            let files_prefix: String =
                fs::read_to_string(directory_path.clone() + "/offline_reexecution_files_prefix")

used twice; make it a constant?

Code quote:

 "/offline_reexecution_files_prefix"

crates/blockifier_reexecution/src/main.rs line 258 at r2 (raw file):

            // Get the client with authentication.
            let config = ClientConfig::default().with_auth().await.unwrap();

this will fail if devs didn't log into google cloud?
when would this fail?
we may want a more informative error in an expect()

Code quote:

let config = ClientConfig::default().with_auth().await.unwrap();

crates/blockifier_reexecution/src/main.rs line 271 at r2 (raw file):

                assert!(
                    client
                        .get_object(&GetObjectRequest {

is there a different request type that just queries if the file exists? shame to download the entire thing before returning an error.
or, is this what get_object (as opposed to download_object) does?

Code quote:

GetObjectRequest

crates/blockifier_reexecution/src/main.rs line 289 at r2 (raw file):

                    .upload_object(
                        &UploadObjectRequest {
                            bucket: "reexecution_artifacts".to_string(),

this bucket name should be a constant

Code quote:

"reexecution_artifacts"

crates/blockifier_reexecution/src/main.rs line 305 at r2 (raw file):

            }

            println!("All blocks uploaded successfully.");

can you add a link to the new directory / new objects on GCS here?

Code quote:

println!("All blocks uploaded successfully.");

crates/blockifier_reexecution/src/main.rs line 334 at r2 (raw file):

                            bucket: "reexecution_artifacts".to_string(),
                            object: files_prefix.clone()
                                + &format!("block_{block_number}/reexecution_data.json"),

this string is duplicated many times, sometimes with a different prefix, but still; please put it in a variable / lambda function for single point of truth

Code quote:

"block_{block_number}/reexecution_data.json"

crates/blockifier_reexecution/src/main.rs line 349 at r2 (raw file):

            }

            println!("All blocks downloaded successfully.");

Suggestion:

 println!("All blocks downloaded successfully to {directory_path}.");

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 0% with 113 lines in your changes missing coverage. Please review.

Project coverage is 77.22%. Comparing base (e3165c4) to head (6d3fbd4).
Report is 551 commits behind head on main.

Files with missing lines Patch % Lines
crates/blockifier_reexecution/src/main.rs 0.00% 113 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2229       +/-   ##
===========================================
+ Coverage   40.10%   77.22%   +37.11%     
===========================================
  Files          26      386      +360     
  Lines        1895    40467    +38572     
  Branches     1895    40467    +38572     
===========================================
+ Hits          760    31249    +30489     
- Misses       1100     6910     +5810     
- Partials       35     2308     +2273     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

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