Cache walk_packages in ad-hoc framework at collection time

[leolara]

This improve greatly the collection time with some tests done in my laptop.

Also, clarifies the names of some variables where "module" and "package" were being used incorrectly.

[jtraglia]

Hmm it doesn't seem to make much of a difference for me.

Saves 2.5 seconds. But I suppose that is better.

[leolara]

Hmm it doesn't seem to make much of a difference for me.

Saves 2.5 seconds. But I suppose that is better.

It saves 5 secs in mine, but I guess mine is slower, it is a M1 macbook. And also it depends on the load of other apps. Unless we don't run a server with a very specific config always that only runs this it is very difficult to make a very scientific assetment.

If you argue that the performance gain is not worth it, we can close this.

[leolara]

vscode ➜ /workspaces/consensus-specs (leolara/cache_walk_packages) $ make _pyspec; time make reftests runner=wrong
Using Python 3.12.9 environment at: venv
Resolved 97 packages in 853ms
      Built eth2spec @ file:///workspaces/consensus-specs
Prepared 1 package in 14.93s
Uninstalled 1 package in 393ms
░░░░░░░░░░░░░░░░░░░░ [0/1] Installing wheels...                                                                                                                                                                                                                                                                                                                                                                                warning: Failed to hardlink files; falling back to full copy. This may lead to degraded performance.
         If the cache and target directories are on different filesystems, hardlinking may not be supported.
         If this is intentional, set `export UV_LINK_MODE=copy` or use `--link-mode=copy` to suppress this warning.
Installed 1 package in 667ms
 ~ eth2spec==1.6.0a6 (from file:///workspaces/consensus-specs)
Using Python 3.12.9 environment at: venv
Resolved 97 packages in 1.29s
      Built eth2spec @ file:///workspaces/consensus-specs
Prepared 1 package in 17.11s
Uninstalled 1 package in 309ms
░░░░░░░░░░░░░░░░░░░░ [0/1] Installing wheels...                                                                                                                                                                                                                                                                                                                                                                                warning: Failed to hardlink files; falling back to full copy. This may lead to degraded performance.
         If the cache and target directories are on different filesystems, hardlinking may not be supported.
         If this is intentional, set `export UV_LINK_MODE=copy` or use `--link-mode=copy` to suppress this warning.
Installed 1 package in 1.08s
 ~ eth2spec==1.6.0a6 (from file:///workspaces/consensus-specs)

real    0m42.056s
user    0m31.406s
sys     0m2.734s
vscode ➜ /workspaces/consensus-specs (leolara/cache_walk_packages) $ make _pyspec; time make reftests runner=wrong
Using Python 3.12.9 environment at: venv
Resolved 97 packages in 934ms
      Built eth2spec @ file:///workspaces/consensus-specs
Prepared 1 package in 14.03s
Uninstalled 1 package in 492ms
░░░░░░░░░░░░░░░░░░░░ [0/1] Installing wheels...                                                                                                                                                                                                                                                                                                                                                                                warning: Failed to hardlink files; falling back to full copy. This may lead to degraded performance.
         If the cache and target directories are on different filesystems, hardlinking may not be supported.
         If this is intentional, set `export UV_LINK_MODE=copy` or use `--link-mode=copy` to suppress this warning.
Installed 1 package in 529ms
 ~ eth2spec==1.6.0a6 (from file:///workspaces/consensus-specs)
Using Python 3.12.9 environment at: venv
Resolved 97 packages in 1.25s
      Built eth2spec @ file:///workspaces/consensus-specs
Prepared 1 package in 15.79s
Uninstalled 1 package in 327ms
░░░░░░░░░░░░░░░░░░░░ [0/1] Installing wheels...                                                                                                                                                                                                                                                                                                                                                                                warning: Failed to hardlink files; falling back to full copy. This may lead to degraded performance.
         If the cache and target directories are on different filesystems, hardlinking may not be supported.
         If this is intentional, set `export UV_LINK_MODE=copy` or use `--link-mode=copy` to suppress this warning.
Installed 1 package in 545ms
 ~ eth2spec==1.6.0a6 (from file:///workspaces/consensus-specs)

real    0m35.511s
user    0m27.046s
sys     0m2.353s

[leolara]

@jtraglia I fixed the bug, I was storing the interator instead of the list. I have now total=190308

[jtraglia]

@jtraglia I fixed the bug, I was storing the interator instead of the list. I have now total=190308

Hmm this is still a different count (more now). What new tests is this finding?

In my screenshot:

total=189930

[jtraglia]

It saves 5 secs in mine, but I guess mine is slower, it is a M1 macbook.

My desktop is an M1 Mac mini, so it's practically the same as your MacBook btw. So I'm not sure why yours is noticeably slower. And you don't have to do make _pyspec. It's going to build that in make reftests regardless, since they are separate commands.

If you argue that the performance gain is not worth it, we can close this.

Performance wise, probably not worth it. But if it's finding missing test cases, definitely worth merging.

[leolara]

I am going to get the arrays of the collected tests in both versions and check why is the difference

[leolara]

@jtraglia I am getting 193198 total in both master and here, after merging master. I think the reason for the divergence was different versions from master and the commit this was based on.

[jtraglia]

I am getting 193198 total in both master and here, after merging master.

Ah I see. Okay thanks for double checking that. Makes sense.

If you argue that the performance gain is not worth it, we can close this.

I think I'd like to make this argument. The extra complexity is not worth the small performance improvement.

[jtraglia]

@leolara these don't match for me. Using the latest commit here after I pulling changes from master.

193619 here vs 193295 in master. Why is there a difference?

[leolara]

I check again

[leolara]

@jtraglia check the code, I have added an assertion that checks they must be the same

[jtraglia]

@jtraglia check the code, I have added an assertion that checks they must be the same

Have you compared to master like I did?

[jtraglia]

But yes, I see your new assertion. I will look into it.

[leolara]

Yes I a different when running make reftests k=wrongwrong in the summary: 197480 in this branch and 197562 in master. I will print the packages and see if the lists differ.

[leolara]

These are the test cases that are different:

181473,181554d181472
< compute_cells_case_valid_0
< compute_cells_case_valid_1
< compute_cells_case_valid_2
< compute_cells_case_valid_3
< compute_cells_case_valid_4
< compute_cells_case_valid_5
< compute_cells_case_valid_6
< compute_cells_invalid_blob_0
< compute_cells_invalid_blob_1
< compute_cells_invalid_blob_2
< compute_cells_invalid_blob_3
< compute_cells_and_kzg_proofs_case_valid_0
< compute_cells_and_kzg_proofs_case_valid_1
< compute_cells_and_kzg_proofs_case_valid_2
< compute_cells_and_kzg_proofs_case_valid_3
< compute_cells_and_kzg_proofs_case_valid_4
< compute_cells_and_kzg_proofs_case_valid_5
< compute_cells_and_kzg_proofs_case_valid_6
< compute_cells_and_kzg_proofs_case_invalid_blob_0
< compute_cells_and_kzg_proofs_case_invalid_blob_1
< compute_cells_and_kzg_proofs_case_invalid_blob_2
< compute_cells_and_kzg_proofs_case_invalid_blob_3
< verify_cell_kzg_proof_batch_case_valid_0
< verify_cell_kzg_proof_batch_case_valid_1
< verify_cell_kzg_proof_batch_case_valid_2
< verify_cell_kzg_proof_batch_case_valid_3
< verify_cell_kzg_proof_batch_case_valid_4
< verify_cell_kzg_proof_batch_case_valid_5
< verify_cell_kzg_proof_batch_case_valid_6
< verify_cell_kzg_proof_batch_case_valid_zero_cells
< verify_cell_kzg_proof_batch_case_valid_multiple_blobs
< verify_cell_kzg_proof_batch_case_valid_same_cell_multiple_times
< verify_cell_kzg_proof_batch_case_valid_not_sorted
< verify_cell_kzg_proof_batch_case_valid_regression1
< verify_cell_kzg_proof_batch_case_incorrect_commitment
< verify_cell_kzg_proof_batch_case_incorrect_cell
< verify_cell_kzg_proof_batch_case_incorrect_proof
< verify_cell_kzg_proof_batch_case_invalid_commitment_0
< verify_cell_kzg_proof_batch_case_invalid_commitment_1
< verify_cell_kzg_proof_batch_case_invalid_commitment_2
< verify_cell_kzg_proof_batch_case_invalid_commitment_3
< verify_cell_kzg_proof_batch_case_invalid_cell_index
< verify_cell_kzg_proof_batch_case_invalid_cell_0
< verify_cell_kzg_proof_batch_case_invalid_cell_1
< verify_cell_kzg_proof_batch_case_invalid_cell_2
< verify_cell_kzg_proof_batch_case_invalid_cell_3
< verify_cell_kzg_proof_batch_case_invalid_proof_0
< verify_cell_kzg_proof_batch_case_invalid_proof_1
< verify_cell_kzg_proof_batch_case_invalid_proof_2
< verify_cell_kzg_proof_batch_case_invalid_proof_3
< verify_cell_kzg_proof_batch_case_invalid_missing_commitment
< verify_cell_kzg_proof_batch_case_invalid_missing_cell_index
< verify_cell_kzg_proof_batch_case_invalid_missing_cell
< verify_cell_kzg_proof_batch_case_invalid_missing_proof
< recover_cells_and_kzg_proofs_case_valid_no_missing
< recover_cells_and_kzg_proofs_case_valid_half_missing_every_other_cell
< recover_cells_and_kzg_proofs_case_valid_half_missing_first_half
< recover_cells_and_kzg_proofs_case_valid_half_missing_second_half
< recover_cells_and_kzg_proofs_case_invalid_all_cells_are_missing
< recover_cells_and_kzg_proofs_case_invalid_more_than_half_missing
< recover_cells_and_kzg_proofs_case_invalid_more_cells_than_cells_per_ext_blob
< recover_cells_and_kzg_proofs_case_invalid_cell_index
< recover_cells_and_kzg_proofs_case_invalid_cell_0
< recover_cells_and_kzg_proofs_case_invalid_cell_1
< recover_cells_and_kzg_proofs_case_invalid_cell_2
< recover_cells_and_kzg_proofs_case_invalid_cell_3
< recover_cells_and_kzg_proofs_case_invalid_more_cell_indices_than_cells
< recover_cells_and_kzg_proofs_case_invalid_more_cells_than_cell_indices
< recover_cells_and_kzg_proofs_case_invalid_duplicate_cell_index
< recover_cells_and_kzg_proofs_case_invalid_shuffled_no_missing
< recover_cells_and_kzg_proofs_case_invalid_shuffled_one_missing
< recover_cells_and_kzg_proofs_case_invalid_shuffled_half_missing
< compute_verify_cell_kzg_proof_batch_challenge_case_empty
< compute_verify_cell_kzg_proof_batch_challenge_case_single_cell
< compute_verify_cell_kzg_proof_batch_challenge_case_multiple_cells_single_blob
< compute_verify_cell_kzg_proof_batch_challenge_case_multiple_cells_multiple_blobs
< compute_verify_cell_kzg_proof_batch_challenge_case_duplicate_cells
< compute_verify_cell_kzg_proof_batch_challenge_case_many_cells
< compute_verify_cell_kzg_proof_batch_challenge_case_non_sequential_indices
< compute_verify_cell_kzg_proof_batch_challenge_case_mixed_commitment_indices
< compute_verify_cell_kzg_proof_batch_challenge_case_max_cell_indices
< compute_verify_cell_kzg_proof_batch_challenge_case_all_cells

[leolara]

It is weird, the last commit doesn't show in the diff of the PR. That might be the cause.

[leolara]

I am getting the same exact tests now.

[jtraglia]

LGTM. Can confirm they are the same now.