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

feat: Add the project itself as an editable dependency #1084

Merged
merged 23 commits into from
Apr 4, 2024

Conversation

olivier-lacroix
Copy link
Contributor

@olivier-lacroix olivier-lacroix commented Mar 29, 2024

Fixes #1083

@pavelzw
Copy link
Contributor

pavelzw commented Mar 29, 2024

Would this also work for projects that are compiled with maturin for example?

@tdejager
Copy link
Contributor

Would this also work for projects that are compiled with maturin for example?

If you would specify that correctly in the pyproject.toml it should. However, I'm kinda unsure about the UV BuildIsolation so we would need to check that it uses the rust and cargo from the conda environment.

@pavelzw
Copy link
Contributor

pavelzw commented Mar 29, 2024

Would be nice if pixi read the build-system.requires in some way

@olivier-lacroix
Copy link
Contributor Author

Note this does not seem to work currently

running cargo run shell --manifest-path examples/flask-hello-world-pyproject/pyproject.toml

then python

then import flask_hello_world_pyproject

yields a ModuleNotFoundError

@pavelzw
Copy link
Contributor

pavelzw commented Mar 29, 2024

I just tested maturin with the regular editable install feature from main.
There, it doesn't re-compile after changing something in the rust code...
Not sure if this is intentional.

# somewhere in rust code
println!("Hello World");


# ...
pixi run test
Hello World

# change it to 
println!("Hello World 2");

# ... 
pixi run test
Hello World

Seems like pip install -e . only builds once and doesn't rebuild on every code change with maturin. Should pixi have different behavior? We could just advise not to use the editable install mode with maturin projects.

@pavelzw
Copy link
Contributor

pavelzw commented Mar 29, 2024

Maybe having an option to turn off automatically adding . in editable mode could fix this for projects that require maturin etc.?

@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Mar 29, 2024

@pavelzw , does automatic recompilation work with pip install -e ? looking at the maturin doc, it seems to say it works only for edits of the python code.

@pavelzw
Copy link
Contributor

pavelzw commented Mar 29, 2024

yeah it looks like maturin doesn't work that way. maybe adding somewhere in the docs that you should disable this feature (using tool.pixi.no-editable-install = true in pyproject.toml) for projects using maturin etc.

@tdejager
Copy link
Contributor

We can also add a flag or maybe a boolean to always rebuild, although it does kind of have a bit of feature overlap with the pixi update we've been disucssing.

@tdejager
Copy link
Contributor

@olivier-lacroix seems main wasn't merged in this branch yet, so there were no editable installs at all 😄 I've merged main and added some small changes. Hope you dont't mind me pushing on this.

I believe it should now work.

Also maybe we should use a proper more python directory structure for this example, even though this works with setuptools, hatch will fail for example: https://hatch.pypa.io/latest/plugins/builder/wheel/#default-file-selection

Even setuptools recommends having a seperate source folder: https://hatch.pypa.io/latest/plugins/builder/wheel/#default-file-selection

@tdejager
Copy link
Contributor

Also, do we need to expand the CLI to include installing the local editable projects with extra's?

@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Mar 30, 2024

@olivier-lacroix seems main wasn't merged in this branch yet, so there were no editable installs at all 😄

🤦

Indeed, it works :-)

@olivier-lacroix
Copy link
Contributor Author

Also maybe we should use a proper more python directory structure for this example, even though this works with setuptools, hatch will fail for example: https://hatch.pypa.io/latest/plugins/builder/wheel/#default-file-selection

Yeah. I have reorganised the folder into a proper python package

@olivier-lacroix
Copy link
Contributor Author

Maybe having an option to turn off automatically adding . in editable mode could fix this for projects that require maturin etc.?

@pavelzw , I do not see the harm in having it installed as editable, even without the rebuild trigger?

@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Mar 30, 2024

Also, do we need to expand the CLI to include installing the local editable projects with extra's?

Yes maybe. Also, in #1077, I propose to create pixi features & environments based on the extras automatically, which is another way to provide that feature

@olivier-lacroix
Copy link
Contributor Author

Quick question @tdejager , I see the source dependency includes a sha256 in the lock file: when is this value re-rendered?

In case that lockfile is used in a dockerfile, it may be useful to be able to opt-out of recording that hash: otherwise, you cannot cache the layer containing all the dependencies if the hash has changed.

what do you think?

@tdejager
Copy link
Contributor

Quick question @tdejager , I see the source dependency includes a sha256 in the lock file: when is this value re-rendered?

In case that lockfile is used in a dockerfile, it may be useful to be able to opt-out of recording that hash: otherwise, you cannot cache the layer containing all the dependencies if the hash has changed.

what do you think?

This is regenerated when the lock file is invalidated, I think there is the '--frozen' flag or something that bypasses this.

@olivier-lacroix olivier-lacroix force-pushed the selfeditable branch 4 times, most recently from 28f1bc5 to 012d538 Compare March 30, 2024 08:03
@pavelzw
Copy link
Contributor

pavelzw commented Mar 30, 2024

I do not see the harm in having it installed as editable, even without the rebuild trigger?

I can think of some situations (for example in docker containers) where you would want to install the package in non-editable mode. So having an on/off switch might still help in these cases

@olivier-lacroix
Copy link
Contributor Author

Alright. message added @tdejager . I kept it short :-).

Copy link
Contributor

@tdejager tdejager left a comment

Choose a reason for hiding this comment

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

Sorry. meant to select Request changes.

@olivier-lacroix
Copy link
Contributor Author

I have added more details in the doc about the build-system section. Let me know if you have any comment @tdejager

Copy link
Contributor

@tdejager tdejager left a comment

Choose a reason for hiding this comment

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

I think this looks great! WDYT @ruben-arts?

@tdejager
Copy link
Contributor

tdejager commented Apr 3, 2024

Pushed a fix where it was using a wrong path in the test. Let's see if that solves things.

@olivier-lacroix
Copy link
Contributor Author

@tdejager ,added platforms. now the downstream tests pass, but not the cargo tests on windows! any idea where that error could come from?

@olivier-lacroix
Copy link
Contributor Author

It was passing before: is this just a temporary failure?

@olivier-lacroix olivier-lacroix requested a review from tdejager April 3, 2024 09:42
@ruben-arts
Copy link
Contributor

He I've only followed this feature from the side line. What is the conclusion? Do we auto add the project itself or do we initialize with the flask-hello-world-pyproject = { path = ".", editable = true } Because while testing it somehow seems to do both for me.

@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Apr 3, 2024

@ruben-arts we initialise with the flask-hello-world-pyproject = { path = ".", editable = true } which will then install it upon a call to pixi install or such

@olivier-lacroix
Copy link
Contributor Author

@tdejager , happy with this PR? all tests are 🆗

@ruben-arts ruben-arts merged commit e89256e into prefix-dev:main Apr 4, 2024
27 checks passed
@olivier-lacroix olivier-lacroix deleted the selfeditable branch April 20, 2024 09:07
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.

Install project in editable mode when using a pyproject.toml manifest automatically
4 participants