Add enzyme distribution step #140244
Closed
Add enzyme distribution step #140244
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
r? @Kobzol |
Kobzol
reviewed
Apr 25, 2025
Kobzol
reviewed
Apr 25, 2025
Shourya742
force-pushed
the
2025-04-24-enzyme-distribution
branch
2 times, most recently
from
1624d86 to
7a9bc08
Compare
Kobzol
reviewed
Apr 28, 2025
| let enzyme = builder.ensure(super::llvm::Enzyme { target: self.target }); | ||
| tarball.set_overlay(OverlayKind::Enzyme); | ||
| tarball.is_preview(true); | ||
| if let Some(llvm_config) = builder.llvm_config(builder.config.build) { |
There was a problem hiding this comment.
Hmm, this is a weird. When I have download-ci-llvm = true enabled, this LLVM config is the CI LLVM (which is version 20). But when I actually build Enzyme locally, it generates libEnzyme-18. My host system LLVM is LLVM 18, so that looks related? I don't know why is the filename containing the version of the host compiler rather than the version of the LLVM source code though.
@ZuseZ4 Does Enzyme have its own versioning separate from LLVM? I found some version 0.0.79 in the source code.
There was a problem hiding this comment.
rg "0\.0\.79" doesn't return anything in Enzyme, where did you find it?
Enzyme has releases but doesn't follow semver (in a meaningful way only uses patch versions), so I conveniently ignore it.
https://github.com/EnzymeAD/Enzyme/releases
We also just use Enzyme's LLVM Pass which allows us to ignore most of the API, so breaking changes barely ever affect us.
They are more relevant for Julia which has a different build infra working on gh releases. We just use the source code from rust-lang/Enzyme.
There was a problem hiding this comment.
Just for clarification of
when I actually build Enzyme locally
I currently only try (and have it working):
A) building LLVM locally, then building Enzyme based on it.
Here we try:
B) building LLVM remote (in CI), then building Enzyme also remote based on it.
I guess you tried
C) building LLVM remote, then downloading it and building Enzyme locally against it?
I thought based on Zulip that you wanted a different solution for C, right?
There was a problem hiding this comment.
Semver is followed for user-level enzyme_autodiff and related functions. All user-level changes have been backwards compatible, so we've never bumped. Internal APIs (like LLVM) have no guarantees of stability
There was a problem hiding this comment.
I saw the version here: https://github.com/rust-lang/enzyme/blob/main/enzyme/CMakeLists.txt#L10
To clarify, I was indeed using option C), but almost inadvertedly, as download-ci-llvm is enabled in a lot of default configs and many people use it, it would be nice if bootstrap either "just worked" with it, or at least if it produced a loud error.
I don't really understand how this works though. The build step for Enzyme seems to pass LLVM_CONFIG_REAL to the llvm-config of the LLVM downloaded from CI. I would thus expect that Enzyme either attaches to that, or that it is somehow linked to the in-tree version of LLVM. But instead it seems to produce a version number tied to the host LLVM used to build Enzyme itself.
How exactly is Enzyme "tied" to a specific LLVM sysroot/source code/build/whatever?
There was a problem hiding this comment.
Enzyme works with the LLVM it was build against, everything else could only work by coincidence. E.g. this is one of the cmake commands I use locally:
cmake .. -G Ninja -DLLVM_DIR=/home/manuel/prog/rust-middle/build/x86_64-unknown-linux-gnu/llvm/build/lib/cmake/llvm -DLLVM_EXTERNAL_LIT=/home/manuel/prog/rust-middle/src/llvm-project/llvm/utils/lit/lit.py -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=YES -DBUILD_SHARED_LIBS=On
We should build Enzyme wherever we also build LLVM. So after building LLVM, we would build Enzyme and set LLVM_DIR to the path of the newly build llvm. We should not point to the LLVM/Clang that was used to build Enzyme and LLVM.
So we should find a way to point Enzyme to a specific LLVM checkout/build.
I mean that's why I added the -DLLVM_DIR definition in rust/src/bootstrap/src/core/build_steps/llvm.rs, it just looks like we don't set it correctly in CI right now? So I guess builder.llvm_out(target) is wrong then (?)
As another puzzle piece, these folders from an apple CI run seem correct? We build using LLVM/Clang-15, but the self-build LLVM: https://github.com/rust-lang-ci/rust/actions/runs/14582626710/job/40902344579
There was a problem hiding this comment.
Oh, I see now, I didn't realize that LLVM_DIR is the thing that links Enzyme to LLVM, I thought it's the output directory where the Enzyme artifacts will be generated.
In that case I suppose that without download-ci-llvm it should mostly do what we want. I think that the path we give to LLVM_DIR is not exactly what it should be, as it's essentially just build/<target>/llvm; it looks like Enzyme's CMake expects it to be something a bit different (e.g. the path you use, build/<target>/llvm/build/lib/cmake/llvm. That being said, the Enzyme CMake seems to use recursive greps for finding the files that it needs, so it looks like it is able to resolve what it requires.
Ok, in that case I'm fine with just adding an error into the llvm::Enzyme step that it currently cannot be combined with download-ci-llvm.
There was a problem hiding this comment.
Oh, in case that the code isn't clear, @Shourya742 can you please add a comment around line 973, saying that we build enzyme against this specific LLVM (and using it with a different LLVM isn't supported)?
There was a problem hiding this comment.
Also @Kobzol now that I already have you here, do you mind looking at the apple issue here? :D https://github.com/rust-lang-ci/rust/actions/runs/14582626710/job/40902344579#step:28:28938 and checking if the llvm path looks right to you? I feel like it should be ok, 15 is used for building everything, 20 is the one we build against. But it still fails to link LLVM. Presumably Enzyme's cmake should not just link against llvm (-lLLVM), but also specify -L based on the given LLVM_DIR?
There was a problem hiding this comment.
I'm not exactly sure how linking works on Apple, but just passing -lLLVM is unlikely to work, IMO. The directory where the libLLVM.dylib (or static archive) files can be found will likely be needed.
|
I would create a separate PR for that, it seems unrelated to #140064. |
|
Yeah I just asked since I had a soft deadline to present autodiff on nightly on April 30 (hence my question), but I won't make it anymore anyway so now I don't mind landing them separately. |
Shourya742
force-pushed
the
2025-04-24-enzyme-distribution
branch
from
7a9bc08 to
ae947f6
Compare
|
@rustbot review |
Kobzol
reviewed
Apr 30, 2025
It did.. let me check again.. |
For me, libEnzyme-20.so is present in the enzyme dist |
This kind of behavior can only occur if the step is being run in dry-run mode. Could we try disabling dry-run mode and see if the issue persists? |
Shourya742
force-pushed
the
2025-04-24-enzyme-distribution
branch
from
ae947f6 to
0d488fa
Compare
|
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. |
|
It's not dry run, the issue is that my host LLVM is 18, so the code doesn't find I would still appreciate one more safeguard now that I saw this though. If the libEnzyme.so file isn't found in the |
Ah!!! makes sense... let me add a hard error... |
Shourya742
force-pushed
the
2025-04-24-enzyme-distribution
branch
from
0d488fa to
0ac71b6
Compare
|
For a change, it now fails for me locally with this wonderful known error 😆 Probably we should wait until the Enzyme adds the |
|
@Kobzol Ooh. You managed to trigger this locally? I spend a lot of hrs and days trying to fix it in CI (but failed) because iteration speed is extremely slow and I can't see what files exist in CI. A local reproducer would be awesome |
|
Fwiw I've given up on trying to fix that bug in CI since I still haven't made relevant progress, so unless we have a reliable local reproducer I'd suggest to just merge it as-is, even if we don't support all cases yet. |
|
@bors try |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 2, 2025
…ution, r=<try> Add enzyme distribution step This PR adds enzyme to bootstrap dist. r? `@ghost` try-job: dist-x86_64-linux
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
|
There is one warning about an unrecognized This seems related since it's a dist runner: https://stackoverflow.com/a/63832669 |
It might be caused by this: @wsmoses Do you know what was the motivation for making paths absolute? |
|
@Shourya742 Can you go to |
|
@Kobzol I got dist-apple CI to actually work for the first time, it managed to compile and link Enzyme: https://github.com/rust-lang-ci/rust/actions/runs/14826601681/job/41620576833 However, it does fully compile locally, so your error from above should be gone. Can you try again if you have a bit of time? |
|
Some changes occurred in src/tools/enzyme cc @ZuseZ4 |
|
|
@bors try |
|
@Shourya742: 🔑 Insufficient privileges: not in try users |
|
@bors delegate+ (For try builds :) ) |
|
✌️ @Shourya742, you can now approve this pull request! If @Kobzol told you to " |
|
@bors try |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 6, 2025
…ution, r=<try> Add enzyme distribution step This PR adds enzyme to bootstrap dist. r? `@ghost` try-job: dist-x86_64-linux
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
@Kobzol could we be running out of memory? Otherwise, I think this should work?
|
|
Hmm, hard to tell, I would rather expect some LLVM version mismatch or some weird interaction with how Enzyme does things with its CMake with our build system. It's possible to run the CI job locally, I'll try to debug it. |
|
I can reproduce the failure locally with the CI job. I suspect that we might have to configure the Enzyme CMake invocation in the same way we configure the LLVM CMake invocation, in particular we might have to use the same If someone wants to take a look, you can reproduce it on Linux with |
|
Based on the discussion on Zulip #t-infra > MCP - Enable autodiff in CI 💬. This PR now stands redundant. Thank you, @Kobzol, for the review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds enzyme to bootstrap dist.
r? @ghost
try-job: dist-x86_64-linux