-
Notifications
You must be signed in to change notification settings - Fork 772
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
make Rust a build dependency for maturin 1.1.0 (2023a generation) #22983
Conversation
|
@boegelbot please test @ jsc-zen3 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2924804453 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
|
@boegelbot please test @ jsc-zen3 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2925164957 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegel |
|
Test report by @boegelbot |
|
@boegelbot please test @ jsc-zen3-a100 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2925237278 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
|
@boegelbot please test @ jsc-zen3-a100 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2925693147 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
|
Test report by @boegel |
…t required at all) + use PythonPackage easyblock
|
@boegelbot please test @ jsc-zen3-a100 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2929869510 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
|
Taking a look why edit: error is pretty clear, when installing So Same problem locally (I was using edit: pymatgen only requires |
|
@boegelbot please test @ jsc-zen3-a100 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2934090712 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
|
@boegelbot please test @ jsc-zen3-a100 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2949903786 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
| @@ -1,4 +1,4 @@ | |||
| easyblock = 'PythonBundle' | |||
| easyblock = 'CargoPythonBundle' | |||
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
crates = [
('ahash', '0.8.6'),
('arrayvec', '0.7.4'),
('associative-cache', '2.0.0'),
('autocfg', '1.1.0'),
('beef', '0.5.2'),
('bytecount', '0.6.7'),
('castaway', '0.2.2'),
('cc', '1.0.83'),
('cfg-if', '1.0.0'),
('chrono', '0.4.31'),
('compact_str', '0.7.1'),
('encoding_rs', '0.8.33'),
('itoa', '1.0.9'),
('itoap', '1.0.1'),
('libc', '0.2.149'),
('libm', '0.2.8'),
('no-panic', '0.1.26'),
('num-traits', '0.2.17'),
('once_cell', '1.18.0'),
('packed_simd', '0.3.9'),
('proc-macro2', '1.0.69'),
('pyo3-build-config', '0.20.0'),
('pyo3-ffi', '0.20.0'),
('quote', '1.0.33'),
('rustversion', '1.0.14'),
('ryu', '1.0.15'),
('serde', '1.0.190'),
('serde_derive', '1.0.190'),
('serde_json', '1.0.107'),
('simdutf8', '0.1.4'),
('smallvec', '1.11.1'),
('static_assertions', '1.1.0'),
('syn', '2.0.38'),
('target-lexicon', '0.12.12'),
('unicode-ident', '1.0.12'),
('version_check', '0.9.4'),
('zerocopy', '0.7.15'),
('zerocopy-derive', '0.7.15'),
]
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?
== 2025-05-31 23:13:07,252 easyblock.py:3091 DEBUG Installing extension orjson with default class PythonPackage (from easybuild.easyblocks.generic.pythonpackage)
In pymatgen-2024.5.1-foss-2023b.eb, orjson has been lifted in a proper dependency, so situation is different there.
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 CargoPythonBundle should make sure that CargoPythonPackage is used for all extensions, so Cargo.extract_step (which ends up setting $CARGO_NET_OFFLINE to true is 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_OFFLINE is being set to true right before the installation of every extension, including orjson
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.
Well, it's being set, but then later being unset again, before the actual pip install happens:
egrep 'installing.*extension|CARGO_NET_OFFLINE|Running shell command.*pip install.*orjson' /software/pymatgen/2023.12.18-foss-2023a/easybuild/*log*
...
== 2025-05-31 23:15:56,812 environment.py:93 INFO Environment variable CARGO_NET_OFFLINE set to true (previously undefined)
== 2025-05-31 23:15:56,852 build_log.py:322 INFO installing extension orjson 3.9.10 (13/25)...
== 2025-05-31 23:15:57,920 environment.py:172 DEBUG Key in old environment found that is not in new one: CARGO_NET_OFFLINE (true)
== 2025-05-31 23:16:05,368 run.py:500 INFO Running shell command '/user/gent/400/vsc40023/eb_arcaninescratch/RHEL9/zen2-ib/software/Python/3.11.3-GCCcore-12.3.0/bin/python -m pip install --prefix=/user/gent/400/vsc40023/eb_arcaninescratch/RHEL9/zen2-ib/software/pymatgen/2023.12.18-foss-2023a --verbose --no-deps --ignore-installed --no-build-isolation .' in /tmp/vsc40023/easybuild_build/pymatgen/2023.12.18/foss-2023a/orjson/orjson-3.9.10
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_OFFLINE is not being set by Cargo.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 orjson being used here (3.9.10), it seems like cargo doesn't need to download anything at all, because the sources of the requires crates are included in the source tarball or orjson:
eb-shell> pwd
/tmp/vsc40023/easybuild_build/pymatgen/2023.12.18/foss-2023a/orjson/orjson-3.9.10
eb-shell> ls include/cargo/
ahash-0.8.6 bytecount-0.6.7 compact_str-0.7.1 libm-0.2.8 proc-macro2-1.0.69 ryu-1.0.15 smallvec-1.11.1 version_check-0.9.4
arrayvec-0.7.4 castaway-0.2.2 encoding_rs-0.8.33 no-panic-0.1.26 pyo3-build-config-0.20.0 serde-1.0.190 static_assertions-1.1.0 zerocopy-0.7.15
associative-cache-2.0.0 cc-1.0.83 itoa-1.0.9 num-traits-0.2.17 pyo3-ffi-0.20.0 serde_derive-1.0.190 syn-2.0.38 zerocopy-derive-0.7.15
autocfg-1.1.0 cfg-if-1.0.0 itoap-1.0.1 once_cell-1.18.0 quote-1.0.33 serde_json-1.0.107 target-lexicon-0.12.12
beef-0.5.2 chrono-0.4.31 libc-0.2.149 packed_simd-0.3.9 rustversion-1.0.14 simdutf8-0.1.4 unicode-ident-1.0.12
So although easybuilders/easybuild-easyblocks#3764 is a correct fix, we don't actually need to provide a list of crates at all for this pymatgen easyconfig, since all crates required by the orjson extension 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.
|
@boegelbot please test @ jsc-zen3 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2952147992 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
…requires in source tarball
|
@boegelbot please test @ jsc-zen3-a100 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2952492705 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
Micket
left a 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.
lgtm
(created using
eb --new-pr)partial fix for #22763