Support "additive" preprocess functions

[JeanChristopheMorinPerso]

Opening an issue regarding #597 (comment) and a comment that followed (#597 (comment)).

Right now, as things are, it is only possible to use one single preprocess function. Either in the rezconfig.py file or in a package.py file. It's one or the other. This is pretty limiting. Local preprocessor is super useful (by local I mean in package.py) and global (in rezconfig.py) is also super useful.

I propose to add the ability to have what I call additive preprocessors. That means adding the ability to define both a global and a local preprocessor and that both are executed.

@nerdvegas goes one step further and propose to add a new preprocess_mode package attribute. Possible values are before and after. If preprocess_mode is defined, both the global and local will be sexcuted. If not, the default (current) behavior is used (so if a global is set but no local, the global will be used. If a local is set (with or withut global), the local wins).

I would push the idea even further and add a way to force both (global and locall) to always be executed, and have the ability to disable the global per package by setting preprocess_mode to None, a little bit like build_command. So preprocess_mode could be defined in rezconfig.py and have a default value set by a studio. The idea is that there will probably be some studios that will want to enfore a preprocess on 95% of their packages, and so to remove some technical details, having the mode defined at a facility level will make things simpler and better for some teams. And for the 5% package left, the package author would be able to set preprocess_mode to None so that the global is not executed.

Let me know what you think. If everybody agrees, I'll work on that.

[nerdvegas]

I don't think it's very intuitive to disable global preprocessing by setting `preprocess_mode = None`. In general it's good to avoid having to set anything to None to select behaviour; for build_command, it's a rare case that does make sense however. I see another issue too. Neither approach has taken into consideration that a package may wish to do local preprocessing before AND after the global preprocessor. I can imagine legitimate cases for doing this. So then, I'm thinking that exposing the global preprocessor as a function that you can call in your own preprocessor makes the most sense. It's backwards compatible, and it gives the most flexibility: # in package.py def preprocess(package, data): do_some_before_stuff() global_preprocess(package, data) do_some_after_stuff() Also, the problem with a global setting to control preprocess mode, is that an author of a package may have assumed some default order (let's say local first, then global); but then, someone goes and changes the global mode. Now the result may be invalid. This would in effect cause the chosen global mode to be set in stone - nobody could change it, for fear of breaking any number of hundreds of packages already in circulation. A

…

On Tue, Apr 23, 2019 at 1:02 AM Jean-Christophe Morin < ***@***.***> wrote: Opening an issue regarding #597 (comment) <#597 (comment)> and a comment that followed (#597 (comment) <#597 (comment)>). Right now, as things are, it is only possible to use one single preprocess function. Either in the rezconfig.py file or in a package.py file. It's one or the other. This is pretty limiting. Local preprocessor is super useful (by local I mean in package.py) and global (in rezconfig.py) is also super useful. I propose to add the ability to have what I call additive preprocessors. That means adding the ability to define both a global and a local preprocessor and that both are executed. @nerdvegas <https://github.com/nerdvegas> goes one step further and propose to add a new preprocess_mode package attribute. Possible values are before and after. If preprocess_mode is defined, both the global and local will be sexcuted. If not, the default (current) behavior is used (so if a global is set but no local, the global will be used. If a local is set (with or withut global), the local wins). I would push the idea even further and add a way to force both (global and locall) to always be executed, and have the ability to disable the global per package by setting preprocess_mode to None, a little bit like build_command. So preprocess_mode could be defined in rezconfig.py and have a default value set by a studio. The idea is that there will probably be some studios that will want to enfore a preprocess on 95% of their packages, and so to remove some technical details, having the mode defined at a facility level will make things simpler and better for some teams. And for the 5% package left, the package author would be able to set preprocess_mode to None so that the global is not executed. Let me know what you think. If everybody agrees, I'll work on that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#609>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSVOE2JAYAWZVRKZSX3PRXHPLANCNFSM4HHQJHHA> .

[JeanChristopheMorinPerso]

I don't think it's very intuitive to disable global preprocessing by
setting preprocess_mode = None. In general it's good to avoid having to
set anything to None to select behaviour; for build_command, it's a rare
case that does make sense however.

I have to agree it's not the most intuitive approach.

I see another issue too. Neither approach has taken into consideration that
a package may wish to do local preprocessing before AND after the global
preprocessor. I can imagine legitimate cases for doing this.

ok.. That's a weird case and not sure someone would want to do that. But it's something we can support I guess.

So then, I'm thinking that exposing the global preprocessor as a function
that you can call in your own preprocessor makes the most sense. It's
backwards compatible, and it gives the most flexibility:

That is an option indeed. But it kind of defeats the purpose of the global preprocessor IMO. To me a global preprocess is something that is applied all the time, unless deactivated (see what I say bellow). Though it remains a valid thing and could be something to add. I clearly see some cases where before and `after are not enough and that someone might want to call the global preprocessor at a really specific time.

Also, the problem with a global setting to control preprocess mode, is that
an author of a package may have assumed some default order (let's say local
first, then global); but then, someone goes and changes the global mode.
Now the result may be invalid. This would in effect cause the chosen global
mode to be set in stone - nobody could change it, for fear of breaking any
number of hundreds of packages already in circulation.

That's definitely a valid point. The way I see things (and that's how we do it at RodeoFX) is that everything that affects our package build/release only can be changed without fear. It's not like it would break the production. Of course if someone don't want to set a default, he will be free to do it. To me it's a documentation issue more than anything else. We could easily add a warning in the rez documentation about what the side effect could be of setting a default mode.

My main use case for having both a global preprocessor and a local one is to have a way to enforce a naming convention for our packages and also to validate that the version of package is correctly set and that it fits with our conventions. So the global preprocessor wouldn't affect the package data for us at RodeoFX. We would use it only to raise an exception and stop someone from releasing something that would break some stuff because they didn't respect some of our rules.

Do you know any other use cases for the global preprocessor? I can't think or find other use cases... Maybe inject/store some custom package attributes common to all packages/studio like some kind of metadata that a studio would use for some things?

Now to come back to preprocess_mode, maybe something like global_preprocess_run instead of preprocess_mode that could accept after, before, both and False? Or maybe disable to keep the action style of the value.the value an action.

I'm open to any of your ideas and suggestions. I'd like to see if we can come up with an elegant and simple solution. And that's basically why I made this issue, to start a discussion :)

[nerdvegas]

Ok, I've had a think. I think perhaps being able to pre AND post the global preprocessor is overkill, so let's forget that. In that case then, the most "rezothonic"(?) way of doing things would be like so: * there is a global 'package_preprocess_mode' setting, with options ("before", "after", "override"). Override (default) matches current behavior. * if a package wants to change the mode, it does so like any other package config override: with scope("config") as c: c.package_preprocess_mode = "after" A ps - I'm also thinking about supporting a new syntax, so we can remove the need for the "with scope('config')" line. It should be possible to expose a 'config' object during pkg defn eval, so instead you could just go: config.package_preprocess_mode = "after" This is fairly low pri as it doesn't add any new functionality, but it'd be nice.

…

On Tue, Apr 23, 2019 at 12:30 PM Jean-Christophe Morin < ***@***.***> wrote: I don't think it's very intuitive to disable global preprocessing by setting preprocess_mode = None. In general it's good to avoid having to set anything to None to select behaviour; for build_command, it's a rare case that does make sense however. I have to agree it's not the most intuitive approach. I see another issue too. Neither approach has taken into consideration that a package may wish to do local preprocessing before AND after the global preprocessor. I can imagine legitimate cases for doing this. ok.. That's a weird case and not sure someone would want to do that. But it's something we can support I guess. So then, I'm thinking that exposing the global preprocessor as a function that you can call in your own preprocessor makes the most sense. It's backwards compatible, and it gives the most flexibility: That is an option indeed. But it kind of defeats the purpose of the global preprocessor IMO. To me a global preprocess is something that is applied all the time, unless deactivated (see what I say bellow). Though it remains a valid thing and could be something to add. I clearly see some cases where before and `after are not enough and that someone might want to call the global preprocessor at a really specific time. Also, the problem with a global setting to control preprocess mode, is that an author of a package may have assumed some default order (let's say local first, then global); but then, someone goes and changes the global mode. Now the result may be invalid. This would in effect cause the chosen global mode to be set in stone - nobody could change it, for fear of breaking any number of hundreds of packages already in circulation. That's definitely a valid point. The way I see things (and that's how we do it at RodeoFX) is that everything that affects our package build/release only can be changed without fear. It's not like it would break the production. Of course if someone don't want to set a default, he will be free to do it. To me it's a documentation issue more than anything else. We could easily add a warning in the rez documentation about what the side effect could be of setting a default mode. My main use case for having both a global preprocessor and a local one is to have a way to enforce a naming convention for our packages and also to validate that the version of package is correctly set and that it fits with our conventions. So the global preprocessor wouldn't affect the package data for us at RodeoFX. We would use it only to raise an exception and stop someone from releasing something that would break some stuff because they didn't respect some of our rules. Do you know any other use cases for the global preprocessor? I can't think or find other use cases... Maybe inject/store some custom package attributes common to all packages/studio like some kind of metadata that a studio would use for some things? Now to come back to preprocess_mode, maybe something like global_preprocess_run instead of preprocess_mode that could accept after, before, both and False? Or maybe disable to keep the action style of the value.the value an action. I'm open to any of your ideas and suggestions. I'd like to see if we can come up with an elegant and simple solution. And that's basically why I made this issue, to start a discussion :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#609 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSWLXHEP666PKYDWLLLPRZYEZANCNFSM4HHQJHHA> .

[JeanChristopheMorinPerso]

I'm not so used to the with scope (as I never had to use it before). Would that only work from inside the preprocess function?

[nerdvegas]

It's not anything to do with the preprocess function, it's just how you override settings within a package: https://github.com/nerdvegas/rez/wiki/Configuring-Rez#package-overrides A

…

On Sun, Apr 28, 2019 at 2:52 AM Jean-Christophe Morin < ***@***.***> wrote: I'm not so used to the with scope (as I never had to use it before). Would that only work from inside the preprocess function? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#609 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSSEHULTZXH4A4VHT2TPSSAEFANCNFSM4HHQJHHA> .

[nerdvegas]

What's latest on this PR, does it conform to my suggested approach as per above? It needs to as this is most in line with how other package-related options work. There also needs to be tests if there isn't already.

To see how to go about this, look at other package-overridable settings, such as release_packages_path.

Thx
A

ps - Sorry to be picking on this PR :) But in order to be able to get through rez work in general, I need to put more emphasis on contributors to do the work, otherwise I'm too much of a bottleneck. Wrt to recent conversations, I'm interested in getting dev pace increased.

[JeanChristopheMorinPerso]

Hi @nerdvegas , I haven't worked on this yet but it's still on my radar (I got pulled in on some stuff in Pipenv and PyCon US stuff and got a lot of work too at RodeoFX). I'll go with what we agreed with (so pretty much #609 (comment)). I should start working on this next week or the week after. It shouldn't be to hard to implement.

As for the PR, I guess you are talking about #597 ? (which add supports for defining a function for the package_preprocess_function straight in the rezconfig.py). I think Marcus PR could go first if he adds tests to it and I can add the additive behavior in a second PR. Or if Marcus doesn't have time to write tests on his PR, I can work on the additive behavior and include his changes in my changes and I'll take care of writing the tests.

I need to put more emphasis on contributors to do the work, otherwise I'm too much of a bottleneck.

I totally agree and it's entirely fine to put more emphasis on contributors. At the end, the more we are and the better it will be.

Wrt to recent conversations, I'm interested in getting dev pace increased.

And I'm also interested into getting involved into this movement and increasing the pace of my contributions (which have been quite light in the upstream rez but more involved in our fork at RodeoFX).

Great times ahead :)

[nerdvegas]

Hi JC, Yes apologies I got these two related-but-different PRs mixed up, we started talking about the additive aspect of it in that PR thread.

I agree with your suggested approach, can you run point with Marcus RE the tests? Also will need to make sure that in-package.py preprocess, when behaving in before/after mode, works regardless of whether the global preprocess is an in-place function, or a string.

[nerdvegas]

Mentioning #597 pls see above, JC/Marcus need to decide if the tests/other issues raised in that PR are to be dealt with there, or if JC wants to combine with this PR and do that here.

[JeanChristopheMorinPerso]

So I've take over Marcus's work in #650. #597 can be closed. Once #650 is merged, I'll push another PR to get the additive behavior.

[JeanChristopheMorinPerso]

Released in 2.36.0! See https://github.com/nerdvegas/rez/releases/tag/2.36.0.