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

Ci run large test #252

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Ci run large test #252

wants to merge 1 commit into from

Conversation

Stavbe
Copy link
Contributor

@Stavbe Stavbe commented Dec 16, 2024

Move big files to google cloud (delete the use of lfs)
Run large files test from ci


This change is Reviewable

@Stavbe Stavbe force-pushed the stav/ci_run_large_test branch 4 times, most recently from 626c4f7 to df21630 Compare December 16, 2024 12:38
@Stavbe Stavbe self-assigned this Dec 16, 2024
Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 13 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)


README.md line 4 at r2 (raw file):

This repo utilizes Google Cloud Storage for test artifacts. The test artifacts can be found and modified at the following URL:
https://console.cloud.google.com/storage/browser/stwo-cairo-testing-artifacts?project=starkware-thirdparties

It's best to put this info next to the test, and delete the false information from the main README
also doc how to add new tests

Code quote:

https://console.cloud.google.com/storage/browser/stwo-cairo-testing-artifacts?project=starkware-thirdparties

stwo_cairo_prover/scripts/run_large_test.sh line 4 at r2 (raw file):

BUCKET_URL="https://storage.googleapis.com/stwo-cairo-testing-artifacts"
DEST_DIR="crates/prover/test_data"

is it in .gitignore?


stwo_cairo_prover/scripts/run_large_test.sh line 5 at r2 (raw file):

BUCKET_URL="https://storage.googleapis.com/stwo-cairo-testing-artifacts"
DEST_DIR="crates/prover/test_data"
TEST_NAME="test_read_from_large_files"

imo this should not be specified,
pull everything and cargo test --features="some_name"
functions tagged with that feature should expect their storage to sit in stwo-cairo-testing-artifacts
basically- it should be easy to add new tests


stwo_cairo_prover/scripts/run_large_test.sh line 31 at r2 (raw file):

# Delete the downloaded files
echo "Cleaning up downloaded files..."
rm -rf "${DEST_DIR}/${TEST_NAME}"*

Sometimes I would like not to clean,
like when I am running locally

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 13 files reviewed, 6 unresolved discussions (waiting on @Stavbe)


stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 211 at r2 (raw file):

        );
        assert_eq!(components.mul_opcode_is_small_t_is_imm_t.len(), 0);
        assert_eq!(components.mul_opcode_is_small_t_is_imm_f.len(), 0);

Does it make sense that those value are 0?

Code quote:

        assert_eq!(components.mul_opcode_is_small_t_is_imm_t.len(), 0);
        assert_eq!(components.mul_opcode_is_small_t_is_imm_f.len(), 0);

README.md line 11 at r2 (raw file):

Download the required input files from Google Cloud Storage.
Execute the test.
Delete the downloaded files after the test is complete.

Suggestion:

This script will:
- Download the required input files from Google Cloud Storage.
- Execute the test.
- Delete the downloaded files after the test is complete.

Copy link
Contributor Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 13 files reviewed, 6 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 211 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Does it make sense that those value are 0?

There is no mul small component currently, so the adapter uses mul big instead (with a todo).


stwo_cairo_prover/scripts/run_large_test.sh line 4 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

is it in .gitignore?

The other script not there on purpoose?


stwo_cairo_prover/scripts/run_large_test.sh line 5 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

imo this should not be specified,
pull everything and cargo test --features="some_name"
functions tagged with that feature should expect their storage to sit in stwo-cairo-testing-artifacts
basically- it should be easy to add new tests

I was trying to do so but the ci didn't like the fetaure header in the toml.
Let's begin with a simpler version?


stwo_cairo_prover/scripts/run_large_test.sh line 31 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

Sometimes I would like not to clean,
like when I am running locally

Why not? You can understand what is written there anyway, and it's easier to not delete manually before commit changes.

Copy link
Contributor Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 13 files reviewed, 6 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)


stwo_cairo_prover/scripts/run_large_test.sh line 31 at r2 (raw file):

Previously, Stavbe wrote…

Why not? You can understand what is written there anyway, and it's easier to not delete manually before commit changes.

can't...

@Stavbe Stavbe force-pushed the stav/ci_run_large_test branch from dd6a990 to 3dead77 Compare December 16, 2024 15:49
Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 6 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)


stwo_cairo_prover/scripts/run_large_test.sh line 4 at r2 (raw file):

Previously, Stavbe wrote…

The other script not there on purpoose?

I meant the directory written in that line


stwo_cairo_prover/scripts/run_large_test.sh line 5 at r2 (raw file):

Previously, Stavbe wrote…

I was trying to do so but the ci didn't like the fetaure header in the toml.
Let's begin with a simpler version?

yes unless you can fix it quick then ill be ok with it here because --ignored is wrong (we have ignored tests that wont pass, or shouldn't i.e. the small mull)


stwo_cairo_prover/scripts/run_large_test.sh line 31 at r2 (raw file):

Previously, Stavbe wrote…

can't...

if the folder is git ignored then you never have to remove, download once and work with it.
if I'm on the train I can't run tests? unplesent...

Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 6 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)


stwo_cairo_prover/scripts/run_large_test.sh line 5 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

yes unless you can fix it quick then ill be ok with it here because --ignored is wrong (we have ignored tests that wont pass, or shouldn't i.e. the small mull)

and for the TEST_NAME var, just remove it and download everything from the cloud folder no?

@Stavbe Stavbe force-pushed the stav/ci_run_large_test branch from 3dead77 to c9c3521 Compare December 19, 2024 14:15
@Stavbe Stavbe force-pushed the stav/ci_run_large_test branch from c9c3521 to 979bb8e Compare December 19, 2024 14:16
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