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

feat(ci): add native dependencies to the CI (dynamic docker) #726

Merged
merged 57 commits into from
Sep 17, 2024

Conversation

varex83
Copy link
Contributor

@varex83 varex83 commented Sep 5, 2024

This change is Reviewable

Copy link
Contributor

@alon-dotan-starkware alon-dotan-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 25 files at r1.
Reviewable status: 1 of 26 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @meship-starkware, and @varex83)


.github/actions/setup-native-deps/action.yml line 25 at r1 (raw file):

        sudo apt-get install -y zstd
        sudo apt-get install llvm-18 llvm-18-dev llvm-18-runtime clang-18 clang-tools-18 lld-18 libpolly-18-dev libmlir-18-dev mlir-18-tools
        sudo apt-get install -y libgmp3-dev

script it and use it in the docker build and in the action

Code quote:

    - name: Add llvm deb repository
      uses: myci-actions/add-deb-repo@11
      with:
        repo: deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-18 main
        repo-name: llvm-repo
        keys-asc: https://apt.llvm.org/llvm-snapshot.gpg.key

    - name: Install LLVM and gmplib
      shell: bash
      run: |
        sudo apt-get update && sudo apt-get upgrade -y
        sudo apt-get install -y zstd
        sudo apt-get install llvm-18 llvm-18-dev llvm-18-runtime clang-18 clang-tools-18 lld-18 libpolly-18-dev libmlir-18-dev mlir-18-tools
        sudo apt-get install -y libgmp3-dev

Copy link
Contributor

@alon-dotan-starkware alon-dotan-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 4 files at r2, all commit messages.
Reviewable status: 3 of 26 files reviewed, all discussions resolved (waiting on @avi-starkware and @meship-starkware)

Copy link
Contributor

@alon-dotan-starkware alon-dotan-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 3 of 26 files reviewed, all discussions resolved (waiting on @avi-starkware and @meship-starkware)

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.548 ms 65.625 ms 65.715 ms]
change: [-8.3804% -5.1999% -2.4495%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.883 ms 66.173 ms 66.652 ms]
change: [-9.3833% -5.8155% -2.7325%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
4 (4.00%) high mild
3 (3.00%) high severe

full_committer_flow performance improved 😺
full_committer_flow time: [47.440 ms 47.495 ms 47.555 ms]
change: [-1.8579% -1.6609% -1.4791%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severe

Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 25 files at r1, 2 of 4 files at r2, 2 of 2 files at r18, 1 of 1 files at r19, 1 of 3 files at r20, 1 of 2 files at r21, 6 of 6 files at r22, 1 of 1 files at r23, 1 of 1 files at r24, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware and @meship-starkware)

Copy link
Collaborator

@noaov1 noaov1 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 @avi-starkware, @meship-starkware, and @varex83)


crates/blockifier/feature_contracts/cairo1/compiled/account_faulty.casm.json line 3 at r24 (raw file):

{
  "prime": "0x800000000000011000000000000000000000000000000000000000000000001",
  "compiler_version": "2.8.2",

Did you recompile those contracts with 2.8.2 version?

Code quote:

"compiler_version": "2.8.2",

Copy link
Contributor Author

@varex83 varex83 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 @avi-starkware, @meship-starkware, and @noaov1)


crates/blockifier/feature_contracts/cairo1/compiled/account_faulty.casm.json line 3 at r24 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Did you recompile those contracts with 2.8.2 version?

yes, since the feature contracts test was arguing

Copy link
Collaborator

@noaov1 noaov1 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware and @meship-starkware)

@noaov1 noaov1 merged commit c1f680a into main Sep 17, 2024
30 checks passed
@noaov1 noaov1 deleted the fix/native-dynamic-ci branch September 17, 2024 08:18
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2024
@rodrigo-pino
Copy link
Contributor

Solution given in #726

@rodrigo-pino rodrigo-pino added the native integration Related with the integration of Cairo Native into the Blockifier label Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
native integration Related with the integration of Cairo Native into the Blockifier
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants