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

Opt-in feature to enable pyo3/multiple-pymethods feature #475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jfortin42
Copy link

@jfortin42 jfortin42 commented Dec 6, 2024

Hi there,

Context

We’ve encountered an issue while using the rust-numpy crate in conjunction with the pyo3 multiple-pymethods feature in one of our internal projects. This feature is essential for implementing methods on a structure across multiple files, which helps in organizing larger codebases. However, since rust-numpy does not currently enable this feature, it caused conflicts when trying to use both crates together.

Reproducible example

reproducible_example.zip

What this pull request does

Simply adds a opt-in multiple-pymethods feature to enable the pyo3/multiple-pymethods feature.

We believe this change will improve the usability of rust-numpy for users with similar requirements
Thank you for considering this PR!

Best regards,
Nuant Team

@kylebarron
Copy link

kylebarron commented Dec 6, 2024

This would cause the numpy crate to no longer compile for pyodide (target wasm32-unknown-emscripten). PyO3/pyo3#2517

I don't think it should be necessary for numpy to enable this feature for you to be able to use it in your own project.

@jfortin42
Copy link
Author

jfortin42 commented Dec 6, 2024

This would cause the numpy crate to no longer compile for pyodide (target wasm32-unknown-emscripten). PyO3/pyo3#2517

I don't think it should be necessary for numpy to enable this feature for you to be able to use it in your own project.

The idea is to offer an opt-in feature to enable pyo3's multiple-pymethods feature in the numpy crate (as is already the case for the pyo3/gil-refs feature)

@Icxolu
Copy link
Contributor

Icxolu commented Dec 6, 2024

There is no need to do that. rust-numpy has no dependency on that feature and cargo will do feature unification behind the scenes. So if you depend on pyo3 with that feature enabled, cargo will compile rust-numpy agains it as well.

With a simple starter project I have no problem compiling with the following Cargo.toml

pyo3 = { version = "0.22", features = ["multiple-pymethods"] }
numpy = "0.22"

(gil-refs was different, because rust-numpy needed to conditionally compile itself as well.)

@jfortin42
Copy link
Author

jfortin42 commented Dec 6, 2024

There is no need to do that. rust-numpy has no dependency on that feature and cargo will do feature unification behind the scenes. So if you depend on pyo3 with that feature enabled, cargo will compile rust-numpy agains it as well.

This is not the case with proc-macros, which is our use case:

Build-dependencies and proc-macros do not share features with normal dependencies.

source: https://doc.rust-lang.org/cargo/reference/features.html#feature-resolver-version-2

With a simple starter project I have no problem compiling with the following Cargo.toml

You can take a look at the simple example we've provided which highlights this problem.

@Icxolu
Copy link
Contributor

Icxolu commented Dec 6, 2024

I can't really tell whos fault this is. From my perpective these should be completely independend builds that should interfere with each other. My guess would be that this is some weird inventory sideeffect.

You can simply add pyo3 with that feature enabled along side rust-numpy and take advantage feature-unification for the proc-macro build by itself to make it compile, but whether this actually will work as intended and not unpredictably share some code between the proc-macro and the lib I can't tell.

Anyway I still don't think adding a feature makes sense, it's always possible to also add pyo3 directly as well and enable it that way.

@jfortin42 jfortin42 changed the title Enable pyo3/multiple-pymethods feature Opt-in feature to enable pyo3/multiple-pymethods feature Dec 6, 2024
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.

3 participants