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

Package Orderers now apply to variants #1684

Conversation

isohedronpipeline
Copy link
Contributor

@isohedronpipeline isohedronpipeline commented Mar 15, 2024

Fixes #369
Fixes #1683

This PR fixes this bug so that package orderers are applied to all packages and variants uniformly in all circumstances, for more expected results.

Carries on the work originally started by @pmolodo with #355 and #413.

Here is an example of how one might take advantage of package ordering applying to variants:
Consider a package python which has versions ['2.6.4', '2.7.16', '3.7.9', '3.9.6', '3.10.8']
The default sorted package orderer would give us this sorted list:
['3.10.8', '3.9.6', '3.7.9', '2.7.16', '2.6.4']
And we end up choosing the first possible option in the resolve: 3.10.8

If we apply the following package order:

package_orderers = [
    {
       "type": "per_family",
       "orderers": [
            {
                "packages": ["python"],
                "type": "version_split",
                "first_version": "2.7.16"
            }
        ]
    }
]

Then we would get packages ordered like this: ['2.7.16', '2.6.4', '3.10.8', '3.9.6', '3.7.9'], where we start at the version_split first version.
Which in turn would resolve to 2.7.16. So far so good. This is the current behavior.

Now consider a package pipeline which has variants:

[
    ["python-2.6.4"],
    ["python-2.7.16"],
    ["python-3.7.9"],
    ["python-3.9.6"],
]

If you do rez env pipeline we would get python-3.9.6 in the resolve, even if we have a package orderer specifically saying we want to prefer python 2.7.16. In order to get the correctly requested version of python, we must include it in the resolve: rez env pipeline python-2.7.16, which largely defeats the purpose of the package orderer, since the point of it is to inform the choices of packages for second order package requests.

@JeanChristopheMorinPerso and I have discussed the possible speed implications. There should be neglible differences, even for very large repositories.

Variants were already being sorted, but this change just changes them to be sorted according to a sort key, in a uniform way to first order requests, instead of simply by version. The sort key for more complicated package orderers are cached anyway, so it really should be very quick.

Since this will change the way packages are resolved, there is a small danger of people no longer getting the same resolve they used to. However, it is the belief of @JeanChristopheMorinPerso, @maxnbk, and I that anyone who is using package ordering would have expected this to be the correct result anyway, and the number of people using package orderers is small enough.

Signed-off-by: Ben Andersen <ben@isohedron.com.au>
@isohedronpipeline isohedronpipeline force-pushed the package_orderer_variants_fix branch from c200011 to cde1475 Compare March 17, 2024 08:51
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Attention: Patch coverage is 87.26415% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 58.23%. Comparing base (c216f9d) to head (2993f66).
Report is 3 commits behind head on main.

Files Patch % Lines
src/rez/package_order.py 86.84% 20 Missing and 5 partials ⚠️
src/rez/resolved_context.py 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1684      +/-   ##
==========================================
+ Coverage   58.05%   58.23%   +0.18%     
==========================================
  Files         126      126              
  Lines       17035    17143     +108     
  Branches     3490     3502      +12     
==========================================
+ Hits         9889     9983      +94     
- Misses       6482     6494      +12     
- Partials      664      666       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Ben Andersen <ben@isohedron.com.au>
@isohedronpipeline isohedronpipeline force-pushed the package_orderer_variants_fix branch from 7d040b0 to 6288676 Compare March 17, 2024 22:03
@maxnbk maxnbk added this to the Next milestone Mar 27, 2024
src/rez/version/_version.py Outdated Show resolved Hide resolved
src/rez/package_order.py Outdated Show resolved Hide resolved
src/rez/package_order.py Outdated Show resolved Hide resolved
src/rez/package_order.py Outdated Show resolved Hide resolved
src/rez/package_order.py Outdated Show resolved Hide resolved
src/rez/package_order.py Outdated Show resolved Hide resolved
src/rez/resolved_context.py Show resolved Hide resolved
src/rez/solver.py Outdated Show resolved Hide resolved
src/rez/solver.py Outdated Show resolved Hide resolved
src/rez/solver.py Outdated Show resolved Hide resolved
isohedronpipeline and others added 3 commits March 27, 2024 14:28
Co-authored-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Signed-off-by: Ben Andersen <156872503+isohedronpipeline@users.noreply.github.com>
Signed-off-by: Ben Andersen <ben@isohedron.com.au>
Signed-off-by: Ben Andersen <ben@isohedron.com.au>
@isohedronpipeline isohedronpipeline force-pushed the package_orderer_variants_fix branch from acbbfb8 to 7566066 Compare March 27, 2024 06:28
isohedronpipeline and others added 6 commits March 28, 2024 15:23
Co-authored-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Signed-off-by: Ben Andersen <156872503+isohedronpipeline@users.noreply.github.com>
Co-authored-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Signed-off-by: Ben Andersen <156872503+isohedronpipeline@users.noreply.github.com>
Co-authored-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Signed-off-by: Ben Andersen <156872503+isohedronpipeline@users.noreply.github.com>
Co-authored-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Signed-off-by: Ben Andersen <156872503+isohedronpipeline@users.noreply.github.com>
Co-authored-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Signed-off-by: Ben Andersen <156872503+isohedronpipeline@users.noreply.github.com>
Signed-off-by: Ben Andersen <ben@isohedron.com.au>
Signed-off-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso 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 fixing this bug! flake8 is failing, but apart from that, I'm happy to get this merged in the next release.

Signed-off-by: Ben Andersen <ben@isohedron.com.au>
@JeanChristopheMorinPerso JeanChristopheMorinPerso merged commit d91cde9 into AcademySoftwareFoundation:main Mar 29, 2024
53 checks passed
@isohedronpipeline isohedronpipeline deleted the package_orderer_variants_fix branch March 31, 2024 01:30
Pixel-Minions added a commit to Pixel-Minions/rez that referenced this pull request Sep 26, 2024
Previously, package orderers were only applied on packages. This was an oversight. With this change, they are now applied to variants too.

---------

Signed-off-by: Ben Andersen <ben@isohedron.com.au>
Co-authored-by: Paul Molodowitch <elrond79@gmail.com>
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.

Package orderers are not applied when choosing a variant PackageOrderer classes don't apply to variants
3 participants