Can't run pre-install on package if package is filtered out

[herronelou]

Hello,

I have setup a filter so that any package with *.dev in the version is ignored by default. While this works well in general, it has now made it impossible for me to build such a package if the package has unit tests running on pre-install, as it fails to resolve the environment for the tests.

Environment

• OS windows
• Rez version 2.104.9
• Rez python version 3.7.9

To Reproduce

1. In config, add a filter to exclude dev packages package_filter = {'excludes': '*.dev'}
2. Setup a package, say test_rez_package and add tests meant to run on pre-install.
3. Set version to 1.0.0.dev
4. Attempt to build

Expected behavior Package should build and run unit tests

Actual behavior Build runs successfully, then tests try to kick in:

Running test: unit
----------------------------------

Resolving test environment: test_rez_package==1.0.0.dev pyest
17:32:20 ERROR    The test environment failed to resolve: Package could not be found:  test_rez_package==1.0.0.dev
[... Here are tests results saying how it all failed ...]
17:32:20 ERROR    PackageTestError: 1 tests failed; the installation has been cancelled

and the install directory gets deleted.

** Notes ** Is it actually a bug, or is it the filter doing its job? I'm unsure, but it sure is annoying.

[herronelou]

I've done some thinking about this, and it might be tricky to to implement without changing any legacy behavior.

Instead, I might give a try to implement it as a new type of filter (to put in includes).

I don't know if there is a specific term used to describe requests with the '==' symbol, as described in the wiki:

package request	description	example versions within request
foo==2.0.0	Only version 2.0.0 exactly	foo-2.0.0

Assuming this is called an "exact" request, I would make a new filter exact_request(), which could be used to include packages if they match an exact request.

Assume the following filter:

package_filter = {
    'excludes': 'glob(*.beta)',
    'includes': 'exact_request()'
}

Assuming a package: foo, with versions foo-1.0.0 and foo-2.0.0.beta:

• Running rez-env foo would resolve foo-1.0.0
• Running rez-env foo-2.0.0.beta would give a resolve error
• Running rez-env foo==2.0.0.beta would resolve to foo-2.0.0.beta

Someone could possibly add that in the excludes, which would exclude all exact requests, I don't know why they would, but they could.

In order to implement, the PackageFilter class would need some info about the request (an extra optional argument on the excludes() method?). It seems doable as only the solver ever calls the filter and it has a version_range available right there to pass to the the filter.

Any thoughts?

[herronelou]

Another possible solution, in https://github.com/AcademySoftwareFoundation/rez/blob/master/src/rezplugins/build_process/local.py#L471, do something like:

        package_filters = PackageFilterList.singleton
        package_filter.add_inclusion(variant.parent.as_exact_requirement())
        # run the tests, and raise an exception if any fail. This will abort
        # the install/release
        runner = PackageTestRunner(
            package_request=variant.parent.as_exact_requirement(),
            package_paths=package_paths,
            cumulative_test_results=self.all_test_results,
            stop_on_fail=True,
            verbose=1,
            package_filter=package_filter
        )

This should stay more localized to only the tests happening at build time, while not affecting other tests, and is closer to a bug fix than my previous comment which is more like a new feature. The reason I did like the new filter in the previous comment is because I can also use it with rez-env in general for testing my packages.

[nerdvegas]

The filter type you need already exists no? https://github.com/AcademySoftwareFoundation/rez/blob/master/src/rez/package_filter.py#L450 .

Then:

package_filters = PackageFilterList.singleton.copy() # use copy don't

alter singleton! package_filter.add_inclusion(RangeRule(variant.parent.as_exact_requirement ())) ...rest is the same...

On Tue, Sep 13, 2022 at 3:05 PM Erwan Leroy @.***> wrote:

Another possible solution, in https://github.com/AcademySoftwareFoundation/rez/blob/master/src/rezplugins/build_process/local.py#L471 , do something like:

    package_filters = PackageFilterList.singleton
    package_filter.add_inclusion(variant.parent.as_exact_requirement())
    # run the tests, and raise an exception if any fail. This will abort
    # the install/release
    runner = PackageTestRunner(
        package_request=variant.parent.as_exact_requirement(),
        package_paths=package_paths,
        cumulative_test_results=self.all_test_results,
        stop_on_fail=True,
        verbose=1,
        package_filter=package_filter
    )

This should stay more localized to only the tests happening at build time, while not affecting other tests, closer to a bug fix than my previous comment which is more like a new feature. The reason I did like the new filter in the previous comment is because I can also use it with rez-env in general for testing my packages.

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1244902675, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOUSRNATNHRN6HA4KS243V6ADRRANCNFSM6AAAAAAQIHW2JQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[herronelou]

Yes indeed, this would work while editing the local build process, likely resolving the issue of aborted builds because of tests. (In short, direct fix to what I believe to be a bug)

My earlier idea about a different filter was sort of a new feature, which as far as I know doesn't currently exist. As I'm basing my development/alpha/beta workflow around filtered packages (because I never want them to resolve accidentally), right now I have to both explicitly request them, and add them to the inclusion list to resolve. With the new filter I was hoping to be able to ask "filter out all *.beta versions, unless explicitly requested", so that anywhere I could run "rez env my_package==0.0.1.beta" and it would resolve.

[nerdvegas]

I don't think a new kind of filter is the way to go - having one that knows about a ResolvedContext request seems a bit complex, but the main reason is that such a filter would allow exact package requirements (as opposed to user-defined requirements) to see dev versions too, and I don't think that's right.

I have an alternative suggestion:

• We add a new setting, default_no_filter_on_exact_request
• We add analogous arg to ResolvedContext constructor, defaults to None (ie inherit from setting)
• If True, and package filter is not explicitly specified in ResolvedContext constructor, then we use add_inclusion to add a range-rule-inclusion for every exact request in the request list (ie we make a copy of the default package filter, and add the inclusions there)

Benefits:

• No new package filter impl
• Backwards compatibility (new setting is False by default)
• No chance of leaking into package requirements (which should always obey the filter imo, exact requirement or not)
• Explicitness (the filter stored into the rxt file will show that each exact requirement has been explicitly allowed for all versions)

Thoughts?

On Thu, Sep 22, 2022 at 1:20 AM Erwan Leroy @.***> wrote:

Yes indeed, this would work while editing the local build process, likely resolving the issue of aborted builds because of tests. (In short, direct fix to what I believe to be a bug)

My earlier idea about a different filter was sort of a new feature, which as far as I know doesn't currently exist. As I'm basing my development/alpha/beta workflow around filtered packages (because I never want them to resolve accidentally), right now I have to both explicitly request them, and add them to the inclusion list to resolve. With the new filter I was hoping to be able to ask "filter out all *.beta versions, unless explicitly requested", so that anywhere I could run "rez env my_package==0.0.1.beta" and it would resolve.

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1253861957, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOUSXFIMW43NKZRP3BFBLV7MRSDANCNFSM6AAAAAAQIHW2JQ . You are receiving this because you commented.Message ID: @.***>

[herronelou]

I don't think a new kind of filter is the way to go - having one that knows about a ResolvedContext request seems a bit complex,

agreed, it also kind of breaks single responsibility principles

but the main reason is that such a filter would allow exact package requirements (as opposed to user-defined requirements) to see dev versions too, and I don't think that's right.

You mean the requirements listed in package.py under requires or variant? I think it would, but if somebody puts requires = ['foo==1.0.0.dev'] in their package.py, doesn't it mean they want that version to load?

I have an alternative suggestion:

• We add a new setting, default_no_filter_on_exact_request
• We add analogous arg to ResolvedContext constructor, defaults to None (ie inherit from setting)
• If True, and package filter is not explicitly specified in ResolvedContext constructor, then we use add_inclusion to add a range-rule-inclusion for every exact request in the request list (ie we make a copy of the default package filter, and add the inclusions there)

Yes that was another thing that crossed my mind, but for some reason I thought adding a new filter would be less disruptive / controversial. Could you clarify this part: If True, and package filter is not explicitly specified in ResolvedContext constructor ?

Benefits:

• No new package filter impl

yep

• Backwards compatibility (new setting is False by default)

filter would also be backward compatible?

• No chance of leaking into package requirements (which should always obey the filter imo, exact requirement or not)

Wouldn't the setting still affect the requirements from a package? If I set the global setting to no filters on exact requests, and a package has it as an exact request, wouldn't it still resolve? I'm not sure I fully grasp the difference?

• Explicitness (the filter stored into the rxt file will show that each exact requirement has been explicitly allowed for all versions)

fair enough

Thoughts?

This would totally work for me, but just for the sake of conversation, I'm adding a con to this solution:

• You wouldn't be able to do this type of filter: package_filter:
  • excludes:
    • glob(*.beta)
  • excludes:
    • after(1429830188) includes:
      • explicit_requirement() (only allow explicit requirements for the timestamp filter, not the glob one)

[nerdvegas]

Below.

On Fri, Sep 23, 2022 at 10:02 AM Erwan Leroy @.***> wrote:

I don't think a new kind of filter is the way to go - having one that knows about a ResolvedContext request seems a bit complex, agreed, it also kind of breaks single responsibility principles but the main reason is that such a filter would allow exact package requirements (as opposed to user-defined requirements) to see dev versions too, and I don't think that's right.

You mean the requirements listed in package.py under requires or variant? I think it would, but if somebody puts requires = ['foo==1.0.0.dev'] in their package.py, doesn't it mean they want that version to load?

It probably does mean they want that, but it doesn't mean that I (being the person creating my context) do. If I've filtered out dev packages, I don't want them, unless I have requested one explicitly myself. As a user I want a guarantee that my filter is respected, and if another package requires something I don't want it to, I want my resolve to fail.

I have an alternative suggestion:

• We add a new setting, default_no_filter_on_exact_request
• We add analogous arg to ResolvedContext constructor, defaults to None (ie inherit from setting)
• If True, and package filter is not explicitly specified in ResolvedContext constructor, then we use add_inclusion to add a range-rule-inclusion for every exact request in the request list (ie we make a copy of the default package filter, and add the inclusions there)

Yes that was another thing that crossed my mind, but for some reason I thought adding a new filter would be less disruptive / controversial. Could you clarify this part: If True, and package filter is not explicitly specified in ResolvedContext constructor ?

If I call ResolvedContext(package_filter=...), that's explicit - I don't think the package filter should be changed to allow your exact requests. If it's not, and that new setting is True, I think it's reasonable to adapt the filter as expected.

Benefits:

• No new package filter impl yep
• Backwards compatibility (new setting is False by default) filter would also be backward compatible?
• No chance of leaking into package requirements (which should always obey the filter imo, exact requirement or not) Wouldn't the setting still affect the requirements from a package? If I set the global setting to no filters on exact requests, and a package has it as an exact request, wouldn't it still resolve? I'm not sure I fully grasp the difference?

Ah I should have made clearer, the default_no_filter_on_exact_request setting would apply only to user requests, not package requests. I don't know how to convey that without the setting name getting silly long.

• Explicitness (the filter stored into the rxt file will show that each exact requirement has been explicitly allowed for all versions) fair enough

Thoughts?

This would totally work for me, but just for the sake of conversation, I'm adding a con to this solution:

• You wouldn't be able to do this type of filter: package_filter:
  • excludes:
    • glob(*.beta)
  • excludes:
    • after(1429830188) includes:
      • explicit_requirement() (only allow explicit requirements for the timestamp filter, not the glob one)

It's fine that this isn't supported imo - because exact=nofilter is only supported in user requests, this is not an issue - you've either asked for the exact dev version in your request, or you haven't. It's easy to reason about.

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1255671261, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOUSR4IANF4MHXHR6R733V7TXP3ANCNFSM6AAAAAAQIHW2JQ . You are receiving this because you commented.Message ID: @.***>

[herronelou]

Thanks for the clarifications, it does make more sense.

Would you agree with my original statement that a package being built and running its own tests should never get filtered out and that the current behaviour can be considered a bug?

The solution would then to do what you suggest, as well as setting the new ResolvedContext argument to True when running the pre-build tests?

[JeanChristopheMorinPerso]

I don't consider this a bug. That behavior was present before pre-install was introduced. If you have a dev package in your requires and simply run rez-test, it would be filtered out. It be a bug or not is up for debate I think but that's not important to this discussion. What's important is to focus on what should be the behavior and how we want to tackle that.

[nerdvegas]

JC, I think Erwan is describing the case where the package itself (the one getting its pre-install tests run) is a dev version - not that its requirements are dev versions (which I agree should be filtered out).

In that specific case, I think this is a bug - it surely cannot be right that the very package getting tested is getting filtered out of its own test env.

On Fri, Sep 23, 2022 at 10:42 AM Jean-Christophe Morin < @.***> wrote:

I don't consider this a bug. That behavior was present before pre-install was introduced. If you have a dev package in your requires and simply run rez-test, it would be filtered out. It be a bug or not is up for debate I think but that's not important to this discussion. What's important is to focus on what should be the behavior and how we want to tackle that.

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1255689540, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOUSRL5NHGUVJZDOBL73LV7T4JHANCNFSM6AAAAAAQIHW2JQ . You are receiving this because you commented.Message ID: @.***>

[herronelou]

@JeanChristopheMorinPerso fair enough, I guess what I'm asking is should it always ignore the filter when running on pre-install as it feels odd that you can't even finish the build if the package matches the conditions to be filtered out.

I think what I have in my 3rd post is very specific, and would only add the one package currently being built, while setting no_filter_on_exact_request when running the pre-install tests might allow some others to also get resolved (if there are some other explicit requests for the test env).

Edit: Allan answered while I was typing, but yes that's what I meant.

[JeanChristopheMorinPerso]

I didn't follow the whole discussion, so sorry if I'm missing some details, but couldn't we just special case this instead of adding yet another setting?

[nerdvegas]

We could yes - I brought up the setting because this has come up a few times before (not necessarily in the context of test hooks).

I agree that simply fixing this issue directly is appropriate then. Erwan - any reason we can't just update the filter with a range inclusion specifically for this case?

Thx A

On Fri, Sep 23, 2022 at 10:54 AM Jean-Christophe Morin < @.***> wrote:

I didn't follow the whole discussion, so sorry if I'm missing some details, but couldn't we just special case this instead of adding yet another setting?

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1255695183, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOUSUHFOUPBLWLOK2LK2LV7T5THANCNFSM6AAAAAAQIHW2JQ . You are receiving this because you commented.Message ID: @.***>

[herronelou]

What do you suggest?

I do like the new setting because it also stops me from having add an include every time I want to resolve a dev package, although I guess that's 2 separate issues.

• not able to build at all: critical and can be fixed specifically for that case only
• have the setting: quality of life, could be considered a new feature

On Thu, 22 Sep 2022, 19:59 allan johns, @.***> wrote:

We could yes - I brought up the setting because this has come up a few times before (not necessarily in the context of test hooks).

I agree that simply fixing this issue directly is appropriate then. Erwan - any reason we can't just update the filter with a range inclusion specifically for this case?

Thx A

On Fri, Sep 23, 2022 at 10:54 AM Jean-Christophe Morin < @.***> wrote:

I didn't follow the whole discussion, so sorry if I'm missing some details, but couldn't we just special case this instead of adding yet another setting?

— Reply to this email directly, view it on GitHub < https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1255695183 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAMOUSUHFOUPBLWLOK2LK2LV7T5THANCNFSM6AAAAAAQIHW2JQ

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1255751276, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJAV5BFI64MKSGBMKJGIV3V7UMKZANCNFSM6AAAAAAQIHW2JQ . You are receiving this because you authored the thread.Message ID: @.***>

[nerdvegas]

I would fix specifically for this case - as you say, the setting is a separate issue. A

On Fri, Sep 23, 2022 at 1:33 PM Erwan Leroy @.***> wrote:

What do you suggest?

I do like the new setting because it also stops me from having add an include every time I want to resolve a dev package, although I guess that's 2 separate issues.

• not able to build at all: critical and can be fixed specifically for that case only
• have the setting: quality of life, could be considered a new feature

On Thu, 22 Sep 2022, 19:59 allan johns, @.***> wrote:

We could yes - I brought up the setting because this has come up a few times before (not necessarily in the context of test hooks).

I agree that simply fixing this issue directly is appropriate then. Erwan - any reason we can't just update the filter with a range inclusion specifically for this case?

Thx A

On Fri, Sep 23, 2022 at 10:54 AM Jean-Christophe Morin < @.***> wrote:

I didn't follow the whole discussion, so sorry if I'm missing some details, but couldn't we just special case this instead of adding yet another setting?

— Reply to this email directly, view it on GitHub <

https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1255695183

, or unsubscribe <

https://github.com/notifications/unsubscribe-auth/AAMOUSUHFOUPBLWLOK2LK2LV7T5THANCNFSM6AAAAAAQIHW2JQ

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub < https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1255751276 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAJAV5BFI64MKSGBMKJGIV3V7UMKZANCNFSM6AAAAAAQIHW2JQ

. You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/rez/issues/1374#issuecomment-1255766035, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOUSQXQUW4I4VYOHSGEUTV7UQJBANCNFSM6AAAAAAQIHW2JQ . You are receiving this because you commented.Message ID: @.***>