mottosso / bleeding-rez

Rez - Reproducible software environments for Windows, Linux and MacOS
GNU Lesser General Public License v3.0
72 stars 10 forks source link

Class-based Packages #22

Open mottosso opened 5 years ago

mottosso commented 5 years ago

Replace the flat package.py with a user-defined class.

pkg.py

from rez import api

class Maya(api.Package):
  name = "maya"
  version = "2019.0.1"
  environ = {
    "MAYA_PLUG_IN_PATH": [],
    "PYTHONPATH": ["{root}/python"]
  }
  metadata = {
    "label": "Autodesk Maya",
    "icon": "{root}/resources/maya_{width}x{width}.png",
    "banner": "{root}/resources/banner_{width}x{height}.png",
  }

  def commands(self):
    if api.platform == "windows":
        api.alias("maya", r"c:\program files\autodesk\maya2018\bin\maya.exe")

# Optional, register in active Python session, no file required.
api.register_package(Maya)

New file format "pkg.py" is used to distinguish between new and old-style package definitions.

Benefits

Cons

-

Parsing

From a user perspective, working with the package.py is akin to working with a YAML file. It's declarative, and carries a series of built-in keywords. But as a Python file, non-standard keywords are unexpected.

From a developer perspective, the package.py file is parsed and converted into a corresponding package object, and that parsing is quite complex.

with open("package.py") as f:
  for line in f:
    ...

Both of these can be solved by sticking with native Python modules and let Python handle parsing.

package = __import__("package.py")
nerdvegas commented 5 years ago

"From a developer perspective, the package.py file is parsed and converted into a corresponding package object, and that parsing is quite complex."

Could you explain what you mean? This is not how rez loads package.py files. It evaluates the file as regular python: https://github.com/nerdvegas/rez/blob/master/src/rez/serialise.py#L206

A class-based definition would be nice, however it would also be a lot of work that doesn't add any new features; furthermore, the existing syntax will still need to be supported, for backwards compatibility reasons.

mottosso commented 5 years ago

Hey Allan, I'm still working out appropriate phrasing and investigations behind each claim before this can be taken word-for-word, stay tuned for this to be posted in the rez project.

nerdvegas commented 5 years ago

Right - but like I said, package definitions aren't parsed out of the python file, they're exec'd like any other python source.

What may have thrown you off course is the SourceCode stuff, where code is indeed getting extracted from the package.py. The reason for this is that package.py files are also created dynamically - for an obvious case of this, you can rez-build a package.yaml-based package, and it will install a package.py. When you install packages, you aren't actually doing that - you're installing a variant into a new/existing package definition in the target repository.

Class-based package definitions won't change any of the above (although if there's a better way to extract source in order to construct the target package definition, then I'm interested - this isn't something I've had the time to look into).

Aside - I just realised that I cannot find this issue in the rez repo, as its on your own. I can't really track issues across multiple repos, so as a general rule I'll only be looking at those in the main repo from here on.

Cheers A

On Tue, Jun 4, 2019 at 4:57 AM Marcus Ottosson notifications@github.com wrote:

Hey Allan, I'm still working out appropriate phrasing and investigations behind each claim before this can be taken word-for-word, stay tuned for this to be posted in the rez project.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mottosso/bleeding-rez/issues/22?email_source=notifications&email_token=AAMOUSXZLOWTQ4KXIYGQXQLPYVSRNA5CNFSM4HPNGKIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW2LN7Q#issuecomment-498382590, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMOUSUGLLDAQJK5NSFTDI3PYVSRNANCNFSM4HPNGKIA .

nerdvegas commented 5 years ago

Just to add to this while it's fresh:

The current process (in a prod env) for loading a package and evaluating its commands into a resolved env is like so:

Now you can probably see from this that one potential improvement would be to avoid the conversion to/from sourcecode via the SourceCode object - instead, the function could be kept intact (one way to do this would be to use https://github.com/uqfoundation/dill to serialise the function into memcached). This could be a good addition, as it would probably be a performance optimization, although we'd have to see what percentage of your average resolve/context source this would save.

What I want to stress is that making the above improvement has nothing to do with whether package definitions are class-based or not. It would be a shame to see an improvement like this made only in the context of a new package definition format, because new formats take a really, really long time to get adopted, and so the performance benefits of commands functions kept intact would, in any practical sense, not actually be seen by any studio using rez.

Bear in mind that I'm raising this point because I'm inferring from your previous comments about parsing the package.py that this is what you're referring to. If it isn't, then carry on as you are.

A

Ps - in case you're wondering, the reason for the SourceCode stuff dates back to when all packages were YAML-based. Py and YAML-based package definitions are still supported alongside one another, and the simplest way to get this to work was to represent their commands functions the same way in both cases. So, it's basically a hangover. New features aren't supported in the old YAML format anymore (there are already a number that are not, eg early/late binding), and at some point it would be good to deprecate these. But even today, studios still have YAML packages, even though they were officially deprecated years ago.

On Tue, Jun 4, 2019 at 10:41 AM Allan Johns nerdvegas@gmail.com wrote:

Right - but like I said, package definitions aren't parsed out of the python file, they're exec'd like any other python source.

What may have thrown you off course is the SourceCode stuff, where code is indeed getting extracted from the package.py. The reason for this is that package.py files are also created dynamically - for an obvious case of this, you can rez-build a package.yaml-based package, and it will install a package.py. When you install packages, you aren't actually doing that - you're installing a variant into a new/existing package definition in the target repository.

Class-based package definitions won't change any of the above (although if there's a better way to extract source in order to construct the target package definition, then I'm interested - this isn't something I've had the time to look into).

Aside - I just realised that I cannot find this issue in the rez repo, as its on your own. I can't really track issues across multiple repos, so as a general rule I'll only be looking at those in the main repo from here on.

Cheers A

On Tue, Jun 4, 2019 at 4:57 AM Marcus Ottosson notifications@github.com wrote:

Hey Allan, I'm still working out appropriate phrasing and investigations behind each claim before this can be taken word-for-word, stay tuned for this to be posted in the rez project.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mottosso/bleeding-rez/issues/22?email_source=notifications&email_token=AAMOUSXZLOWTQ4KXIYGQXQLPYVSRNA5CNFSM4HPNGKIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW2LN7Q#issuecomment-498382590, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMOUSUGLLDAQJK5NSFTDI3PYVSRNANCNFSM4HPNGKIA .

mottosso commented 5 years ago

Thanks, that's very helpful. I'll take this into account when/if the official issue reaches the Rez project.