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

chore: update to maturin's recommended project layout for rust/python… #695

Merged
merged 2 commits into from
May 14, 2024

Conversation

Michael-J-Ward
Copy link
Contributor

@Michael-J-Ward Michael-J-Ward commented May 14, 2024

Supersedes #694

Rationale for this change

The previous layout leads to an import error when installing with maturin build and pip install ..

This error was common enough that maturin changed the recommended project layout to what this commit does.

A prior PR attempted to solve this by altering lib.name in Cargo.toml, but that did not work for me.


Maintainer of pyo3 explains the issue on the prior PR

In particular the problem is that maturin develop (or pip install -e .) will use the checkout's Python sources, but pip install . will put a copy into site-packages, which then conflicts with the checkout sources if you're running Python from the repository root. (You'll end up importing from the local sources rather than the installed contents in site-packages.)

… projects

The previous layout leads to an import error when installing with `maturin build` and `pip install .`.

This error was common enough that `maturin` changed the recommended project layout to what this commit does.

A prior PR attempted to solve this by altering `lib.name` in Cargo.toml, but that did not work for me.

- [Prior PR](apache#694)
- [maturin ImportError issue](PyO3/maturin#490)
- [maturin changes recommended project structure](PyO3/maturin#855)
Copy link

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still suggest changing Cargo.toml too, to _internal, that's what we have in pydantic-core which David probably set, so is likely to be right.

@Michael-J-Ward
Copy link
Contributor Author

I'm sure @andygrove would have the same Q as on previous PR.

@davidhewitt, could you spare a little more knowledge about the effects of changing lib.name?

Does this have any impact on downstream maturin projects that use this crate as a dependency? If they also use the name _internal, will there be a conflict?

Should we replace all references to _internal with datafusion_internal instead?

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @Michael-J-Ward

@Michael-J-Ward
Copy link
Contributor Author

@andygrove , I have another commit queued up if you want to switch the lib.name and python module from _internal to _datafusion_internal

Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request May 14, 2024
This apes what `pydantic-core` does and was recommended as part of fixing the maturin build issue.

- [pydantic-core](https://github.com/pydantic/pydantic-core/blob/e1fc99dd3207157aad77defc20ab6873fd268b5b/Cargo.toml#L49-L51)
- [suggestion](apache#695 (review))
@andygrove
Copy link
Member

@andygrove , I have another commit queued up if you want to switch the lib.name and python module from _internal to _datafusion_internal

I think we can undo this last commit and ignore my question/concern. I missed that the maturin project definition was already qualifying _internal as datafusion._internal, so I think there should be no conflict with downstream maturin projects.

Sorry for the noise.

@Michael-J-Ward
Copy link
Contributor Author

@andygrove, the commit that changes lib.name is not in this PR. It's in a different branch of my fork.

I created my own noise be referencing the suggestion in this PR in that commit message.

@andygrove andygrove merged commit 0c82b3f into apache:main May 14, 2024
21 checks passed
@davidhewitt
Copy link

Looks like you figured this out already, but yes I agree no need to change lib.name here 👍

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.

4 participants