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 no-default-feature option #1092

Merged
merged 33 commits into from
Apr 29, 2024

Conversation

olivier-lacroix
Copy link
Contributor

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

Fixes #1067

@olivier-lacroix olivier-lacroix changed the title Add no-default-feature option feat: Add no-default-feature option Mar 31, 2024
@olivier-lacroix olivier-lacroix marked this pull request as ready for review March 31, 2024 06:54
@ruben-arts
Copy link
Contributor

Hey @olivier-lacroix, thanks again, looks already really good!

I would like to extend this with an example, possibly add it to an existing one. Also pixi's own lint environment might benefit from it.

When you add this to an example you'll see that the schema/model.py needs to be updated. If you need any help with that let us know. But its mostly very easy.

@olivier-lacroix
Copy link
Contributor Author

Schema updated @ruben-arts

For some reason, using no-default-feature in the polarify example fails :-/

docs/configuration.md Outdated Show resolved Hide resolved
examples/polarify/pixi.toml Outdated Show resolved Hide resolved
Co-authored-by: Ruben Arts <ruben@prefix.dev>
@olivier-lacroix
Copy link
Contributor Author

Ah indeed, that makes sense @ruben-arts . Maybe displaying a warning if there are no channels configured would be helpful and allow user to add the necessary channels?

@olivier-lacroix
Copy link
Contributor Author

@ruben-arts I think it's great! As far as I am concerned, I think I would have figured out what the problem was with that message :-)

@ruben-arts
Copy link
Contributor

I'm now looking at the platforms which I think don't do the correct thing now. It should act the same as the channels in my opinion as now the default to the default platforms. except when you define them.

@olivier-lacroix
Copy link
Contributor Author

I think most users may want to exclude packages, without excluding default platform or channels. Would it make sense to move away from the boolean parameter, and have an enum? it could be something like

  • from-default-feature = "all"
  • from-default-feature = "no-packages"
  • from-default-feature = "none"

Or something like that with better naming :-). What do you think?

@ruben-arts
Copy link
Contributor

It could be a slippery slope, as features include the complete environment definition. e.g. tasks, dependencies, system-requirements, activation etc. So do you allow for a definition for all of them or do we simplify it?

I think for the first version it's still fine to require the redefinition of channels and platforms but either both or non. So the situation with platforms included but channels excluded is not right.

Normally I like @pavelzw's opinion on these kind of decisions.

@pavelzw
Copy link
Contributor

pavelzw commented Apr 3, 2024

I agree with you that excluding platforms but including channels sounds a bit weird...
IMO the most used parts will be to ignore the default dependencies but only doing this in no-default-feature also sounds a bit odd and needing to repeat oneself with the channels is also not ideal IMO.

What do you think of a ignore-default = ["dependencies"] and ignore-default = ["platforms", "channels", ...]?
This would allow people to just write

lint = { features=["lint"], ignore-default = ["dependencies"] }

in the most relevant use-case and it behaves the way you expect without any magic implicit assumptions involved. WDYT @0xbe7a?

@ruben-arts
Copy link
Contributor

What do you think of a ignore-default = ["dependencies"] and ignore-default = ["platforms", "channels", ...]?
This would allow people to just write

I'm not sure that would make it much less verbose but that could also be added in a second PR. As in the lint example you want to ignore everything apart from the channels/platforms, in most scenarios, as tasks and your editable stuff and activation.scripts are probably not needed in a lint env.

@pavelzw
Copy link
Contributor

pavelzw commented Apr 3, 2024

Hmm, especially with pypi-dependencies and tasks this could be true...

How about ignore-default = true expands to ignore-default = ["dependencies", "activation-scripts", "pypi-dependencies", "tasks"] implicitly and we mention this in the docs?

This would then still add only a single additional config option that allows flexibility with sane defaults for true and false.

@ruben-arts
Copy link
Contributor

I actually like that. I could imagine use-cases like this where the production env uses a conda build package of itself.

[project]
name = "bla"
channel = ["conda-forge"]
platforms = ["linux-64", "osx-64", "osx-arm64"]

[dependencies]
pytorch = "*"
python = "*"

[pypi-dependencies]
bla = { path = ".", editable = true}

[feature.lint]
channel = ["conda-forge"]
platforms = ["linux-64", "win-64", "osx-64", "osx-arm64"]
dependencies = { ruff = "*"}
tasks = {lint = "ruff check"}

[feature.prod.dependencies]
bla = "1.2.3" 

[environments]
lint = { features = ["lint"], ignore-default = true }
default = {solve-group = "default"}
prod = { feature = ["prod"], ignore-default = ["pypi-dependencies"], solve-group = "default"}

@ruben-arts ruben-arts self-assigned this Apr 4, 2024
@tdejager
Copy link
Contributor

tdejager commented Apr 4, 2024

Nice discussion.

The only thing I think is a bit un-expected as a toml user is when a type of a variable changes. So I'd rather have two options ignore-all-defaults = true and ignore-default = ["pypi-dependencies", ..] which are ideally mutually exclusive.

@pavelzw
Copy link
Contributor

pavelzw commented Apr 4, 2024

ignore-all-defaults = true would imply to also ignore channels, platforms.
IMO this makes it in 95% of cases not useful.
Then I would just always use ignore-default = ["dependencies", "pypi-dependencies"] which would make ignore-all-defaults irrelevant (for me).

If we were not to ignore all defaults, this would feel weird to me since it says all in the name 😅

@olivier-lacroix olivier-lacroix force-pushed the nodefaultfeature branch 3 times, most recently from 2c7d9fc to 86a61af Compare April 27, 2024 02:53
@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Apr 27, 2024

@tdejager it's ready for review!

Hopefully this strikes the right balance between practicality, and consistency, by not considering the default feature as a special case that can be added by component to an environment.

In the most recent commit, I undid my refactoring of SolveGroup, which made most sense when we could select individual components of the default feature for an environment, and proposed another one :-/ , grouping all methods operating on collections containing a group of features (SolveGroup, Environment and GroupedEnvironment) under a trait. This avoids a fair bit of repetition in the code, but let me know what you think 😄 .

@olivier-lacroix olivier-lacroix force-pushed the nodefaultfeature branch 5 times, most recently from 1cee041 to 926ee98 Compare April 28, 2024 09:30
@olivier-lacroix olivier-lacroix force-pushed the nodefaultfeature branch 3 times, most recently from afd6392 to 02392e1 Compare April 28, 2024 10:45
@tdejager
Copy link
Contributor

Testing the final things with it now.

@ruben-arts
Copy link
Contributor

Had some fun testing, used it in combination with wolfs pet project which worked like a charm:

[feature.ai-forge]
channels = ["https://repo.prefix.dev/ai-forge", "conda-forge"]
platforms = ["linux-64", "osx-64", "osx-arm64"]
dependencies = {whisper-cpp = "*"}

[environments]
ai-forge = {features = ["ai-forge"], no-default-feature = true}

Thanks @olivier-lacroix for building this and @tdejager for the extensive review!

@ruben-arts ruben-arts merged commit 79ad23c into prefix-dev:main Apr 29, 2024
28 checks passed
@olivier-lacroix olivier-lacroix deleted the nodefaultfeature branch May 19, 2024 03:40
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.

Add no-default-feature = true to environment definition
5 participants