Skip to content
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

Restore compatibility with Rust 1.74 #563

Merged
merged 2 commits into from
Feb 4, 2025
Merged

Conversation

tiran
Copy link
Contributor

@tiran tiran commented Jan 24, 2025

What does this PR do?

PR #544 added support for no_std feature. The PR changed std::error::Error to core::error::Error. The core::error trait was stabilized in Rust 1.81, so the change bumped MSRV to 1.81. Before the Python package built with Rust 1.66 and the safetensors create with all features built with 1.74.

This commit restores compatibility with Rust 1.74 for std builds:

  • mixed_integer_ops feature requires 1.66
  • half v2.4.1 requires 1.70
  • clap_lex v0.7.4 requires 1.74

I'm also adding rust-version to Cargo.toml, so cargo creates a backwards compatible Cargo.lock. By default, Cargo >= 1.83 creates a v4 lock file, which is not compatible with Cargo < 1.78.

PR huggingface#544 added support for `no_std` feature. The PR changed
`std::error::Error` to `core::error::Error`. The `core::error` trait was
stabilized in Rust 1.81, so the change bumped MSRV to 1.81. Before the
Python package built with Rust 1.66 and the `safetensors` create with
all features built with 1.74.

This commit restores compatibility with Rust 1.74 for `std` builds:

- `mixed_integer_ops` feature requires 1.66
- `half v2.4.1` requires 1.70
- `clap_lex v0.7.4` requires 1.74

I'm also adding `rust-version` to `Cargo.toml`, so cargo creates a
backwards compatible `Cargo.lock`. By default, Cargo >= 1.83 creates a
`v4` lock file, which is not compatible with Cargo < 1.78.

Signed-off-by: Christian Heimes <christian@python.org>
@Narsil
Copy link
Collaborator

Narsil commented Feb 4, 2025

Do you mind explaining a bit more the where and what of the issue (what distro, build system etc ?) ?

I'm curious why cargo new doesn't define rust-version as a default in Cargo.toml directly.
Thanks for the fix.

@Narsil
Copy link
Collaborator

Narsil commented Feb 4, 2025

I updated the github action to unify both locations for testing.

@Narsil Narsil merged commit 581f43b into huggingface:main Feb 4, 2025
26 of 27 checks passed
@tiran
Copy link
Contributor Author

tiran commented Feb 4, 2025

@Narsil Thanks for merging my PR!

Sure, I'm happy to tell you a bit more about our work. I'm a member of Red Hat's AI platform core components team. We are providing AI stacks for several AI accelerators (CUDA, Gaudi, ROCm, ...) for other teams in Red Hat. As part of our workflow, we are rebuilding all Python wheels from sources and with system packages. This is done for security and compliance reasons (e.g. SBOM), but also to build packages with special settings, optimizations, or to support more hardware.

Python wheels are built in our Fromager pipeline on RHEL 9. We have created Fromager to automate re-building a dependency tree from sdists. The tool also does some extra steps to create self-contained source distributions. For Rust-based packages (maturin, setuptools-rust), it cargo vendors all crates into an sdist, too.

We ran into problems with Cargo.lock v4 when Rust 1.83 made v4 the default for projects without lower MSRV. For compatibility with hardware vendors, we have to support even RHEL y-stream releases with extended update support (e.g. 9.4) and cannot update to 9.5. RHEL 9.4 has with Rust 1.75, which does not understand Cargo.lock v4. I honestly don't know why cargo new doesn't add MSRV automatically...

Our build system has the capability to apply patches and we could just fix our build problem in a downstream patch. However we prefer to contribute our fixes to upstream, so upstream and other downstream rebuilders benefit from our work, too. Safetensors is the third project I fixed. The blake3 maintainer has merged my MSRV change, too. My hf_transfer PR is waiting for review.

Feel free to ping me if you have more questions. I'm planning to write a longer explanation around downstream packaging and MSRV for Python packages with Rust extensions, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants