feat: Add no-default-feature option#1092
Conversation
1e80d92 to
6eae2b7
Compare
|
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 When you add this to an example you'll see that the |
e000b1e to
22398e6
Compare
22398e6 to
1801aa5
Compare
|
Schema updated @ruben-arts For some reason, using |
Co-authored-by: Ruben Arts <ruben@prefix.dev>
|
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? |
|
@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 :-) |
|
I'm now looking at the |
|
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
Or something like that with better naming :-). What do you think? |
|
It could be a slippery slope, as features include the complete environment definition. e.g. I think for the first version it's still fine to require the redefinition of Normally I like @pavelzw's opinion on these kind of decisions. |
|
I agree with you that excluding platforms but including channels sounds a bit weird... What do you think of a 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? |
I'm not sure that would make it much less verbose but that could also be added in a second PR. As in the |
|
Hmm, especially with How about This would then still add only a single additional config option that allows flexibility with sane defaults for |
|
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"} |
|
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 |
|
If we were not to ignore all defaults, this would feel weird to me since it says all in the name 😅 |
2c7d9fc to
86a61af
Compare
86a61af to
0414ac2
Compare
|
@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 😄 . |
1cee041 to
926ee98
Compare
afd6392 to
02392e1
Compare
…ironment in a trait
02392e1 to
04cdb23
Compare
|
Testing the final things with it now. |
|
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! |
Fixes #1067