Remove config.py

[iLLiCiTiT]

Issue

I see the intention of config.py content, but it complicates things a lot. (All my comments in https://github.com/ynput/ayon-usd/pull/21 were ignored?)

First of all you as an client addon probably should not call settings getter via ayon_api as we have function in ayon_core.settings for that which caches the value, and resolves which variant of settings should be used -> which is wrong there, because it uses bundle name, that works only in for bundles. Also most of places where settings are needed already have precached settings (in prelaunch hooks, in publishing, in create context etc.).

• Don't cache get or cache settings on your own if there is a way. Right now production and staging settings don't work.

Secondly the constants to store keys to get settings are uber confusing, it is actually pretty hard to see where settings are used. Please get the settings at places where are used, instead of having those helpers.

• Places that are using the settings should get the settings.

It looks like there are some constants that are only once (might be wrong), but are based on settings, that probably should not happen either if the value can change during process lifetime. There are artist that have runnin AYON launcher for days, that should not be hard cached.

• Don't hard cache values that can change. Example: USD_ZIP_PATH is set on import, you should not do any processing on import, and you should follow changes of settings over time.

Why settings models have _settings suffix or ayon_ prefix in attribute name? It is obvious those are ayon settings you don't need to mark them that specifically in attribute name.

• More reasonable attribute names in settings ayon_usd_resolver_settings -> resolver, lakefs_settings > lakefs.

---

Code change example

@SingletonFuncCache.func_io_cache
def get_global_lake_instance():
    addon_settings = (
        get_addon_settings()
    )  # the function is cached, but this reduces the call stack
    return wrapper.LakeCtl(
        server_url=str(
            get_addon_settings_value(addon_settings, ADDON_SETTINGS_LAKE_FS_URI)
        ),
        access_key_id=str(
            get_addon_settings_value(addon_settings, ADDON_SETTINGS_LAKE_FS_KEY_ID)
        ),
        secret_access_key=str(
            get_addon_settings_value(addon_settings, ADDON_SETTINGS_LAKE_FS_KEY)
        ),
    )

Should be just

from ayon_core.settings import get_studio_settings

class _LocalCache:
    lake_instance = None

def get_global_lake_instance(settings=None):
    """Create lakefs connection.

    Warning:
        This returns singleton object which uses connection information
            available on first call.

    Args:
        settings (Optional[Dict[str, Any]]): Prepared studio or
            project settings.

    Returns:
        wrapper.LakeCtl: Connection object.

    """
    if _LocalCache.lake_instance is not None:
        return _LocalCache.lake_instance

    if not settings:
        settings = get_studio_settings()
    lakefs_settings = settings["ayon_usd"]["lakefs_settings"]
    return wrapper.LakeCtl(
        server_url=lakefs_settings["ayon_usd_lake_fs_server_uri"],
        access_key_id=lakefs_settings["access_key_id"],
        secret_access_key=lakefs_settings["secret_access_key"],
    )

Only with this change the content of the file is almost half size (without docstrings). Btw I would remove global from the name. If LakeCtl should be singleton we might have few issues.

Small annoying questions

If LakeFs should be used more across the addons, shouldn't the logic to connect/get files, fill connection settings etc. be in specific LakeFS addon?

BTW LakeCtl in bin repository does set os.environ so if I would create 2 instances with different connection information they would fight each other? Does it have to have set the env variables?

[Lypsolon]

I see the intention of config.py content, but it complicates things a lot. (All my comments in #21 were ignored?)

nope they where not ignored but they stayed open and i asked questions to all of them that where not answered and the decision was made to move as much as possible to Issues and then Merge and see what happens.

and now we are here and this Issue is actually very informative.

First of all you as an client addon probably should not call settings getter via ayon_api as we have function in ayon_core.settings for that which caches the value, and resolves which variant of settings should be used -> which is wrong there, because it uses bundle name, that works only in for bundles. Also most of places where settings are needed already have precached settings (in prelaunch hooks, in publishing, in create context etc.).

very well i shall move over to core-settings system. i assume there are no documentations for that system other than inline ?

i see that Addon.initialize() has a settings input. tray_init dosn't have that? would it be a valid idea to have the "UsdLib" download in initialize()

regarding this i assume tray init is not the right place in the first place ?

def tray_init(self):

"""Initialization part of tray implementation.

Triggered between `initialization` and `connect_with_addons`.

This is where GUIs should be loaded or tray specific parts should be
prepared.
"""

i also remember you talking about not importing QT things at the top. is there a way to know if we have QT available? or even better if we run Headless ? would be great to just not start the Download UI if we run Headless.

• Don't cache get or cache settings on your own if there is a way. Right now production and staging settings don't work.

thanks for providing this info this exact case actually happend today so i can stop debugging for it.

Secondly the constants to store keys to get settings are uber confusing, it is actually pretty hard to see where settings are used. Please get the settings at places where are used, instead of having those helpers.

• Places that are using the settings should get the settings.

i can get behind the idea of accessing settings in a different way but direct [] access to the settings dict is not the way to go i believe. having a central place that defines what portion of the settings dict should be accessed to gain a specific settings value is way better in my eyes as it allows us to change the access keys over time without having to rewrite this at multiple places.

would a DataClass that takes the "general" settings dict as an input and then provides getters or member variables to access those parts be easier to understand in your eyes ?

It looks like there are some constants that are only once (might be wrong), but are based on settings, that probably should not happen either if the value can change during process lifetime. There are artist that have runnin AYON launcher for days, that should not be hard cached.

following the settings changes can be done. it was avoided to keep the system from changing at "runtime" or between tasks as the assumption was that artist would not like this but if it is the preferred way this can be done.

• Don't hard cache values that can change. Example: USD_ZIP_PATH is set on import, you should not do any processing on import, and you should follow changes of settings over time.

i get the general sentiment of settings changes over time if that is the prefered solution.

but specifically for USD zip path. this can only change when the addon changes its version or its position we could have it in a simple function to stay dynamic but it would still be interesting to here how data that is connected to the location of the addon should be handled

Why settings models have _settings suffix or ayon_ prefix in attribute name? It is obvious those are ayon settings you don't need to mark them that specifically in attribute name.

• More reasonable attribute names in settings ayon_usd_resolver_settings -> resolver, lakefs_settings > lakefs.

they where named this way to make it easy to search for them in the code base.

Code Changes you Showed

i generally agree that making the settings access a bit more simple would be a fair idea. but i dont think we should actually use the direct dict[key] access to do soo. i believe a data class that takes the settings as an input and exposes everything you need in the addon would be a way that allows to keep changes to data access in an central location. (for changes in key names for example. it just keeps us from forgetting to update all the keys if we change something and that keeps bugs from being created)

Only with this change the content of the file is almost half size (without docstrings). Btw I would remove global from the name. If LakeCtl should be singleton we might have few issues.

i think the global makes it simpler to know where it comes from and what the intend is. yes it could be a global but i argue that currently this is "okay" and should be improved when a decision on how we want to handle lakeFs in the future is made. i believe currently we should see this code as temporary.

Small annoying questions

If LakeFs should be used more across the addons, shouldn't the logic to connect/get files, fill connection settings etc. be in specific LakeFS addon?

yes it should be. and i would wish it would be but at the beginning there was a big debate if we should even have it separated into its own library and it was quite a back and forth if we should or not and now this is the outcome from that.

BTW LakeCtl in bin repository does set os.environ so if I would create 2 instances with different connection information they would fight each other? Does it have to have set the env variables?

it dose need those env variables but we should / could think about setting them in a separate sub process. kinda having a sub process being setup then setting the env variables there. or we find the time to do the rewrite to the raw Python implementation and just dont have this problem anymore. what i believe is the current plan.

it was implemented this way because it was fast and having a "wrapper" means that we can re implement without having to change any depending code.

[iLLiCiTiT]

nope they where not ignored but they stayed open and i asked questions to all of them that where not answered and the decision was made to move as much as possible to Issues and then Merge and see what happens.

I also wrote to ask me directly in DM if you're not sure how to handle it. If you need help as soon as possible then please write DM... Resolve comment "as nothing was written" is not the way.

i also remember you talking about not importing QT things at the top. is there a way to know if we have QT available? or even better if we run Headless ? would be great to just not start the Download UI if we run Headless.

This is one of examples that is hard to explain in plain text, I could write a book about it, that would not explain anything, or have 10 minut call where I would show you.

i can get behind the idea of accessing settings in a different way but direct [] access to the settings dict is not the way to go i believe.

It is the way to go. It is the easiest and most readable way how to get settings. Does not require 3 additional functions and constants that developer has to go through to understand WTF is happening there. Also current approach does not allow to reuse already fetched settings.

would a DataClass that takes the "general" settings dict as an input and then provides getters or member variables to access those parts be easier to understand in your eyes ?

This is Python not C++, use dictionary [], if it should be accessible as public API structure, then it would maybe make sense, but it is used only by the addon, addon defines the structure of settings, accessing it in single function, you're using shotgun to kill a fly.

they where named this way to make it easy to search for them in the code base.

After 5 years of experience with settings/presets, I can tell you that it won't help, make them understandable, and make them searchable by using [] on dictionary with double quotes, if you want to find it then search for "lakefs", there is nothing better.

and i would wish it would be but at the beginning there was a big debate if we should even have it separated into its own library and it was quite a back and forth if we should or not and now this is the outcome from that.

I would say that the debate was about the binaries, not about providing api for usage, especially when it's dependent on env variables and the fact only one credentials can be used at a time > it is actually complicated to have another addon using lakefs now.