-
Notifications
You must be signed in to change notification settings - Fork 1
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
Remove config.py #31
Comments
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.
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.
thanks for providing this info this exact case actually happend today so i can stop debugging for it.
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. 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 ?
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.
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
they where named this way to make it easy to search for them in the code base.
i generally agree that making the settings access a bit more simple would be a fair idea.
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.
yes it should be.
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. 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. |
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.
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.
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.
This is Python not C++, use dictionary
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
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. |
Issue
I see the intention of
config.py
content, but it complicates things a lot. (All my comments in #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.).
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.
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.
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 orayon_
prefix in attribute name? It is obvious those are ayon settings you don't need to mark them that specifically in attribute name.ayon_usd_resolver_settings
->resolver
,lakefs_settings
>lakefs
.Code change example
Should be just
Only with this change the content of the file is almost half size (without docstrings). Btw I would remove
global
from the name. IfLakeCtl
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 setos.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?The text was updated successfully, but these errors were encountered: