-
Notifications
You must be signed in to change notification settings - Fork 140
ci: add a github action ci to test Rust std::autodiff #2430
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
base: main
Are you sure you want to change the base?
Conversation
run: | | ||
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y | ||
source ~/.cargo/env | ||
rustup toolchain install nightly |
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.
until Rust adds CI for Enzyme, I think this should be fixed to a specific commit
.github/workflows/enzyme-rust.yml
Outdated
jobs: | ||
rust-autodiff: | ||
name: Rust Autodiff Tests LLVM ${{ matrix.llvm }} | ||
runs-on: [self-hosted, Linux, X64, 32-core] |
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.
this is not the correct setup for the runner, see https://github.com/EnzymeAD/Enzyme/blob/main/.github/workflows/enzyme-mlir.yml for an example
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.
it needs to run on linux-x86-n2-32
cmake ../enzyme -DLLVM_EXTERNAL_LIT=`which lit` -DCMAKE_BUILD_TYPE=Release -DLLVM_DIR=/usr/lib/llvm-${{ matrix.llvm }}/lib/cmake/llvm | ||
make -j `nproc` LLVMEnzyme-${{ matrix.llvm }} | ||
- name: Clone and configure Rust compiler | ||
run: | |
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.
this is never told to use the enzyme we built above?
.github/workflows/enzyme-rust.yml
Outdated
source ~/.cargo/env | ||
git clone --depth 1 --branch master https://github.com/rust-lang/rust.git | ||
cd rust | ||
./configure --enable-llvm-link-shared --enable-llvm-plugins --enable-llvm-enzyme --release-channel=nightly --enable-llvm-assertions --enable-clang --enable-lld --enable-option-checking --enable-ninja --disable-docs |
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.
we should also use ccache here, and cache intermediate artifacts
@wsmoses @ZuseZ4 #2430 (comment) Could you give me some hints, examples, suggestions, or something I should study? |
I have no experience with ccache and to me it sounds like something that should be added in a follow-up pr to avoid feature creep. But it looks like you've already added it in your last commit? If it works I guess it should stay. For the pinning of the version, I would assume we want to fix the rustc we build, and not necessarily the rust version we use to build rustc. It's fine to do both, but right now you only pin the version used to build. You should also check out a specific commit of github.com/rust-lang/rust instead of just the master branch. This way, we can verify that new Enzyme versions don't break support. We would update the rustc commit every once in a while. This way, an update to rustc can't break Enzyme CI. I don't know whats the relevant difference between your. yml file and the mlir example given, so I'd leave that for Billy to answer. |
.github/workflows/enzyme-rust.yml
Outdated
echo "CC=ccache gcc" >> $GITHUB_ENV | ||
echo "CXX=ccache g++" >> $GITHUB_ENV |
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.
Who uses this? What exactly is built using GCC? I think Rust still uses LLVM. The rust invocation will prefix its invocations with ccache
by itself as far as I can see, thanks to the --enable-ccache
a few lines above.
.github/workflows/enzyme-rust.yml
Outdated
run: | | ||
. ~/.cargo/env | ||
rm -rf build && mkdir build && cd build | ||
RUST_LLVM_DIR=${GITHUB_WORKSPACE}/rust/build/x86_64-unknown-linux-gnu/llvm |
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.
I would shell-quote this. GitHub probably has no space or special chars in the checkout dir, but better be safe.
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.
I shell-quoted it in this commit!
use quotes
.github/workflows/enzyme-rust.yml
Outdated
run: | | ||
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y | ||
. ~/.cargo/env | ||
# Pin to specific nightly commit for reproducibility |
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.
What is this used for? Rustc bootstrap? Might be worth it to add a comment saying what is now reproducible. Enzyme itself is built against the latest rust compiler (you check out the main branch of rust-lang/rust), so I imagine it won't really help there?
959f1ee
to
f6c53f9
Compare
.github/workflows/enzyme-rust.yml
Outdated
|
||
container: | ||
image: ${{ (contains(matrix.os, 'linux') && 'ghcr.io/enzymead/reactant-docker-images@sha256:91e1edb7a7c869d5a70db06e417f22907be0e67ca86641d48adcea221fedc674' ) || '' }} | ||
image: ${{ (contains(matrix.os, 'linux') && 'ghcr.io/enzymead/reactant-docker-images:main' ) || '' }} |
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.
this should use the specific hash, not main
.github/workflows/enzyme-rust.yml
Outdated
- name: Install dependencies | ||
run: | | ||
apt-get update | ||
apt-get install -y binutils ninja-build cmake gcc g++ python3 python3-dev ccache nodejs npm |
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.
do you need all these deps?
.github/workflows/enzyme-rust.yml
Outdated
. ~/.cargo/env | ||
git clone https://github.com/rust-lang/rust.git | ||
cd rust | ||
git checkout 51ff895062ba60a7cba53f57af928c3fb7b0f2f4 |
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.
can you move the commit to the matrix? that'll make it easier to update, and or if in the future we want to checkout multiple and/or stable tags
ec70a9d
to
2071224
Compare
This currently builds Enzyme twice, once as part of rustc and once standalone. |
.github/workflows/enzyme-rust.yml
Outdated
- name: Clone and configure Rust compiler | ||
run: | | ||
. ~/.cargo/env | ||
git clone https://github.com/rust-lang/rust.git |
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.
you should use another actions/checkout for this [and can give it the relevant tag/commit]
@wsmoses this looks ready? Can you run CI, I don't have permission |
eadd4f3
to
9e3b7aa
Compare
--enable-clang \ | ||
--enable-lld \ | ||
--disable-optimize-llvm \ | ||
--enable-option-checking \ |
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.
we should use the github actions cache here, but lets see if it builds first
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.
otherwise ccache doesn't do anything as the run is ephemeral
fails
|
@sgasho Not sure if it's related, or needed but you can also try to do a submodule update for enzyme and commit it. |
|
726c421
to
79282f3
Compare
@sgasho Looks like you managed it! The last test is failing, but that one frequently changes the IR generated, not sure if it's because of rustc, LLVM, or Enzyme changing. I'll look into making it more robust, it's not like that feature (batching) is well tested in Rust atm anyway. From my side that sounds like it's good to merge? Also side note, we'll probably switch from linking against Enzyme to just dlopen'ing Enzyme, which should make this CI even simpler in the future: rust-lang/rust#146623 |
…youxu bless autodiff batching test This pr blesses a broken test and unblocks running rust in the Enzyme CI: EnzymeAD/Enzyme#2430 Enzyme is the plugin used by our std::autodiff and (future) std::batching modules, both of which are not build by default. In the near future we also hope to enable std::autodiff in the Rust CI. This test is the only one to combine two features, automatic differentiation and batching/vectorization. This combination is even more experimental than either feature on its own. I have a wip branch in which I enable more vectorization/batching and as part of that I'll think more about how to write those tests in a robust way (and likely change the interface). Until that lands, I don't care too much about what specific IR we generate here; it's just nice to track changes. r? compiler
…youxu bless autodiff batching test This pr blesses a broken test and unblocks running rust in the Enzyme CI: EnzymeAD/Enzyme#2430 Enzyme is the plugin used by our std::autodiff and (future) std::batching modules, both of which are not build by default. In the near future we also hope to enable std::autodiff in the Rust CI. This test is the only one to combine two features, automatic differentiation and batching/vectorization. This combination is even more experimental than either feature on its own. I have a wip branch in which I enable more vectorization/batching and as part of that I'll think more about how to write those tests in a robust way (and likely change the interface). Until that lands, I don't care too much about what specific IR we generate here; it's just nice to track changes. r? compiler
…youxu bless autodiff batching test This pr blesses a broken test and unblocks running rust in the Enzyme CI: EnzymeAD/Enzyme#2430 Enzyme is the plugin used by our std::autodiff and (future) std::batching modules, both of which are not build by default. In the near future we also hope to enable std::autodiff in the Rust CI. This test is the only one to combine two features, automatic differentiation and batching/vectorization. This combination is even more experimental than either feature on its own. I have a wip branch in which I enable more vectorization/batching and as part of that I'll think more about how to write those tests in a robust way (and likely change the interface). Until that lands, I don't care too much about what specific IR we generate here; it's just nice to track changes. r? compiler
6c69259
to
708beb1
Compare
…youxu bless autodiff batching test This pr blesses a broken test and unblocks running rust in the Enzyme CI: EnzymeAD/Enzyme#2430 Enzyme is the plugin used by our std::autodiff and (future) std::batching modules, both of which are not build by default. In the near future we also hope to enable std::autodiff in the Rust CI. This test is the only one to combine two features, automatic differentiation and batching/vectorization. This combination is even more experimental than either feature on its own. I have a wip branch in which I enable more vectorization/batching and as part of that I'll think more about how to write those tests in a robust way (and likely change the interface). Until that lands, I don't care too much about what specific IR we generate here; it's just nice to track changes. r? compiler
.github/workflows/enzyme-rust.yml
Outdated
uses: actions/cache@v4 | ||
with: | ||
path: ~/.cache/ccache | ||
key: ccache-${{ matrix.os }}-${{ matrix.commit }} |
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.
maybe also include rust here?
Rollup merge of #147315 - ZuseZ4:fix-ad-batching-test, r=jieyouxu bless autodiff batching test This pr blesses a broken test and unblocks running rust in the Enzyme CI: EnzymeAD/Enzyme#2430 Enzyme is the plugin used by our std::autodiff and (future) std::batching modules, both of which are not build by default. In the near future we also hope to enable std::autodiff in the Rust CI. This test is the only one to combine two features, automatic differentiation and batching/vectorization. This combination is even more experimental than either feature on its own. I have a wip branch in which I enable more vectorization/batching and as part of that I'll think more about how to write those tests in a robust way (and likely change the interface). Until that lands, I don't care too much about what specific IR we generate here; it's just nice to track changes. r? compiler
I blessed the last Rust test, so now rust CI should pass if rerun. |
Or rather if the commit is updated, presumably. The test shouldn't pull main but a fixed commit |
That's unlucky, another update just changed a function hash in a test case. I added another fix on the Rust side to make the test more robust, it should land in a few hrs: rust-lang/rust#147364 (comment) |
…eyouxu update autodiff testcases unblock EnzymeAD/Enzyme#2430 (again). Just as I landed a fix for the last test, this one broke. The test should now be fine if the name mangling hash changes again. Also removed an outdated fixme that's not needed since moving autodiff to an intrinsic. The test currently just checks that it compiles, I'll add more precise checks once we actually run this in CI. r? compiler
…eyouxu update autodiff testcases unblock EnzymeAD/Enzyme#2430 (again). Just as I landed a fix for the last test, this one broke. The test should now be fine if the name mangling hash changes again. Also removed an outdated fixme that's not needed since moving autodiff to an intrinsic. The test currently just checks that it compiles, I'll add more precise checks once we actually run this in CI. r? compiler
Rollup merge of #147364 - ZuseZ4:update-autodiff-tests, r=jieyouxu update autodiff testcases unblock EnzymeAD/Enzyme#2430 (again). Just as I landed a fix for the last test, this one broke. The test should now be fine if the name mangling hash changes again. Also removed an outdated fixme that's not needed since moving autodiff to an intrinsic. The test currently just checks that it compiles, I'll add more precise checks once we actually run this in CI. r? compiler
@sgasho the fix landed, so if you update the rust commit it should pass now :) |
closes: #2429
related: rust-lang/rust#145899
result of act dry-run