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: Support pyproject.toml #999

Merged
merged 30 commits into from
Mar 28, 2024
Merged

Conversation

olivier-lacroix
Copy link
Contributor

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

Hi there,

A (very) rough and partial implementation of pyproject.toml support for pixi.

  • A kind field is added to the Manifest struct to differentiate between the pixi.toml and pyproject.toml
  • The kind is detected from the (potentially discovered) manifest file
  • the pyproject.toml is parsed, reading the tool.pixi part(s) as it would the content of pixi.toml
  • pyproject project.requires_python and project.dependencies are injected into the Manifest as conda dependencies, unless they already exist in tool.pixi.dependencies or tool.pixi.pypi_dependencies

Keen to get some early feedback before I progress any further on your interest in such a feature and on the proposed approach.

Would be further required, among others:

  • Handling of the addition / removal of a dependency
  • Handling of extras, mapping them to features & environments

@olivier-lacroix olivier-lacroix changed the title Support pyproject.toml [WIP] Support pyproject.toml Mar 17, 2024
@olivier-lacroix olivier-lacroix changed the title [WIP] Support pyproject.toml feat: Support pyproject.toml Mar 17, 2024
@ruben-arts
Copy link
Contributor

Hello @olivier-lacroix and everyone following this.

The team is very positive on the support for pyproject.toml and we love the work you put in to prepare a first version.

Its however not a small feat to support a second manifest. Next to this PR we would also need to support file modification for the cli commands like pixi add and pixi remove.

So we would like to take some time to design it properly.

Meanwhile we would like you to continue on this PR and get it tested. I will support this PR and start testing right away.

To the community: I would like to ask you to start testing this PR if you can!

Again thanks for the work, we really appreciate the work from the community and hope we can get this feature to a next level, together ❤️!

@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Mar 18, 2024

Thanks @ruben-arts !

Keen to get your feedback on the directions / choices made here as I do not know the codebase very well. It's also my first chunk of Rust code, so pretty sure things can be improved :-).

Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

Some initial thoughts, we should also add a test to make sure we support the pixi full spec in pyproject.toml. It's kinda inherited because you just read the toml table prefixed with tool.pixi but it is a good test to make sure.

src/project/manifest/pyproject.rs Show resolved Hide resolved
src/project/manifest/pyproject.rs Outdated Show resolved Hide resolved
src/project/mod.rs Show resolved Hide resolved
Copy link
Contributor

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

I like where this is going! But I think we need to refactor the structure a little bit more, let me explain:

Currently the manifest::Manifest contains a kind and a toml_edit::Document. I would opt to move both of these fields into an enum that represents and editable source of the Manifest. We have a lot of functions in Manifest that modify this document but depending on whether the source is a pyproject.toml or a pixi.toml this will need to behave differently. To adhere to the single responsibility design I think it makes sense to move this into its own type.

e.g.:

pub enum ManifestSource {
    PyProjectToml(toml_edit::Document),
    PixiToml(toml_edit::Document),
}

src/project/manifest/pyproject.rs Outdated Show resolved Hide resolved
src/project/manifest/pyproject.rs Outdated Show resolved Hide resolved
src/project/manifest/pyproject.rs Outdated Show resolved Hide resolved
src/project/mod.rs Show resolved Hide resolved
@ruben-arts ruben-arts added the 🗒️ configuration Issue related to configuration of project or global behavior label Mar 19, 2024
src/project/manifest/document.rs Outdated Show resolved Hide resolved
src/project/manifest/document.rs Outdated Show resolved Hide resolved
src/project/manifest/document.rs Outdated Show resolved Hide resolved
src/project/manifest/document.rs Outdated Show resolved Hide resolved
@olivier-lacroix
Copy link
Contributor Author

@baszalmstra I took your comments into account.
@ruben-arts do you have such a pixi.toml handy I could convert into an equivalent pyproject.toml?

@ruben-arts
Copy link
Contributor

@olivier-lacroix In theory all pixi.toml should be convertable to a pyproject.toml right? I might not be reading your question correctly.

@olivier-lacroix
Copy link
Contributor Author

Yes @ruben-arts . ButI understood you wanted a test with a file that had all pixi features?

@ruben-arts
Copy link
Contributor

Ah sorry I see, @olivier-lacroix. You could check schema/examples/valid/full.toml, this should include most of it.

@ruben-arts
Copy link
Contributor

He @olivier-lacroix, your going very fast, awesome!

We've taken a good look at how we would want to implement it. The only big change for now is that we want to import the project.dependencies as pypi-dependencies as we cannot trust the map between conda and pypi.

We're currently working hard on supporting pypi source dependencies. Which we also want to mix with this feature where we should install the actual project itself aswell using pixi install if we are using a pyproject.toml as our pixi configuration. Which will hopefully greatly improve this experience.

@ruben-arts
Copy link
Contributor

In my opinion it is ready for review! Awesome work @olivier-lacroix

@olivier-lacroix olivier-lacroix marked this pull request as ready for review March 26, 2024 20:24
@olivier-lacroix
Copy link
Contributor Author

Thank you @ruben-arts !

@ruben-arts
Copy link
Contributor

@olivier-lacroix Added some documentation, let me know if it looks good.

```yaml
- uses: prefix-dev/[email protected]
with:
manifest-path: pyproject.toml
Copy link
Contributor

@pavelzw pavelzw Mar 27, 2024

Choose a reason for hiding this comment

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

Hmm, i'm wondering whether we should make this behavior implicit. Currently it's always running with manifest-path $PWD/pixi.toml

EDIT: let's do this discussion in prefix-dev/setup-pixi#87

Copy link
Contributor Author

@olivier-lacroix olivier-lacroix left a comment

Choose a reason for hiding this comment

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

Thanks a lot for that @ruben-arts !!

docs/advanced/pyproject_toml.md Outdated Show resolved Hide resolved
docs/advanced/pyproject_toml.md Show resolved Hide resolved
Copy link
Contributor

@pavelzw pavelzw left a comment

Choose a reason for hiding this comment

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

Just to be sure: when specifying both [tool.pixi] in pyproject.toml and pixi.toml, pixi would fail?

docs/advanced/pyproject_toml.md Show resolved Hide resolved
@olivier-lacroix
Copy link
Contributor Author

Just to be sure: when specifying both [tool.pixi] in pyproject.toml and pixi.toml, pixi would fail?

Not sure what you mean @pavelzw ?

@pavelzw
Copy link
Contributor

pavelzw commented Mar 27, 2024

When we basically define the pixi configuration two times, once in pixi.toml and once in pyproject.toml. Does pixi handle this error case or just silently ignore one config?

@olivier-lacroix
Copy link
Contributor Author

It will pick pixi.toml in priority, and ignore the pyproject.toml. The same way the discovery for pixi.toml up the folder tree stopped at the first pixi.toml found.

@ruben-arts
Copy link
Contributor

I would be in favor of having this behavior as imo conda packages

Do you(@pavelzw) mean you want the automatic mapping @olivier-lacroix describes in: #532 ?

@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Mar 27, 2024

Hi all, let me know if you expect me to add anything else to this PR before merging.

re. the presence of both pixi.toml and pyproject.toml, let me know if you want pixi to fail, or if documenting the current behaviour is enough (I feel like the current behaviour is OK, but that's just me :-) )

Copy link
Contributor

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Some small doc changes, but otherwise looks good to me!

docs/advanced/multi_platform_configuration.md Outdated Show resolved Hide resolved
docs/advanced/pyproject_toml.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

Thanks a lot @olivier-lacroix!

@ruben-arts ruben-arts merged commit 8f485c6 into prefix-dev:main Mar 28, 2024
19 of 20 checks passed
@olivier-lacroix
Copy link
Contributor Author

Great! thank you all for your guidance & feedback!

@ruben-arts
Copy link
Contributor

@olivier-lacroix we are working on a blog-post where we are also going to mention your work on pyproject.toml. But I'm not sure how to mention you, to give credit where credit's due.

@olivier-lacroix
Copy link
Contributor Author

@ruben-arts , that's very kind of you and also quite unecessary :-). my github handle is not very creative and is my firstname-lastname

@ruben-arts
Copy link
Contributor

@olivier-lacroix will mention your GitHub then! If you have other socials let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗒️ configuration Issue related to configuration of project or global behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants