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

Dependency Hell #204

Closed
mejrs opened this issue Sep 18, 2021 · 22 comments
Closed

Dependency Hell #204

mejrs opened this issue Sep 18, 2021 · 22 comments

Comments

@mejrs
Copy link
Member

mejrs commented Sep 18, 2021

Today I read this post on users.rust-lang.org: Dependency Hell.

I don't really know anything about how cargo resolves versions, so I figured I'd raise it here.

Is there something that can be done about this? One of the commenters raised that rust-numpy can reexport nd_array - is that likely to make things easier?

@mtreinish
Copy link
Contributor

mtreinish commented Sep 18, 2021

As one of the people who asked for the ranges in #176 to support building retworkx with rust 1.41 (for a small but dedicated user base on raspberry pi that wanted pre-built wheels on piwheels.org which only will use the debian packaged rust). I have to say while it's a bit annoying to manage the ndarray version (or num-complex which has a similar version range issue see PyO3/pyo3#1798) it's not untenable, you just need to do it manually and carefully (cargo update --precise is helpful here). I view this as more of a cargo issue than anything we can really solve in rust-numpy or any downstream project using rust-numpy. Cargo's lack of ability to reason about msrv and default to update to the latest version of the crate for individual dependencies is the limiting factor here.

As for re-exporting I'm not sure if it will make things easier for the issue posted in that thread or not. Mostly because the issue was more that they needed cargo to resolve the ndarray version to be the same between rust-numpy, their crate, and hdf5. If we re-exported ndarray from rust-numpy it would solve the version issue between their crate and rust-numpy, but I think they would still need that version to line up with what cargo picks for hdf5 too.

@adamreichold
Copy link
Member

I think a section on dependency management using cargo update --precise in this crate's README would be nice. The issue with Cargo behaving a bit unexpectedly w.r.t. the version range specification also just came up again in #188.

I also wonder whether the use case of providing prebuilt wheels using a specific version of Rust could be managed by overriding dependencies for that build?

@adamreichold
Copy link
Member

Gave extending the README a try in #206

@adamreichold
Copy link
Member

The re-export and the extension of the README was merged. @mtreinish @mejrs Do you think this is sufficient to close or should something else be done?

@mtreinish
Copy link
Contributor

Yeah, I think that's enough to close it, I don't think we can realistically do anything else here

@bluss
Copy link

bluss commented Nov 13, 2021

What do you think, should we push harder for crates to avoid "<" version specs? They seem to universally cause trouble. They can initially seem like a good idea, but ecosystem wide, it's a problem.

(Edit: I'm speaking as ndarray maintainer, so I'm a good person to request changes with ndarray.)

@adamreichold
Copy link
Member

Personally, I think that

Looking at ndarray's reverse dependencies, the plain ndarray = "0.15" seems preferable from an ecosystem perspective.

as discussed in #188. But in this particular case, this would probably require the Raspi builds being done using dependency override?

@adamreichold
Copy link
Member

@mtreinish Is the scripting producing those builds Rust 1.41 public? Could you post a link here? I would really like to investigate whether this could be handled using dependency overrides so that rust-numpy itself could migrate to the dependency

ndarray = "0.15"

which would be more useful from an ecosystem perspective.

@mtreinish
Copy link
Contributor

It's for piwheels I don't have all the details of the build system. The build system code is hosted at https://github.com/piwheels/piwheels. AFAIK the default workflow for each package is to just do pip wheel $package --no-deps for things on pypi with an sdist.

This is the PR where I added rustc/cargo to their build env: piwheels/piwheels#263. I had previously attempted to use rustup and got a nack from the maintainers on that approach as they would only use the debian packaged compiler. They do have a mechanism for additional options/steps on specific packages but I don't know how that works or where it's documented.

@adamreichold
Copy link
Member

If I follow this correctly, this would basically when to include the right

[patch.crates-io]
ndarray = "0.13"
num-complex = "0.2"

directives in the Cargo.toml of retworkx which is then part of its sdist and thereby enable building that sdist on older Debian versions?

Would you be alright with this approach (and thereby overriding the dependencies specified by rust-numpy)? We can still test this scenario in the CI here so that we do not accidentally break it.

I think this would be quite beneficial for the Rust ecosystem as retworkx (in contrast to retworkx-core) seems to be a leaf crate so that override seems bearable IMHO if it enables better dependency resolution in most other crates depending on rust-numpy.

@adamreichold
Copy link
Member

I am think the whole discussion is moot the build overrides mechanism allows to replace the source of dependency but not its version specification, so downstream crates can never use "0.13.0" if we specify "0.15.0" here, even if they explicit patch the crates.io dependency. 🤔

@adamreichold
Copy link
Member

Things will also change with the next PyO3 release which has an MSRV of 1.48 so Buster does not work anymore, but Bullseye which is currently at 1.48 will.

The only thing that would prevent the desired ndarray = "0.15" dependency from working out then is that ndarray 0.15.0 bumped the MSRV to 1.49. @bluss Do you think supporting 1.48 and thereby Debian Bullseye would be possible for ndarray?

@adamreichold
Copy link
Member

adamreichold commented Jan 5, 2022

But then again, the next release of ndarray will have an MSRV of 1.51, probably due to using const generics, and for the ecosystem service, it isn't ndarray = "0.15" that is important, but rather ndarray = "0.<latest-release>". So we'll probably either end up ndarray = ">= 0.14, < 0.16" to support Rust 1.48 or ndarray = "0.16" with an MSRV of 1.51 which would rule out all Debian native builds.

@mtreinish I also guess piwheels is about being able to build from source using a Debian-supplied toolchain, meaning that manylinux binary wheels built using a current toolchain would not be helpful?

@adamreichold
Copy link
Member

I have to admit that since Debian essentially freezes the toolchain version, there might be a point where the answer would be that rust-numy version 0.x (e.g. 0.15) is the last version supported on Debian release y (e.g. Buster). :-(

@mtreinish
Copy link
Contributor

I also guess piwheels is about being able to build from source using a Debian-supplied toolchain, meaning that manylinux binary wheels built using a current toolchain would not be helpful?

Yeah, that's the issue if we want to support people being able to publish packages built with rust-numpy on piwheels it needs to be built with a Debian supplied toolchain. In general I would prefer to just use rustup inside the manylinux containers to build the packages, that's what we do for the wheels published to pypi. But we don't control the piwheels environment

Honestly, for retworkx I'm just debating for raspberry pi to drop 32bit support outside of building from source because we're already publishing manylinux arm64 wheels on pypi that work fine on raspberry pi 3 and 4 with a 64 bit OS install. But I'm waiting for 64bit raspbian to get a bit more prevalent before I do that.

@adamreichold
Copy link
Member

Honestly, for retworkx I'm just debating for raspberry pi to drop 32bit support outside of building from source because we're already publishing manylinux arm64 wheels on pypi that work fine on raspberry pi 3 and 4 with a 64 bit OS install. But I'm waiting for 64bit raspbian to get a bit more prevalent before I do that.

When we upgrade to PyO3 0.16 and hence bump the MSRV to 1.48, would that be alright with you if we go to 1.49 instead and bump ndarray to >=0.15 as well or would that be too early in your opinion? This would not affect already released versions on 32 bit platforms and hence existing locked projects.

@adamreichold
Copy link
Member

ndarray 0.15 certainly seems to fail to build spectacularly using rustc 1.48.0 due to

error[E0277]: the trait bound `<<Self as data_traits::DataOwned>::MaybeUninit as data_traits::DataOwned>::MaybeUninit: RawDataSubst<MaybeUninit<<Self as data_traits::RawData>::Elem>>` is not satisfied
   --> src/data_traits.rs:520:23
    |
518 | pub unsafe trait DataOwned: Data {
    |                  --------- required by a bound in this
519 |     /// Corresponding owned data with MaybeUninit elements
520 |     type MaybeUninit: DataOwned<Elem = MaybeUninit<Self::Elem>>
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `RawDataSubst<MaybeUninit<<Self as data_traits::RawData>::Elem>>` is not implemented for `<<Self as data_traits::DataOwned>::MaybeUninit as data_traits::DataOwned>::MaybeUninit`
521 |         + RawDataSubst<Self::Elem, Output=Self>;
    |           ------------------------------------- required by this bound in `data_traits::DataOwned`
    |
help: consider further restricting the associated type
    |
518 | pub unsafe trait DataOwned: Data where <<Self as data_traits::DataOwned>::MaybeUninit as data_traits::DataOwned>::MaybeUninit: RawDataSubst<MaybeUninit<<Self as data_traits::RawData>::Elem>> {
    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@mtreinish
Copy link
Contributor

When we upgrade to PyO3 0.16 and hence bump the MSRV to 1.48, would that be alright with you if we go to 1.49 instead and bump ndarray to >=0.15 as well or would that be too early in your opinion? This would not affect already released versions on 32 bit platforms and hence existing locked projects.

Not really, because it would block us from upgrading to the next rust-numpy release until we're ready to drop support for piwheels (which we're not there yet). It also contradicts the rust version support policy in pyo3: https://github.com/PyO3/pyo3/blob/main/Contributing.md#python-and-rust-version-support-policy which the readme here says rust-numpy follows: https://github.com/PyO3/rust-numpy#requirements

@adamreichold
Copy link
Member

Not really, because it would block us from upgrading to the next rust-numpy release until we're ready to drop support for piwheels (which we're not there yet).

Fair enough.

It also contradicts the rust version support policy in pyo3: https://github.com/PyO3/pyo3/blob/main/Contributing.md#python-and-rust-version-support-policy which the readme here says rust-numpy follows: https://github.com/PyO3/rust-numpy#requirements

Indeed, but I suspect this might become untenable eventually if there is a point where we cannot support the latest ndarray release and one sufficiently old to build on Debian/RHEL/Alpine as a single dependency. Maybe we could have two features ndarray (enabled by default) and ndarray_0_13 (disabled by default), but the maintenance cost of actually making them additive might be problematic.

@davidhewitt
Copy link
Member

Note that after PyO3 0.16 I doubt I'll propose raising MSRV again until at least the end of 2022 (depending on what Debian/RHEL/Alpine support as the year ends).

Maybe we could have two features ndarray (enabled by default) and ndarray_0_13 (disabled by default), but the maintenance cost of actually making them additive might be problematic.

At first glance I agree this sounds super painful. I suspect this might end up being the right solution if we want to offer maximum support for users. It's worth noting that if we wanted to support an nalgebra optional dependency it would probably create a very similar set of problems as ndarray and ndarray_0_13 features would, so maybe there's value in trying to abstract things to make having these features possible?

@adamreichold
Copy link
Member

It's worth noting that if we wanted to support an nalgebra optional dependency it would probably create a very similar set of problems as ndarray and ndarray_0_13 features would, so maybe there's value in trying to abstract things to make having these features possible?

Yes, I think so. It might involve a few breaking changes where inherent methods need to be pushed into traits so that we can have multiple implementations under different names targeting different versions of ndarray.

@adamreichold
Copy link
Member

Closing as the current state of affairs has been discussed and acted upon. The discussion of how to better support older versions of ndarray and nalgebra is continued in #263 (comment)

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 a pull request may close this issue.

5 participants