ynput / ayon-usd

USD Addon for AYON.
Apache License 2.0
4 stars 1 forks source link

Allow For Creation of pinning Files without root_info #39

Closed Lypsolon closed 1 month ago

Lypsolon commented 2 months ago

Is there an existing issue for this?

Please describe the feature you have in mind and explain what the current shortcomings are?

Pinning files should be allowed to be generated even without a root_info dict as you can use Ayon without setting a project root for your site id.

How would you imagine the implementation of the feature?

remove the check for root_info and allow the creation without it.

Are there any labels you wish to add?

Describe alternatives you've considered:

No response

Additional context:

No response

Lypsolon commented 2 months ago

a moment ago @BigRoy mentioned that there is an option to have a siteId without project roots set, this issue debates if this is a intended use case and if it is will remove the root_info check from the pinning file creation to allow for the creation of pinning files that act more like caching files and loos the root replace function.

BigRoy commented 2 months ago

Let me start of by saying that I have no idea how to configure things so that I do have something valid returned for root_info. :) It's just that on my debug server and production scenarios this returns an empty dict:

import ayon_api
import os
root_info = ayon_api.get_project_roots_for_site(os.environ.get("AYON_PROJECT_NAME"))
print(root_info)
# {}

source

Lypsolon commented 2 months ago

@martastain @dee-ynput i assumed that site roots are needed for more than just the resolver? can you comment on that. if the comment is that its not needed we can just remove the check.

antirotor commented 2 months ago

Site roots are needed elsewhere indeed. And btw, ideally it should be:

import ayon_api
from ayon_core.pipeline import get_current_project_name
root_info = ayon_api.get_project_roots_for_site(get_current_project_name())
print(root_info)

(just because I am scared of enviornment variables)

antirotor commented 2 months ago

First - get_project_roots_for_site() is now deprecated - replaced by get_project_root_overrides.

But I think that there is a bug in ayon_api itself. Because to get the roots, you need either site_id or platform. If you don't set one, it will get back 400 error. If you look at the get_project_root_overrides_by_site_id it isn't passing the site_id or the platform to the server at all, it is probably expecting that server will return all sites and ayon_api will just filter the correct one.

https://github.com/ynput/ayon-python-api/blob/dcfb95c8607187abae531b7ae195049ac59a96cb/ayon_api/server_api.py#L3018

BigRoy commented 2 months ago

But I think that there is a bug in ayon_api itself. Because to get the roots, you need either site_id or platform. If you don't set one, it will get back 400 error. If you look at the get_project_root_overrides_by_site_id it isn't passing the site_id or the platform to the server at all, it is probably expecting that server will return all sites and ayon_api will just filter the correct one.

That seems to match the REST api docs though:

image

Anyway, no matter what I do.. it's all an empty dict for me:

import ayon_api
from ayon_core.pipeline import get_current_project_name

project_name = get_current_project_name()
site_id = ayon_api.get_site_id()
root_info = ayon_api.get_project_roots_for_site(project_name, site_id)

print(f"{site_id=}")
print(f"{root_info=}")
print(ayon_api.get(f"projects/{project_name}/roots").text)
print(ayon_api.get(f"projects/{project_name}/roots/{site_id}").text)

Basically results in:

site_id='spiritual-garnet-impala'
root_info={}
{}
{"code":404,"detail":"API endpoint /api/projects/ayontest/roots/spiritual-garnet-impala not found","path":"/api/projects/ayontest/roots/spiritual-garnet-impala"}

The real question is - why does it work for Lyon?

BigRoy commented 2 months ago
print(ayon_api.get(f"projects/{project_name}/siteRoots").text)
# {"work":"C:/projects"}

This does return something. Is that what we'd expect? And again, why did it work for Lyon? 🥹

Lypsolon commented 2 months ago

This does return something. Is that what we'd expect? And again, why did it work for Lyon?

yes this works for me no problem.

Lypsolon commented 2 months ago

okay for the relative path issue: basic testing returned that the relative paths get combined with the layer path to an absolute path.

sooo we need to support rootless paths on both sides key:value and then we are good. i need to check if the resolver already dose this if not i will just run the root replacement functions for both sides and fix the root_generate function on the pinning file branch.

just to keep you updated and for reference.

BigRoy commented 2 months ago

So - I'm trying to make @iLLiCiTiT understand what's going on here. ;) And he's basically saying we're doing it wrong.

  1. We shouldn't be using the the root override api logic - because it doesn't include all the roots (but only the the overrides for the sites and that is why it's empty for me)
    • We SHOULD use ayon_api.get_project or ayon_api.get_projects to the roots for YOUR site. According to @iLLiCiTiT at least. :)
  2. Anatomy.find_root_template_from_path in ayon_core has functionality to do exactly what we're trying to do.

But better:

  1. @iLLiCiTiT says: even better implement it on server backend and add argument to the resolve endpoint that basically returns the rootless path to begin with. Especially because that can then even return it without knowing your site, and do "less" work on the server.

Edge case?

  1. What if you have an entity URI that is of a different project than the current one? We may need the roots for all the projects inside the pinning file, and also store the project_name so we know which site roots to use for that project. Etc.

So e.g. we need to store the rootless paths like {root[project][work]} instead of {root[work]} OR just store some metadata with each mapping in the pinning file:

{
    "project_names": ["ProjectA", "ProjectB"],
    "{some long uri}": {
        "rootless_path": "{root[work]}/bla/bla",
        "project_name": "ayontest",
    }
}

And preferably then store top level all the projects used in this JSON file so we don't need to iterate all the JSON entries to make a set of the present project_name.


Question: Where does the resolver get the Site ID from?

This was a question from @iLLiCiTiT - he was curious where it's taken from when the resolver starts using the pinning file. It could be defined in multiple places.

Lypsolon commented 2 months ago
  1. We shouldn't be using the the root override api logic - because it doesn't include all the roots (but only the the overrides for the sites and that is why it's empty for me)

well we want only the overwrites for the Site your on and not for all sites this is how the resolver works, and yes we currently do not support cross project usage the GH issue is open for a long long while but it was never pushed forward.

also why would we want site roots for other machines? that makes no sens at all. if your resolving to a path that is not a root on your system your resolving something outside of Ayon fouler and that should not be tracked.

https://github.com/ynput/ayon-cpp-api/issues/5

https://github.com/ynput/ayon-cpp-api/blob/2045db0cf3aa6b69ffd85e50891213c44869c3c6/src/AyonCppApi/AyonCppApi.cpp#L92

  • We SHOULD use ayon_api.get_project or ayon_api.get_projects to the roots for YOUR site. According to @iLLiCiTiT at least. :)

sure why not should not really change anything.

  1. Anatomy.find_root_template_from_path in ayon_core has functionality to do exactly what we're trying to do.

okay i guess maybe it dose that? bit hard to guess from the function name. PS: it would be so great if there would be hosted code documentation some where so that we can link against it for reference and for communication.

  1. @iLLiCiTiT says: even better implement it on server backend and add argument to the resolve endpoint that basically returns the rootless path to begin with. Especially because that can then even return it without knowing your site, and do "less" work on the server.

that is the case and it has always been the case. the resolver only gets a rootless path from the server and then puts the path together. this is also why we use this root replace system. there are multiple things we could do here to do what he is saying.

A (nope): we could give the resolver a flag that then returns the rootless path instead of the resolved path. <- i wont do this because its just not the task of the resolver it adds complexity because it probably needs to be a runtime switch it also dosnt really work in all situations eg if you want to construct a stage then you will get the problem that the stage is broken, i guess we could do it so that you can call a function on the resolver and then the resolver will write the rootless paths into a separate map with the keys.

B: we could write a function that acts like the resolver and hits the resolve endpoint and then gives us back the rootless path.

Edge case?

  1. What if you have an entity URI that is of a different project than the current one? We may need the roots for all the projects inside the pinning file, and also store the project_name so we know which site roots to use for that project. Etc.

welp so point 1: currently you dont because the c++ api dose not support it. point 2: yes cross project usage is something to be fixed but its a bigger game than just fixing pinning generation.

So e.g. we need to store the rootless paths like {root[project][work]} instead of {root[work]} OR just store some metadata with each mapping in the pinning file:

jup having a root identifier thats universally ture would be best. i would also like it if that would be the same on the endpoint and on the pinning file.

And preferably then store top level all the projects used in this JSON file so we don't need to iterate all the JSON entries to make a set of the present project_name.

Question: Where does the resolver get the Site ID from?

This was a question from @iLLiCiTiT - he was curious where it's taken from when the resolver starts using the pinning file. It could be defined in multiple places.

here you go. its searching all the places i was able to get at the time (the code is very old btw, last changed 7 months ago) https://github.com/ynput/ayon-cpp-api/blob/2045db0cf3aa6b69ffd85e50891213c44869c3c6/src/AyonCppApi/AyonCppApi.cpp#L92

  • (and good point made by him) it may also need to generate a site id, if the local machine does not have it yet.

why dose it need to generate a site id? just because? the only thing we need it for is to get the site root. and generating the site id dose not automatically set the root. soooo yeah any reason for that?

iLLiCiTiT commented 2 months ago

We SHOULD use ayon_api.get_project or ayon_api.get_projects to the roots for YOUR site. According to @iLLiCiTiT at least. :)

If you pass header x-ayon-site-id={site id} then the roots in project have already site specific roots.

why dose it need to generate a site id? just because? the only thing we need it for is to get the site root. and generating the site id dose not automatically set the root. soooo yeah any reason for that?

So TD can potentially change the roots from AYON web ui without having direct access to the machine. Maybe a feature/enhancement, not "must have".

NOTE: We've discussed the changes related to how we understood what is this issue about. Maybe we didn't get what is the goal? I still don't fully understand what should happen, and where (and why).

Lypsolon commented 1 month ago

If you pass header x-ayon-site-id={site id} then the roots in project have already site specific roots.

im gonna go and guess: your talking about the /resolve endpoint on the server? if so yes doing this will return a full path for your machine but martin mentioned a good while back that this is slow (the cpp api used to use x-ayon-site-id with the /resolve endpoint) and so we decided it would be best to emulate the behavior in the cpp api by getting the site roots at instance construction and the rootless path from the server and combined them back to a full path.

So TD can potentially change the roots from AYON web ui without having direct access to the machine. Maybe a feature/enhancement, not must have.

im not following is this connected to pinning file creation? why would a TD want to change a site root remotely for a machine that generating a pinning file? or do you mean it in a generally seance? aka: every machine should have a site id? that i could understand but i dont think that should be handled by pinning creation?

NOTE: We've discussed the changes related to how we understood what is this issue about. Maybe we didn't get what is the goal? I still don't fully understand what should happen, and where (and why).

who is WE if i might ask just for context?

sooo: ähhh: the beginning of the issue was that @BigRoy wanted to create a pinning file on a machine where no Project roots where available. i belive this has changed by now to: how do we get the site roots because apparently some things work for some and not for others (but im a bit lost tbh too).

reason why we need the site roots: the resolver gets the site roots so we dont need the server resolve endpoint to generate machine specific paths, this proved to be faster.

now we want to generate a pinning file. this will be a key:value pair the key is the assetIdent that the resolver gets(not important) and the value needs to be the resolver path without the site root so that we can generate a machine path on the machine that is rendering the scene, for this we set the site roots as an env variable on the rendering machine and put the paths back together with the same system we use to do this task when hitting the resolve endpoint.

we cant do cross project resolution with the resolver because the c++ api is getting its site roots at construction (this should be changed in the future but is not started) there is also the question on how we know to what project a given uri belongs because the return from the /resolve endpoint dose not give us this info if we use the path only flag (we do this to reduce the size of the return as its probable that we need to communicate 100k+ times so every byte we can shave of reduces the network latency)

i have no idea if that really answered your question but i hope it gave an idea ?

iLLiCiTiT commented 1 month ago

Use /projects/{project name} with having x-ayon-site-id in headers. With that the received project entity will have roots with already applied site overrides. You don't have to do anything else.

Lypsolon commented 1 month ago

Use /projects/{project name} with having x-ayon-site-id in headers. With that the received project entity will have roots with already applied site overrides. You don't have to do anything else.

I mentioned that we actively decided against doing that for performance reasons in the resolver. i also mentioned that this is not what we want for a pinning file because the pinning file needs to be portable.

(this portion i forgot to mention in the message atop, i assumed roy informed you if your missing context let me know) when a pinning file is used the resolver dose not communicate with the server this is done to freeze the Usd stage in time and to avoid overloading the server.

so no for multiple reasons we cant just get the machine local path from the server.

iLLiCiTiT commented 1 month ago

I think he didn't understand either? I'll let you chat, because I'm more and more confused about "what is this issue about".

BigRoy commented 1 month ago

that is the case and it has always been the case. the resolver only gets a rootless path from the server and then puts the path together. this is also why we use this root replace system. there are multiple things we could do here to do what he is saying.

A (nope): we could give the resolver a flag that then returns the rootless path instead of the resolved path. <- i wont do this because its just not the task of the resolver it adds complexity because it probably needs to be a runtime switch it also dosnt really work in all situations eg if you want to construct a stage then you will get the problem that the stage is broken, i guess we could do it so that you can call a function on the resolver and then the resolver will write the rootless paths into a separate map with the keys.

Sorry - what? No.

The resolver gets the resolved path - it doesn't need to do anything with sites or whatever, the backend does that already for you.

However, the way I understood with the file pinning is that you wanted to write the pinning file with rootless paths. Hence, my point was - can't we make that then resolve to the rootless path from the backend directly?

This is about the generation of the pinning file - it's not about the resolver itself at all.

So, to generate the pinning file -> use the API resolve entry point to resolve to a rootless path. Easy as that.

BigRoy commented 1 month ago

sooo: ähhh: the beginning of the issue was that @BigRoy wanted to create a pinning file on a machine where no Project roots where available. i belive this has changed by now to: how do we get the site roots because apparently some things work for some and not for others (but im a bit lost tbh too).

Also, no, the beginning of this issue is not about me wanting to do that. It's about the examples code provided that I should use returns no roots for me because I have no root overrides on my site. I just have the configured default roots in studio settings, hence "root overrides' are {} as I understood after the call with @iLLiCiTiT

In simple terms:

  1. AYON has studio roots
  2. A project can override these roots
  3. A site can override these roots

I only have 1. and hence have no overrides.

Lypsolon commented 1 month ago

Sorry - what? No.

The resolver gets the resolved path - it doesn't need to do anything with sites or whatever, the backend does that already for you.

okay im gonna say it again. yes the backed can do that. but like i already mentioned we dont use that system (for performance reasons). the reason why we dont use that system is as follows.

the resolver is a lot faster in generating a machine local path than the server is because the server has to ask his database the resolver only needs to get the site roots once at the beginning.

i know that its doing this because i wrote the code, i can put a std::cout into the code and show you that the resolver(or more the c++ api) dose infarct request the rootless path from the server and then puts the correct path together in the c++ code. further more its actually using an extra argument with his request to ensure that we only get back the path to save on network bandwidth. pathOnly sadly the documentation dosnt mention what happens if you dont give the server the side id and why this can be helpful.

this is the endpoint we hit btw "/api/projects/" + ayonProjectName + "/siteRoots" https://github.com/ynput/ayon-cpp-api/blob/2045db0cf3aa6b69ffd85e50891213c44869c3c6/src/AyonCppApi/AyonCppApi.cpp#L119

here is the top layer code for getting talking with the endpoint. and the specific line that deactivates the root resolution nlohmann::json jsonPayload = {{"resolveRoots", false}, {"uris", nlohmann::json::array({uriPath})}}; https://github.com/ynput/ayon-cpp-api/blob/2045db0cf3aa6b69ffd85e50891213c44869c3c6/src/AyonCppApi/AyonCppApi.cpp#L284

here is the function that replaces the roots: https://github.com/ynput/ayon-cpp-api/blob/2045db0cf3aa6b69ffd85e50891213c44869c3c6/src/AyonCppApi/AyonCppApi.cpp#L138

it even has a logging key so you can expose this env variable AYON_LOGGIN_LOGGIN_KEYS="AyonApi/" to see the rootless path its getting and the machine path its returning.

to be fair: we currently send the side id to the server because i did not remove it, we dont need to do that anymore and here is an issue to remove it. https://github.com/ynput/ayon-cpp-api/issues/19

However, the way I understood with the file pinning is that you wanted to write the pinning file with rootless paths. Hence, my point was - can't we make that then resolve to the rootless path from the backend directly?

yes we can do that we just simply didn't because it was not an easier way. here is the thing, we do need to actually resolve all the paths because we need to open the other Usd Layers, aka all the sub-layers and references and so on because we need all paths needed for Stage construction not just the few in the first layer. with that in mind i decided that its simpler to first get all the abs paths and then replace the root of them. it was a simple question of whats simpler to implement.

yes it would be possible to A: do dubble resolution aka; first resolve the actual usd path so that we can open the depended usd file and then ask the resolve endpoint for the rootless path. B: implement a function in the resolver that returns the rootlets path, i did not do that because its overkill and simply not needed.

This is about the generation of the pinning file - it's not about the resolver itself at all. So, to generate the pinning file -> use the API resolve entry point to resolve to a rootless path. Easy as that.

resolve a rootless path? you mean resolve an uri to an rootless path right? because resoling the rootless path dosnt help us.

also your ideas arent covering that you can reference a usd file that is managed by ayon without an uri. first gathering all the paths and then making them rootless dose cover this case.

Lypsolon commented 1 month ago

Also, no, the beginning of this issue is not about me wanting to do that. It's about the examples code provided that I should use returns no roots for me because I have no root overrides on my site. I just have the configured default roots in studio settings, hence "root overrides' are {} as I understood after the call with @iLLiCiTiT

In simple terms:

  1. AYON has studio roots
  2. A project can override these roots
  3. A site can override these roots

I only have 1. and hence have no overrides.

ahh good to know i didnt know it was about the example code i assumed because of the name of the issue(named after what i understood in the catch-up meeting) you want to create pinning file without root_info because you dont have root info.

Lypsolon commented 1 month ago

okay the REST API endpoint /api/projects/{prj_name}/siteRoots dose return the default site root if no overwrite is set.

sooo the c++ site is working and its time to figure out what to put into the example.

@iLLiCiTiT am i right assuming that ayon_api.get_project_roots_for_site is not hitting the endpoint atop of the message ?

i would have assumed that thees to would be the same. (tested via interactive ayon terminal)

>>>print(requests.get("https://ayon_dev.open-planck.de/api/projects/AY01_VFX_demo/siteRoots", headers={"x-ayon-site-id": "groovy-amiable-reindeer", "Authorization": "Bearer ffa5d1bad0cb32bae4d983ba267d678ac8c1e2c0a407e57274152ca9be0df9b5"}).text)
>>>{"work":"/mnt/share/projects"}
>>> print(ayon_api.get_project_roots_for_site("AY01_VFX_demo"))
>>>{}
iLLiCiTiT commented 1 month ago

@iLLiCiTiT am i right assuming that ayon_api.get_project_roots_for_site is not hitting the endpoint atop of the message ?

No, as mentioned above, that method is deprecated. It was replaced with get_project_root_overrides_by_site_id.

There are 2 methods using that endpoint get_project_roots_by_site_id and get_project_roots_by_platform, but we don't use them because we use get_project (/projects/{project name}) to get roots, as most of the time we also need other data from project.

Do you need python implementation or C++ implementation? Because if you need python implementation in pipeline part, then there are better ways how to get the roots.

Lypsolon commented 1 month ago

No, as mentioned above, that method is deprecated. It was replaced with get_project_root_overrides_by_site_id.

ahhhhh many thanks now i understand what is replaced by what.

There are 2 methods using that endpoint get_project_roots_by_site_id and get_project_roots_by_platform, but we don't use them because we use get_project (/projects/{project name}) to get roots, as most of the time we also need other data from project.

okay now i get the idea with using project template (what roy mentioned)

Do you need python implementation or C++ implementation? Because if you need python implementation in pipeline part, then there are better ways how to get the roots.

c++ site works no problem. we need a better python example for this document i assumed that the other function would work but was not aware that its deprecated (do we have an option to log a warning when a deprecated function is used from inside the function, like a cpp compiler warning? just out of interest) .

iLLiCiTiT commented 1 month ago

we need a better python example for this document i assumed that the other function would work but was not aware that its deprecated (do we have an option to log a warning when a deprecated function is used from inside the function, like a cpp compiler warning? just out of interest) .

In develop it is warned. Not sure if last release already did.

c++ site works no problem.

I'm asking because if you're in pipeline (e.g. during publishing) you already have multiple ways how to access roots without need to call anything in ayon_api. So where it's used does matter. When you're in pipeline part, you should be using Anatomy.

Lypsolon commented 1 month ago

I'm asking because if you're in pipeline (e.g. during publishing) you already have multiple ways how to access roots without need to call anything in ayon_api. So where it's used does matter. When you're in pipeline part, you should be using Anatomy.

yeah good point maybe we should do multiple examples then. one for ayon_api "raw dog it" one for your in publishing ?

BigRoy commented 1 month ago

yeah good point maybe we should do multiple examples then. one for ayon_api "raw dog it" one for your in publishing ?

Yes, sort of. It'd even be nice if potentially the generate pinning file function allows root_info=None (like it's optional) where it defaults to your site's root info to begin with.

And of course the example, can be in the docstring ;)

Anyway, I'm still not entirely sure now how to proceed with testing the generate pinning file PR itself hehe.

Lypsolon commented 1 month ago

Yes, sort of. It'd even be nice if potentially the generate pinning file function allows root_info=None (like it's optional) where it defaults to your site's root info to begin with.

hmmm i dont want ayon imports in standalone tools as they should be able to run standalone so having it optional and having a system that auto gets them is not the way.

i even target testing standalone tools without ayon at all, makes testing a lot easyer and the addition is just to small to argument the extra complexity that we will have on the CI side.

And of course the example, can be in the docstring ;)

when we have a good example and the automatic documentation like the cpp tools we could even link to the examples page.

Anyway, I'm still not entirely sure now how to proceed with testing the generate pinning file PR itself hehe.

well for testing i think this is good enough. its what the cpp api dose so its the same in the end. from what i get of this discussion, the data is right just the way we access it isn't so its a question of building a better example.

print(ayon_api.get(f"projects/{project_name}/siteRoots").text)
# {"work":"C:/projects"}
BigRoy commented 1 month ago

This can be closed - the issue here was mostly due to the roots used during the test run as returned from the AYON API was just the wrong result because using the wrong API function for this.

The correct API call seems to be:

import ayon_api
from ayon_core.pipeline import get_current_project_name

root_info = ayon_api.get_project_roots_by_site_id(get_current_project_name())

This can also be seen in how it's used in the Integrate Pinning File plugin: https://github.com/ynput/ayon-usd/blob/f927cf4f5abe7164228086750b127528dca477ec/client/ayon_usd/plugins/publish/integrate_pinning_file.py#L79-L82