-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add virtual packages field to the Environment model #14979
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
Conversation
@@ -269,6 +274,14 @@ def merge(cls, *environments): | |||
) | |||
) | |||
|
|||
virtual_packages = list( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any restrictions when merging virtual packages for an environment? Like, should we ensure that there are not colliding packages with the same name but different version/build? Or any other rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Virtual packages are PackageRecords
, so they only take a scalar version (not a range). They are usually work as a lower bound in the solver. So I guess the strictest merge in the event of two different versions for the same package would be to keep the largest version (strictest lower bound); e.g. given __linux=2.0
and __linux=2.1
, we would keep __linux=2.1
.
However other virtual packages, like archspec
are not so easily comparable and it might not make sense to solve a conflict between archspec=0=x64
and archspec=0=m1
because they won't be satisfiable ever.
I don't know what you want to do here, but these are the options:
- Merge lists by keeping the latest version of repeated packages, when applicable. This will require checking all known virtual packages and some custom logic for exceptions such as
archspec
. They re technically plugins so we might find others in the wild, but in practice these are unlikely. - Always error out if there are different packages for the same virtual name.
I think that a logic for the first option might be something like this. Given two package records for the same virtual package name:
- If all build strings are the same, keep the latest version.
- If the build strings differ, raise error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync'd offline about this, summarizing the findings here:
- we maybe don't want to be doing any merging of any type of package in the model. Doing operations against packages, like comparing versions, ect. should all be handled by the solver layer
- For example consider a similar case of merging environment models that have multiple requested specs with the same name but different versions. I think we would want the merged environment to have the whole list of requested packages, and then the solver will work out the rest.
- Also, consider how virtual packages are handled currently. They get slurped up into the solve, similar to a package. So currently, the responsibility of reporting errors having to do with virtual packages is in the solver layer. This makes sense, because this layer is dealing with understanding the relationship between packages and encodes all the rules around how packages (and their versions) should relate to each other. I think we wouldn't want to duplicate logic related to this in other areas of the code.
- In the most ideal situation we maybe want to be dealing with this in
conda_libmamba_solver.state
- but we dont' need to implement that here/just yet.
pre-commit.ci autofix |
CodSpeed Instrumentation Performance ReportMerging #14979 will not alter performanceComparing Summary
|
7206b48
to
2ca7255
Compare
2ca7255
to
7255252
Compare
Can you add a news item? Adding this PR number to this line https://github.com/soapy1/conda/blob/7255252fbe6289eeaa358ae0b37d92b735b125fc/news/14870-add-environment-data-model#L3 will be enough. |
seems like one test so far is failing with a transient networking issue, will restart the failing test when the others have run |
f67b9ae
to
62e78f5
Compare
Description
fixes: conda/conda-planning#66
Checklist - did you ...
news
directory (using the template) for the next release's release notes?