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

regression and benchmark testing between jerigon and zkevm #751

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

Conversation

temaniarpit27
Copy link
Contributor

@temaniarpit27 temaniarpit27 commented Oct 26, 2024

@github-actions github-actions bot added the ci label Oct 26, 2024
@temaniarpit27 temaniarpit27 force-pushed the arpit/704 branch 15 times, most recently from 45eb44d to 581be7e Compare October 26, 2024 19:03
@temaniarpit27 temaniarpit27 changed the title draft for integration testing regression and benchmark testing between jerigon and zkevm Oct 27, 2024
@temaniarpit27 temaniarpit27 force-pushed the arpit/704 branch 11 times, most recently from 9967181 to 18f7058 Compare October 27, 2024 09:17
@temaniarpit27 temaniarpit27 force-pushed the arpit/704 branch 3 times, most recently from 0dceebc to 4dc1e8f Compare October 27, 2024 09:28
- name: Run Erigon Network
run: |
cd ..
tar xf "$(pwd)/zk_evm/test_data/erigon-data.tar.gz" || {
Copy link
Contributor Author

@temaniarpit27 temaniarpit27 Oct 27, 2024

Choose a reason for hiding this comment

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

@Nashtare @praetoriansentry what would be best way to store the file? currently added with repo

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not big, I think it can stay with repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are using a shorter data (672) blocks, if we use that, its probably fine but if we choose to go with a larger set of data, we may need to think

run: |
export ETH_RPC_URL="http://localhost:8545"
rm -rf proofs/* circuits/* ./proofs.json test.out verify.out leader.out
random_numbers=($(shuf -i 1-500 -n 5))
Copy link
Contributor Author

@temaniarpit27 temaniarpit27 Oct 27, 2024

Choose a reason for hiding this comment

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

@Nashtare @praetoriansentry currently using random 5 blocks for integration testing, any pref for block count

Copy link
Contributor

@atanmarko atanmarko Oct 29, 2024

Choose a reason for hiding this comment

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

I don't think it is a good idea to take random 5 blocks. Should be all 500 probably, if we run it once a week. Maybe 10 or 100, if 500 is too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya its just for testing, we will choose whatever we think is fine. i was thinking somewhere around 100

echo "$(date +"%Y-%m-%d %H:%M:%S")" &>> "$OUTPUT_LOG"

# Define the blocks to process
blocks=(100 200 300 400 500)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@praetoriansentry @Nashtare currently using random blocks, any idea which blocks to use for benchmark testing

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could read the list from the config file (CSV to make it simple?).

Copy link
Contributor Author

@temaniarpit27 temaniarpit27 Oct 29, 2024

Choose a reason for hiding this comment

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

yes this is just a placeholder. we will choose block numbers first and if thats too much, we will put that in config file

@temaniarpit27
Copy link
Contributor Author

@praetoriansentry @Nashtare currently using erigon.stopped.0.to.672.tar.gz as erigon data. Anything else we need to use. This is a test network data which has 672 blocks

name: Jerigon Zero Testing

on:
# Uncomment when ready to run on a schedule
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO

@temaniarpit27
Copy link
Contributor Author

Sample output in log file

2024-10-27 09:25:52
Processing block: 1
Proving duration for block 1: 2.523 seconds, performance time: 1.467795802, performance user time: 2.039623000, performance system time: 0.119509000
Processing block: 2
Proving duration for block 2: 1.511 seconds, performance time: 1.498711832, performance user time: 1.577669000, performance system time: 0.125585000
Processing completed at: 2024-10-27 09:25:59

2024-10-27 09:32:03
Processing block: 1
Proving duration for block 1: 2.334 seconds, performance time: 1.487433304, performance user time: 1.674814000, performance system time: 0.129767000
Processing block: 2
Proving duration for block 2: 1.563 seconds, performance time: 1.551011885, performance user time: 1.970833000, performance system time: 0.193141000
Processing completed at: 2024-10-27 09:32:10

@praetoriansentry @Nashtare let me know what more data we want to add to this.

jobs:
jerigon_zero_testing:
name: Jerigon Zero Testing - Integration and Benchmarking
runs-on: zero-ci
Copy link
Contributor

Choose a reason for hiding this comment

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

zero-ci is shared among other tasks. Ben created zero-reg - stronger, and for use only with benchmarks. We should just make sure that we schedule checks at different time.

Comment on lines +8 to +12
push:
branches: [develop]
pull_request:
branches:
- "**"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be removed when PR is debugged, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is just for testing

run: |
export ETH_RPC_URL="http://localhost:8545"
rm -rf proofs/* circuits/* ./proofs.json test.out verify.out leader.out
random_numbers=($(shuf -i 1-500 -n 5))
Copy link
Contributor

@atanmarko atanmarko Oct 29, 2024

Choose a reason for hiding this comment

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

I don't think it is a good idea to take random 5 blocks. Should be all 500 probably, if we run it once a week. Maybe 10 or 100, if 500 is too much.

- name: Run Erigon Network
run: |
cd ..
tar xf "$(pwd)/zk_evm/test_data/erigon-data.tar.gz" || {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not big, I think it can stay with repo.

# Run performance stats
if ! perf stat -e cycles ./target/release/leader --runtime in-memory --load-strategy monolithic --block-batch-size "$BLOCK_BATCH_SIZE" --proof-output-dir "$PROOF_OUTPUT_DIR" stdio < "output_${block}.json" &> "$BLOCK_OUTPUT_LOG"; then
echo "Performance command failed for block: $block" &>> "$OUTPUT_LOG"
cat "$BLOCK_OUTPUT_LOG" &>> "$OUTPUT_LOG"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use one output file to append performance measurements, and the other for error log details. This way, any error that happens will dump big log to the previous performance results and would make them hard to read.

set +o pipefail
if ! cat "$BLOCK_OUTPUT_LOG" | grep "Successfully wrote to disk proof file " | awk '{print $NF}' | tee "$PROOFS_FILE_LIST"; then
echo "Proof list not generated for block: $block. Check the log for details." &>> "$OUTPUT_LOG"
cat "$BLOCK_OUTPUT_LOG" &>> "$OUTPUT_LOG"
Copy link
Contributor

@atanmarko atanmarko Oct 29, 2024

Choose a reason for hiding this comment

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

Is $BLOCK_OUTPUT_LOG the whole leader output terminal log? We should just keep minimal details and time measurements, as we want to track changes over weeks - not to append whole debug/info leader log to the results file. One liner with the block that error-ed in results file, and separate file with whole error log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is printed only in case of failure, if a block proving fails. otherwise we wont know why a block failed
if it succeeds, then we are printing only details about time measurements.
maybe, we can move failure blocks to a new file or something like that

Copy link
Contributor

Choose a reason for hiding this comment

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

You can print 'Proof list not generated for block: $block. Check the log for details."' so that we know the block failed. Then you check the error logs in the other file.

echo "Processing block: $block" &>> "$OUTPUT_LOG"

# Fetch block data
if ! ./target/release/rpc --rpc-url "$ETH_RPC_URL" fetch --start-block "$block" --end-block "$block" > "output_${block}.json"; then
Copy link
Contributor

Choose a reason for hiding this comment

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

output_${block}.json is a bit misleading. Maybe witness_${block}.json?

process_block() {
local block=$1

echo "Processing block: $block" &>> "$OUTPUT_LOG"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the results file minimal - maybe one line with date/time of measurement, and the timings for the blocks. Would be easy that way to open file and see how the performances are changed over weeks and months.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#751 (comment)

this is the expected output
let me know if we need to reduce more. i think this also gives clear info along with being concise

Copy link
Contributor

Choose a reason for hiding this comment

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

Line Processing block: 1 is redundant, does not bring any new information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants