Continue on missing variant packages

[dgovil]

This PR adds a new config variable called error_on_missing_variant_requires that controls what happens when a variant lists missing packages in its requirements.

By default, it is True and will continue the existing behaviour of erroring when it encounters a variant with missing packages in its list of requirements. This means that if the first variant encounters a missing package in its request, it will not continue even if the second variant can resolve.

If it is disabled, it will print to stderr, treat the current phase as failed and continue on to the next phase. If all variants fail, it will be the same as if no variants could resolve.

This is of use to use internally because we often encounter the following scenarios:

1. Since we only cache the packages that have been resolved, it means that when a user goes offline, they hit resolve errors because they can't access packages used by a variant that they aren't even trying to resolve.
2. For the same reason, when we introduce other platforms into the mix, they won't cache down macOS specific packages and error when a resolve tries that path.
3. Some of our users aren't allowed to access certain packages due to security reasons.

In each of these cases, another variant further down the list would have successfully resolved, but does not get the chance since an exception is immediately raised.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: dgovil / name: Dhruv Govil (90e91b9, 958cb14, c3dc3c5, de55d7f, 3aa2b5c, 21a87e7, 5650b12, 5621e93, 2313693, 7698459, 94579ce, 678cdb0)
• ✅ login: JeanChristopheMorinPerso / name: Jean-Christophe Morin (999526e, 4fa2678)

[JeanChristopheMorinPerso]

Hi @dgovil ! THanks for creating this PR! I remember our discussion on that a couple of months ago and I think it make sense.

Could you fix the DCO check (you can follow the instructions in https://github.com/AcademySoftwareFoundation/rez/pull/1550/checks?check_run_id=17914385709) and also edit the PR description to replace package/variants where appropriate?

For example:

1c1
< By default, it is True and will continue the existing behaviour of erroring when it encounters a variant with missing packages. This means that if the first variant encounters a missing package, it will not continue even if the second variant can resolve.
---
> By default, it is True and will continue the existing behaviour of erroring when it encounters a package with missing variants. This means that if the first package encounters a missing variant, it will not continue even if the second variant can resolve.

I feel like that would make more sense or did I misunderstood something?

[JeanChristopheMorinPerso]

Thanks for adding a test! I think we should also add a test to test when only one variant is missing?

[dgovil]

Addressed the notes above:

1. I changed it from missing_variant_packages to missing_variant_requires because that is the term that seems to be used for the list of items in a variant per the Scope code.
2. Moved it to the package resolution section
3. Clarified the PR title+description that its talking about the variant requirements based on the term used by the Scope solve code

Let me know if there's anything else I can do for this PR.

[dgovil]

I fixed the linter issues. I don't know how to fix the copyright one. I filed #1554 because the copyright script doesn't work on macOS, but I don't see copyright information on any of the other tests. I tried adding the generic spdx header from other files but it doesn't seem to be happy with that either.

[JeanChristopheMorinPerso]

@dgovil the copyright workflow is now passing. We should really rework it as it's non intuitive and always give us problems. I'd like o move to use an open source tool instead of writing our own.

[JeanChristopheMorinPerso]

Note to myself or @dgovil, we should run the benchmarks to ensure that we didn't change the solver in an unexpected way. Feel free to run them, but I will also understand if you don't feel like it since they are not documented at all. If you don't want to run them, I'll run them for you. Just let me know.

[dgovil]

How does one run it?

[JeanChristopheMorinPerso]

I think I'll just push a commit on main to enable us to run the benchmark on selected PRs (via a label). But if you are curious, see https://github.com/AcademySoftwareFoundation/rez/blob/main/.github/workflows/benchmark.yaml.

[JeanChristopheMorinPerso]

@dgovil I merged the main branch and the benchmark will now run on your PR.

[JeanChristopheMorinPerso]

And there seems to no no noticeable differences in terms of spee and no resolve failed: https://github.com/AcademySoftwareFoundation/rez/actions/runs/6661912380/attempts/1#summary-18105674262

[JeanChristopheMorinPerso]

Great job @dgovil ! Thanks for editing the docs. I think they are clearer now. We might want to eventually add more docs on when this is useful, but for this PR, I think the documentation is enough.

Your PR will be released in rez 2.114.0 or 3.0.0. I'm not yet sure in which release it will go. 2.114.0 should hopefully be released in a week or two. 3.0.0 is planned for early january.

[dgovil]

Awesome. Thanks for the help and review!

…

On Mon, Nov 6, 2023 at 5:18 PM Jean-Christophe Morin < ***@***.***> wrote: ***@***.**** approved this pull request. Great job @dgovil <https://github.com/dgovil> ! Thanks for editing the docs. I think they are clearer now. We might want to eventually add more docs on when this is useful, but for this PR, I think the documentation is enough. Your PR will be released in rez 2.114.0 or 3.0.0. I'm not yet sure in which release it will go. 2.114.0 should hopefully be released in a week or two. 3.0.0 is planned for early january. — Reply to this email directly, view it on GitHub <#1550 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB4XUUEV46ETTCD62SLDGLYDGD6LAVCNFSM6AAAAAA6JTD27KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJWGYYDKOJWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[maxnbk]

Requested a comment in the config that mem-caching of resolves might be affected ; In fact, this needs to be verified, but was a potential concern that was deemed not important enough to warrant holding the PR back. nonetheless, a note in the config and a TODO in the code would be sufficient, and we should make a github issue to track whether or not it is in fact an issue.
Thanks @dgovil !

[JeanChristopheMorinPerso]

Looks good to me. Thanks again @dgovil ! (FYI, the latest benchmark failed, but it's normal. I forgot to disable py2 benchmarks after in the PR that removed support for Python 2 in install.py).