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: make sure that the cairo_native flag works in python #2465

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

meship-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch 5 times, most recently from 7ee2b5e to 7aa7fed Compare December 5, 2024 12:52
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 9.28%. Comparing base (e3165c4) to head (cecfbcb).
Report is 781 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #2465       +/-   ##
==========================================
- Coverage   40.10%   9.28%   -30.82%     
==========================================
  Files          26     138      +112     
  Lines        1895   18350    +16455     
  Branches     1895   18350    +16455     
==========================================
+ Hits          760    1704      +944     
- Misses       1100   16305    +15205     
- Partials       35     341      +306     

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

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch from 7aa7fed to 3c13353 Compare December 5, 2024 16:24
@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch from 3c13353 to 7c5864a Compare December 8, 2024 10:27
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.

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @Yoni-Starkware)


a discussion (no related file):
blocking (this is only a test PR and shouldn't be merged, right?)

Copy link
Contributor Author

@meship-starkware meship-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 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @Yoni-Starkware)


a discussion (no related file):

Previously, dorimedini-starkware wrote…

blocking (this is only a test PR and shouldn't be merged, right?)

Yes, I don't know why You and Yoni are reviewers

@meship-starkware meship-starkware removed the request for review from Yoni-Starkware December 8, 2024 15:15
Copy link
Collaborator

@Yoni-Starkware Yoni-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 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)


a discussion (no related file):

Previously, meship-starkware (Meshi Peled) wrote…

Yes, I don't know why You and Yoni are reviewers

Does it work?
Let's merge it! to make sure native does not break

:lgtm:

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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @Yoni-Starkware)


a discussion (no related file):

Previously, Yoni-Starkware (Yoni) wrote…

Does it work?
Let's merge it! to make sure native does not break

:lgtm:

what? why?
we don't (I think) want to add native to all CI, just the feature-combo build...
(change the blockifier ==> change the native blockifier ==> trigger tests with native active)

and if this is to be merged, I have an objection to initializing the submodule in different places (in the main CI);
can the checkout step be part of the bootstrapping action...?
if you are running the bootstrap action you are relying on scripts from the repo, so the checkout should probably coupled with the bootstrap anyway

Copy link
Collaborator

@Yoni-Starkware Yoni-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: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)


a discussion (no related file):

Previously, dorimedini-starkware wrote…

what? why?
we don't (I think) want to add native to all CI, just the feature-combo build...
(change the blockifier ==> change the native blockifier ==> trigger tests with native active)

and if this is to be merged, I have an objection to initializing the submodule in different places (in the main CI);
can the checkout step be part of the bootstrapping action...?
if you are running the bootstrap action you are relying on scripts from the repo, so the checkout should probably coupled with the bootstrap anyway

@meship-starkware worth checking (but keep it mind that we might be able to get rid of the submodule soon)

We will run native E2E very soon, so turning this feature on is a high priority.

Copy link
Contributor Author

@meship-starkware meship-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: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)


a discussion (no related file):

Previously, Yoni-Starkware (Yoni) wrote…

@meship-starkware worth checking (but keep it mind that we might be able to get rid of the submodule soon)

We will run native E2E very soon, so turning this feature on is a high priority.

It does not work. It does not build the artifact because we don't have the Cairo native submodules. I am working on a solution

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch from 7c5864a to cc30089 Compare December 9, 2024 07:43
Copy link
Contributor Author

@meship-starkware meship-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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


.github/workflows/upload_artifacts_workflow.yml line 74 at r2 (raw file):

      - uses: ./.github/actions/bootstrap
      - name: Initialize and update submodules
        run: git submodule update --init --recursive

I don't think this is a good solution; I just want to see if it can build the artifact with that.

Code quote:

      - name: Initialize and update submodules
        run: git submodule update --init --recursive

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch 2 times, most recently from 85c1a15 to f552c5e Compare December 9, 2024 09:48
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 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware and @meship-starkware)


scripts/build_native_blockifier.sh line 23 at r3 (raw file):

}

init_submodule &&

is this the correct syntax for "run and exit if failed"?

Code quote:

init_submodule &&

.github/workflows/main.yml line 117 at r3 (raw file):

          # Fetch the entire history.
          fetch-depth: 0
      - uses: ./.github/actions/bootstrap

should this be part of the bootstrap script? you can init submodules inside it, no?

Code quote:

        with:
          # required to clone native as a gitsubmodule
          submodules: recursive
          # Fetch the entire history.
          fetch-depth: 0
      - uses: ./.github/actions/bootstrap

Copy link
Contributor Author

@meship-starkware meship-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: all files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware and @dorimedini-starkware)


scripts/build_native_blockifier.sh line 23 at r3 (raw file):

Previously, dorimedini-starkware wrote…

is this the correct syntax for "run and exit if failed"?

According to Copilot this is the syntax or run and only run the next command upon success.

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch 3 times, most recently from f552c5e to 47eb9c8 Compare December 9, 2024 11:33
Copy link
Contributor Author

@meship-starkware meship-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: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware, @dorimedini-starkware, and @Yoni-Starkware)


.github/workflows/main.yml line 117 at r3 (raw file):

Previously, dorimedini-starkware wrote…

should this be part of the bootstrap script? you can init submodules inside it, no?

It seems like the - uses: actions/checkout@v4 should happen before calling the bootstrap script because otherwise, it does not know the script.

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch from 47eb9c8 to a17b2fe Compare December 9, 2024 11:36
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 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alon-dotan-starkware and @meship-starkware)


.github/workflows/main.yml line 117 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

It seems like the - uses: actions/checkout@v4 should happen before calling the bootstrap script because otherwise, it does not know the script.

yes but you can checkout without submodule init, and later explicitly init using a git command, during bootstrapping.
I prefer to share this code, rather than duplicate it

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch from a17b2fe to 29ecc4c Compare December 9, 2024 12:48
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 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alon-dotan-starkware and @meship-starkware)


.github/workflows/main.yml line 108 at r5 (raw file):

        with:
          # required to clone native as a gitsubmodule
          submodules: recursive

delete

Code quote:

          # required to clone native as a gitsubmodule
          submodules: recursive

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch from 29ecc4c to 54ca364 Compare December 9, 2024 12:57
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 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alon-dotan-starkware and @meship-starkware)

@meship-starkware meship-starkware force-pushed the meship/check_that_python_works_with_native branch from 54ca364 to cecfbcb Compare December 9, 2024 14:19
Copy link
Contributor Author

@meship-starkware meship-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: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @alon-dotan-starkware and @dorimedini-starkware)


.github/workflows/main.yml line 108 at r5 (raw file):

Previously, dorimedini-starkware wrote…

delete

Done.

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 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alon-dotan-starkware)

@meship-starkware meship-starkware merged commit 036bbc0 into main Dec 10, 2024
12 checks passed
@meship-starkware meship-starkware deleted the meship/check_that_python_works_with_native branch December 10, 2024 06:28
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants