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.

Sign up for GitHub

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 test config in CI #770

Merged
merged 9 commits into from
Nov 6, 2024
Merged

Use test config in CI #770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bf97b07
use test config
sai-deng Nov 4, 2024
02d76b3
changes
sai-deng Nov 5, 2024
2742903
fix
sai-deng Nov 5, 2024
6acd3e9
fix
sai-deng Nov 5, 2024
0ed7a71
increase time
sai-deng Nov 5, 2024
612d456
address comments
sai-deng Nov 5, 2024
8de1a41
add comments
sai-deng Nov 5, 2024
e4b17ac
address comments
sai-deng Nov 6, 2024
0fa6e50
fix
sai-deng Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,26 @@ jobs:
CARGO_INCREMENTAL: 1
RUST_BACKTRACE: 1

run_ignored_tests:
name: Slow evm_arithmetization tests in release mode
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- name: Checkout sources
uses: actions/checkout@v4

- uses: actions-rust-lang/setup-rust-toolchain@v1

- name: Set up rust cache
uses: Swatinem/rust-cache@v2
with:
cache-on-failure: true

- name: Run specific ignored tests in release mode
run: |
cargo test --release \
--test empty_tables --test erc721 --test two_to_one_block -- --ignored

test_zero_bin:
name: Test zero_bin
runs-on: ubuntu-latest
Expand Down Expand Up @@ -173,4 +193,4 @@ jobs:
uses: actions/checkout@v3

- name: Run the script
run: ./scripts/prove_stdio.sh artifacts/witness_b3_b6.json
run: ./scripts/prove_stdio.sh artifacts/witness_b3_b6.json "" use_test_config
4 changes: 4 additions & 0 deletions evm_arithmetization/tests/empty_tables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ use plonky2::util::timing::TimingTree;

/// This test focuses on testing zkVM proofs with some empty tables.
#[test]
// This test is run in CI under the "Run Specific Ignored Tests in Release Mode" job.
// It is marked as ignored to prevent it from running by default in debug mode due to its longer
// execution time.
#[ignore]
fn empty_tables() -> anyhow::Result<()> {
type F = GoldilocksField;
const D: usize = 2;
Expand Down
4 changes: 4 additions & 0 deletions evm_arithmetization/tests/erc721.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ type C = KeccakGoldilocksConfig;
/// `1337` from address `0x5B38Da6a701c568545dCfcB03FcB875f56beddC4` to address
/// `0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2`.
#[test]
// This test is run in CI under the "Run Specific Ignored Tests in Release Mode" job.
// It is marked as ignored to prevent it from running by default in debug mode due to its longer
// execution time.
#[ignore]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is erc721 taking that long that we're ignoring it on default?

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, I found that the test running on CI is about 4x slower than on my MacBook. The test time in release mode on my MacBook can be found here: #739

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to start doing test selection like this, I think we need a better test runner (cargo-nextest)

It allows you to categorise your tests, have different timeouts, profiles for ci etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, we only need minimal functionality, so our existing setup works fine. Using cargo-nextest would indeed be beneficial, and we can look at converting to it in a future PR.

fn test_erc721() -> anyhow::Result<()> {
init_logger();

Expand Down
4 changes: 4 additions & 0 deletions evm_arithmetization/tests/two_to_one_block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@ fn get_test_block_proof(
}

#[test]
// This test is run in CI under the "Run Specific Ignored Tests in Release Mode" job.
// It is marked as ignored to prevent it from running by default in debug mode due to its longer
// execution time.
#[ignore]
fn test_two_to_one_block_aggregation() -> anyhow::Result<()> {
init_logger();
let some_timestamps = [127, 42, 65, 43];
Expand Down
1 change: 1 addition & 0 deletions scripts/prove_rpc.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ else
echo "Proving blocks from ($START_BLOCK) to ($END_BLOCK)"
command="cargo r --release --package zero --bin leader -- \
--runtime in-memory \
--use-test-config \
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure we want the script to always use the testing config, rather take it as optional argument perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume we always use the testing config in CI tests, while our cron jobs use the standard config?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we had said in the dev sync we'd use a mix of both in CI? I'm fine either way though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Keep Execute bash script to generate and verify a proof for a small block. with default config.

--load-strategy on-demand \
--proof-output-dir $PROOF_OUTPUT_DIR \
--block-batch-size $BLOCK_BATCH_SIZE \
Expand Down
14 changes: 10 additions & 4 deletions scripts/prove_stdio.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export RUSTFLAGS='-C target-cpu=native -Zlinker-features=-lld'

INPUT_FILE=$1
TEST_ONLY=$2
USE_TEST_CONFIG=$3

if [[ $INPUT_FILE == "" ]]; then
echo "Please provide witness json input file, e.g. artifacts/witness_b19240705.json"
Expand Down Expand Up @@ -117,12 +118,17 @@ fi

cargo build --release --jobs "$num_procs"


start_time=$(date +%s%N)
"${REPO_ROOT}/target/release/leader" --runtime in-memory \

cmd=("${REPO_ROOT}/target/release/leader" --runtime in-memory \
--load-strategy on-demand -n 1 \
--block-batch-size "$BLOCK_BATCH_SIZE" \
--proof-output-dir "$PROOF_OUTPUT_DIR" stdio < "$INPUT_FILE" &> "$OUTPUT_LOG"
--block-batch-size "$BLOCK_BATCH_SIZE")

if [[ "$USE_TEST_CONFIG" == "use_test_config" ]]; then
cmd+=("--use-test-config")
fi

"${cmd[@]}" --proof-output-dir "$PROOF_OUTPUT_DIR" stdio < "$INPUT_FILE" &> "$OUTPUT_LOG"
end_time=$(date +%s%N)

grep "Successfully wrote to disk proof file " "$OUTPUT_LOG" | awk '{print $NF}' | tee "$PROOFS_FILE_LIST"
Expand Down
Loading