-
Notifications
You must be signed in to change notification settings - Fork 21
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(infra): re-enable cairo native build #989
Conversation
e492e26
to
b28e29d
Compare
Benchmark movements: |
b28e29d
to
331827b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #989 +/- ##
==========================================
- Coverage 74.18% 70.50% -3.69%
==========================================
Files 359 88 -271
Lines 36240 11331 -24909
Branches 36240 11331 -24909
==========================================
- Hits 26886 7989 -18897
+ Misses 7220 2964 -4256
+ Partials 2134 378 -1756
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
331827b
to
f8264ad
Compare
Benchmark movements: |
7d58737
to
77a5bd7
Compare
Benchmark movements: |
e7980f6
to
9d4d586
Compare
There was a problem hiding this 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 26 files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware and @noaov1)
.cargo/config.toml
line 1 at r6 (raw file):
[env]
The expected behaviour for MacOS users would be different. Perhaps leaving comments how to config this correctly for other systems.
scripts/setup_native_deps.sh
line 36 at r6 (raw file):
TABLEGEN_180_PREFIX="$MLIR_SYS_180_PREFIX" export LIBRARY_PATH
I find myself unable to select multiple lines, I think this variables declarations are redundant (defined on top and exported below this comment)
There was a problem hiding this 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 26 files reviewed, 14 unresolved discussions (waiting on @alon-dotan-starkware and @rodrigo-pino)
Dockerfile
line 1 at r6 (raw file):
# Dockerfile with multi-stage builds for efficient dependency caching and lightweight final image.
Dockerfile
line 62 at r6 (raw file):
########################### # Uses Alpine Linux to run a lightweight and secure container. FROM alpine:3.17.0 AS papyrus_node
Is this change okay? @dan-starkware
Code quote:
# Uses Alpine Linux to run a lightweight and secure container.
FROM alpine:3.17.0 AS papyrus_node
Dockerfile
line 73 at r6 (raw file):
RUN set -ex; \ apk update; \ apk add --no-cache tini; \
was this flag here for a reason? @dan-starkware
Code quote:
--no-cache
.github/workflows/blockifier_ci.yml
line 50 at r6 (raw file):
native-blockifier-artifacts-push: runs-on: starkware-ubuntu-latest-large
This machine change needs to be python-side tested (I'll do it).
@dorimedini-starkware don't expect this to work, and if it does, @alon-dotan-starkware please advise why...? He though we needed to build the SO on ubuntu 20.04
Code quote:
runs-on: starkware-ubuntu-latest-large
.github/workflows/committer_ci.yml
line 120 at r6 (raw file):
repo: context.repo.repo, body: fs.readFileSync('bench_new.txt', 'utf8'), path: 'Commits'
Code quote:
path: 'Commits'
.github/workflows/main.yml
line 98 at r6 (raw file):
run-tests: runs-on: starkware-ubuntu-latest-large
why ubuntu 22?
Code quote:
runs-on: starkware-ubuntu-latest-large
.github/workflows/papyrus_benchmark.yaml
line 7 at r6 (raw file):
# TODO: Uncomment and run this automatically when the storage benchmark is fixed. # push: # branches: [main]
indentation would matter here
Suggestion:
# branches: [main]
.github/workflows/papyrus_ci.yml
line 5 at r6 (raw file):
on: push: branches: [ main ]
Why was the extra whitespace added?
(same below)
Code quote:
branches: [ main ]
.github/workflows/papyrus_docker-publish.yml
line 7 at r6 (raw file):
push: branches: [ main ] tags: [ "v*.*.*" ]
Why was the extra whitespace added?
Code quote:
branches: [ main ]
tags: [ "v*.*.*" ]
.github/workflows/papyrus_docker-publish.yml
line 28 at r6 (raw file):
jobs: docker-build-push: runs-on: starkware-ubuntu-latest-large
Why is this change related to this PR?
Code quote:
runs-on: starkware-ubuntu-latest-large
.github/workflows/papyrus_nightly-tests.yml
line 45 at r6 (raw file):
- run: brew install protobuf@$PROTOC_VERSION
Is the new kine needed?
crates/blockifier/src/execution/contract_class.rs
line 15 at r6 (raw file):
use cairo_lang_utils::bigint::BigUintAsHex; #[allow(unused_imports)] use cairo_native::executor::AotNativeExecutor;
Why is this needed?
@varex83
Code quote:
#[allow(unused_imports)]
use cairo_native::executor::AotNativeExecutor;
There was a problem hiding this 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 26 files reviewed, 15 unresolved discussions (waiting on @alon-dotan-starkware and @rodrigo-pino)
scripts/setup_native_deps.sh
line 60 at r6 (raw file):
function main() { [ "$(uname)" = "Linux" ] && install_essential_deps_linux setup_llvm_deps
Need to add:
sudo apt-get install libzstd-dev
There was a problem hiding this 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 26 files reviewed, 17 unresolved discussions (waiting on @alon-dotan-starkware and @rodrigo-pino)
Cargo.toml
line 91 at r6 (raw file):
cached = "0.44.0" cairo-felt = "0.9.1" cairo-lang-casm = "2.8.2"
I updated the compiler version in a separate PR. Please rebase.
Code quote:
cairo-lang-casm = "2.8.2"
scripts/setup_native_deps.sh
line 90 at r1 (raw file):
} install_cairo_native_runtime() {
What did that method do and why isn't it needed anymore?
Code quote:
install_cairo_native_runtime() {
There was a problem hiding this 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 26 files reviewed, 17 unresolved discussions (waiting on @alon-dotan-starkware, @noaov1, and @varex83)
crates/blockifier/src/execution/contract_class.rs
line 15 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is this needed?
@varex83
It is not, we can remove
Previously, noaov1 (Noa Oved) wrote…This is just where to post the comment. AFAIK, this line doesn't work, so it doesn't matter, but if it does work, it's better to include it. |
There was a problem hiding this 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 26 files reviewed, 17 unresolved discussions (waiting on @aner-starkware, @dan-starkware, @noaov1, @rodrigo-pino, and @varex83)
Dockerfile
line 1 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Done.
Dockerfile
line 62 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Is this change okay? @dan-starkware
Done.
Dockerfile
line 73 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
was this flag here for a reason? @dan-starkware
Done.
.github/workflows/blockifier_ci.yml
line 50 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
This machine change needs to be python-side tested (I'll do it).
@dorimedini-starkware don't expect this to work, and if it does, @alon-dotan-starkware please advise why...? He though we needed to build the SO on ubuntu 20.04
its only the push... the build itself executed in the docker itself which is ubuntu 20.04
.github/workflows/committer_ci.yml
line 120 at r6 (raw file):
Previously, aner-starkware wrote…
This is just where to post the comment. AFAIK, this line doesn't work, so it doesn't matter, but if it does work, it's better to include it.
Done.
.github/workflows/main.yml
line 98 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
why ubuntu 22?
it makes no difference
.github/workflows/papyrus_benchmark.yaml
line 7 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
indentation would matter here
nope... its comment, when uncomment the indentation need to be fixed
There was a problem hiding this 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 26 files reviewed, 17 unresolved discussions (waiting on @aner-starkware, @dan-starkware, @noaov1, @rodrigo-pino, and @varex83)
.github/workflows/papyrus_ci.yml
line 5 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why was the extra whitespace added?
(same below)
code styling
.github/workflows/papyrus_docker-publish.yml
line 28 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is this change related to this PR?
its needed for tests to pass....
need more disk space
.github/workflows/papyrus_nightly-tests.yml
line 45 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Is the new kine needed?
were?
crates/blockifier/src/execution/contract_class.rs
line 15 at r6 (raw file):
Previously, rodrigo-pino (Rodrigo) wrote…
It is not, we can remove
Done.
scripts/setup_native_deps.sh
line 90 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
What did that method do and why isn't it needed anymore?
dont really know I consulted with @rodrigo-pino and he mentioned its not needed
scripts/setup_native_deps.sh
line 36 at r6 (raw file):
Previously, rodrigo-pino (Rodrigo) wrote…
I find myself unable to select multiple lines, I think this variables declarations are redundant (defined on top and exported below this comment)
Done.
scripts/setup_native_deps.sh
line 60 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Need to add:
sudo apt-get install libzstd-dev
Done.
071e392
to
d089336
Compare
There was a problem hiding this 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 26 files reviewed, 17 unresolved discussions (waiting on @alon-dotan-starkware, @aner-starkware, @dan-starkware, @noaov1, @rodrigo-pino, and @varex83)
.github/workflows/papyrus_benchmark.yaml
line 7 at r6 (raw file):
Previously, alon-dotan-starkware wrote…
nope... its comment, when uncomment the indentation need to be fixed
yes, so indent now, so when it's uncommented no thought will be required.
GH is not good at notifying devs that an action is malformed - the action is simply skipped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 23 files at r1, 6 of 10 files at r4, 1 of 6 files at r5, 3 of 4 files at r6, 2 of 7 files at r7, 1 of 2 files at r8, all commit messages.
Reviewable status: 18 of 26 files reviewed, 7 unresolved discussions (waiting on @alon-dotan-starkware, @aner-starkware, @dan-starkware, @rodrigo-pino, and @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 18 of 26 files reviewed, 8 unresolved discussions (waiting on @alon-dotan-starkware, @aner-starkware, @dan-starkware, @rodrigo-pino, and @varex83)
Dockerfile
line 36 at r8 (raw file):
# # This enables the creation of fully self-contained binaries that do not depend on the system's dynamic libraries, # # resulting in more portable executables that can run on any Linux distribution. # RUN rustup target add x86_64-unknown-linux-musl
Suggestion:
# Reinstalling the stable Rust toolchain to ensure a clean environment
# RUN rustup toolchain uninstall stable-x86_64-unknown-linux-gnu && rustup toolchain install stable-x86_64-unknown-linux-gnu
# Add the x86_64-unknown-linux-musl target to rustup for compiling statically linked binaries.
# This enables the creation of fully self-contained binaries that do not depend on the system's dynamic libraries,
# resulting in more portable executables that can run on any Linux distribution.
# RUN rustup target add x86_64-unknown-linux-musl
d089336
to
b7ccd92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 18 of 26 files reviewed, 8 unresolved discussions (waiting on @alon-dotan-starkware, @aner-starkware, @dan-starkware, @noaov1, and @varex83)
scripts/setup_native_deps.sh
line 90 at r1 (raw file):
Previously, alon-dotan-starkware wrote…
dont really know I consulted with @rodrigo-pino and he mentioned its not needed
It's needed for native runtime, but it is not needed now. Better to add it when actually using it.
There was a problem hiding this 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: 18 of 26 files reviewed, 7 unresolved discussions (waiting on @alon-dotan-starkware, @aner-starkware, @dan-starkware, @rodrigo-pino, and @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 7 files at r7, 1 of 1 files at r9.
Reviewable status: 24 of 26 files reviewed, 6 unresolved discussions (waiting on @alon-dotan-starkware, @aner-starkware, @dan-starkware, @rodrigo-pino, and @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 23 files at r1, 6 of 10 files at r4, 1 of 6 files at r5, 3 of 4 files at r6, 2 of 7 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: 24 of 26 files reviewed, 6 unresolved discussions (waiting on @alon-dotan-starkware, @aner-starkware, @dan-starkware, and @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 24 of 26 files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware, @aner-starkware, @dan-starkware, @noaov1, and @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 10 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alon-dotan-starkware)
There was a problem hiding this 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alon-dotan-starkware)
This change is