-
Notifications
You must be signed in to change notification settings - Fork 12
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
Collapse loaded paths to AYON project root env vars #212
base: develop
Are you sure you want to change the base?
Collapse loaded paths to AYON project root env vars #212
Conversation
mapping = {} | ||
for key, value in os.environ.items(): | ||
if key.startswith("AYON_PROJECT_ROOT_"): | ||
mapping[key] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is faster, but we may not need to go through the env keys because the anatomy object already has this piece of information.
I wonder if there's a cached anatomy object instead of creating/querying it.
mapping = {} | |
for key, value in os.environ.items(): | |
if key.startswith("AYON_PROJECT_ROOT_"): | |
mapping[key] = value | |
anatomy = Anatomy(context["project"]["name"], project_entity=context["project"]) | |
mapping = anatomy.root_environments() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly refrained from doing this due to speed reasons - getting the Anatomy object usually results in quite some queries which I hoped to avoid.
Plus, technically - even if the anatomy has it defined.. if it's not in os.environ
it wouldn't actually work in the current session if the anatomy had changed since startup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking it may be better to just 'cache it once' in the current session since that part of os.environ
(for the roots) shouldn't update on context changes regardless. Right @iLLiCiTiT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking it may be better to just 'cache it once' in the current session since that part of os.environ (for the roots) shouldn't update on context changes regardless.
The envs were aded in Pype 2. We didn't have real usage for them since OpenPype, but didn't remove them.
I mostly refrained from doing this due to speed reasons - getting the Anatomy object usually results in quite some queries which I hoped to avoid.
If you pass in project_entity
it does not do any additional queries.
But what if you're loading from library project? The environments are completelly different in that case, or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does having the key in os.environ mean it's actually added to hscript or doesn't it?
Honestly, I don't know because the last item I plated with env vars I was checking if the variables is set in both environ and hscript.
ayon-houdini/client/ayon_houdini/api/lib.py
Line 1013 in 26f8cfb
hou.hscript("set {}={}".format(var, new)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not set to hscript
no, however houdini vars still expand them.
As far as I know, Houdini expands them in order:
- is hscript var? -> expand
- is env var? -> expand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW with Anatomy you could just use this and skip filepath_from_context
.
current_project = get_current_project_name()
project_name = context["project"]["name"]
if project_name == current_project:
anatomy = Anatomy(
project_name, project_entity=context["project"]
)
return get_representation_path(
context["representation"],
anatomy.roots.root_environmets_fill_data()
)
return super().filepath_from_context(context)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that leave the roots in tact? Also, good point on only supporting current project there!
The envs were aded in Pype 2. We didn't have real usage for them since OpenPype, but didn't remove them.
If we do remove them - then it makes sense to explicitly 'enable' them in ayon-houdini
to support this Houdini feature. We can make a PR for that once we need at that point in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that leave the roots in tact? Also, good point on only supporting current project there!
Default behavior of root_environmets_fill_data
is to return env keys as values for root {"work": "$AYON_AYON_PROJECT_ROOT_WORK", ...}
-> which is what you're guessing here, whereas the suggested code does fill it with the root that is really requested by the template.
EDITED: Question is if get_representation_path
is able to handle that? I don't remember if it still does validate path existence?
# Sort by length to ensure that the longest matching key is first | ||
# so that the nearest matching root is used | ||
for key, value in sorted(mapping.items(), | ||
key=lambda x: len(x[1]), | ||
reverse=True): | ||
if value and match_path.startswith(value.replace("\\", "/")): | ||
# Replace start of string with the key | ||
path = f"${key}" + path[len(value):] | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if going the other way is easier to understand and maintain.
I'm assuming the longest match will result in a smaller string (unless the root name is big enough to break this assumption)
Also, we can skip sorting if we the matching is one path.
# Sort by length to ensure that the longest matching key is first | |
# so that the nearest matching root is used | |
for key, value in sorted(mapping.items(), | |
key=lambda x: len(x[1]), | |
reverse=True): | |
if value and match_path.startswith(value.replace("\\", "/")): | |
# Replace start of string with the key | |
path = f"${key}" + path[len(value):] | |
break | |
paths = [] | |
for key, value in mapping.items(): | |
if value and path.startswith(value.replace("\\", "/")): | |
# Replace start of string with the key | |
paths.append(f"${key}{path[len(value):]}") | |
if len(paths) == 1: | |
return paths[0] | |
# sort and return the smallest path. | |
return sorted(paths)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this less because it'd mean we'd always be checking against all roots, even if we had already matched one. So it'd perform less optimal. However, sure - the amount of roots may never grow that large that it'd really be a problem.
Overall I just did a prototype here and did not look at 'cleaning the code' at all - it was merely testing whether this could be functional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the amount of roots may never grow that large that it'd really be a problem.
I agree. but, the code just looks hard to read that's why I tried picking a different approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client/ayon_houdini/plugins/load/load_image.py
is failing because of
ayon-houdini/client/ayon_houdini/plugins/load/load_image.py
Lines 137 to 138 in 26f8cfb
if not os.path.exists(path): | |
raise RuntimeError("Path does not exist: %s" % path) |
Changelog Description
Collapse loaded paths to AYON project root env vars
Additional review information
Fix the loader part of #210
Loading assets should now load the paths using e.g.
$AYON_PROJECT_ROOT_WORK
variable instead of the absolute path making the scene directly cross-OS compatible within AYON.TODO
Testing notes:
$AYON_PROJECT_ROOT_WORK
variable.