-
Notifications
You must be signed in to change notification settings - Fork 773
make Rust a build dependency for maturin 1.1.0 (2023a generation) #22983
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
Merged
Micket
merged 11 commits into
easybuilders:develop
from
boegel:20250531113359_new_pr_maturin110
Jun 11, 2025
Merged
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
cc78ff6
make Rust a build dependency for maturin 1.1.0 (2023a generation)
boegel 1d968e9
use CargoPythonBundle or CargoPythonPackage easyblock when Rust is a …
boegel 70242e7
use CargoPythonBundle easyblock for jupyter-server 2.7.2 + add list o…
boegel 600597f
add list of crates to easyconfig for torch-em 0.7.1
boegel 20fd2f8
add list of crates to easyconfig for vLLM 0.4.0
boegel 2d44fc3
add additional crates required by torch-em 0.7.1 extensions
boegel 90b6618
fix order of checksums in torcm-em easyconfig
boegel ed6e8f8
remove maturin + Rust build dependencies for nbclassic & pymatgen (no…
boegel 293e133
add back maturin as build dependency for pymatgen
boegel 2e4f299
add Rust build dependency + use CargoPythonBundle for pymatgen v2023.…
boegel f21d6b6
add comment to clarify that orjson 3.9.10 vendors the Rust crates it …
boegel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
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.
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.
orjson is a rust package
I.. really don't understand how this could pass a build without this. We should be setting cargo to offline mode, but maybe the maturin/pip wrapper thing overrides those environment variables, allowing it to go online?
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.
Yeah, I was also a bit puzzled about how this could work without providing a list of crates...
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 guess this explains it?
In
pymatgen-2024.5.1-foss-2023b.eb,orjsonhas been lifted in a proper dependency, so situation is different there.Uh oh!
There was an error while loading. Please reload this page.
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.
@Micket
CargoPythonBundleshould make sure thatCargoPythonPackageis used for all extensions, soCargo.extract_step(which ends up setting$CARGO_NET_OFFLINEtotrueis used for every extension?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.
Hmm, no, should be fine, the log tells me that
$CARGO_NET_OFFLINEis being set totrueright before the installation of every extension, includingorjsonThere 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.
Well, it's being set, but then later being unset again, before the actual
pip installhappens: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 think the problem is that
$CARGO_NET_OFFLINEis not being set byCargo.set_cargo_vars, which results in it not being set anymore when the actual installation of extensions is done.That's probably an unexpected side effect from the changes implemented in easybuilders/easybuild-framework#4868
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.
For the specific version of
orjsonbeing used here (3.9.10), it seems likecargodoesn't need to download anything at all, because the sources of the requires crates are included in the source tarball ororjson:So although easybuilders/easybuild-easyblocks#3764 is a correct fix, we don't actually need to provide a list of
cratesat all for thispymatgeneasyconfig, since all crates required by theorjsonextension included in it are shipped/vendored with 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.
Lets make a comment that explains that this specific older version of orjson used vendored crates.
Updating to newer versions will require adding the crates.
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.
@Micket OK, makes sense, done in f21d6b6