Make LakeFS distribution of USD Libs and AYON USD Resolver optional #72

BigRoy · 2024-10-18T14:11:06Z

Changelog Description

Make LakeFS distribution of USD Libs and AYON USD Resolver optional.

Additional info

Remove the experimental checkbox, which is now the "enable" button on the LakeFS distribution section.
Cleanup setting titles/docstrings

Fixes #71

TODO

Disable the generationg of the AYON USD Resolver Pinning File if the Resolver is not downloaded (or better yet, make that a checkbox of its own somewhere to enable/disable that plug-in)

Testing notes:

Deploy new USD addon package
Launcher and applications should run fine without USD libs and resolver.
Then, Enable the LakeFS distribution in settings: ayon+settings://usd/lakefs/enabled
When enabled, it should behave like before and check for latest USD libs and resolvers + download and configure them accordingly for usage in the applications.

+ Remove the experimental checkbox, which is now the "enable" button on the LakeFS distribution section. + Cleanup setting titles/docstrings

Lypsolon

its capable of downloading and installing a resolver and the usd libs.
tested on hou20.5 almalinux9

idk where it comes from.
but the lakeFs keys are in the addon ? i believe they should not ?
aaaaand the addon points at the Houdini branch of lakeFs not the v0.2.0

BigRoy · 2024-10-18T14:37:41Z

but the lakeFs keys are in the addon ? i believe they should not ?
aaaaand the addon points at the Houdini branch of lakeFs not the v0.2.0

That's likely because you had previously configured this "version" of the addon and there hasn't been a version bump since.. so your server still applies those same settings.

I have not changed any of the defaults or settings values.

mkolar

I would remove any mention of lakeFS from the UI and the keys completely. The distribution mechanism is irrelevant for the admin and users. We're configuring whether they will get AYON USD binaries or not, that's it.

BigRoy · 2024-10-18T16:06:07Z

I would remove any mention of lakeFS from the UI and the keys completely. The distribution mechanism is irrelevant for the admin and users. We're configuring whether they will get AYON USD binaries or not, that's it.

Changing the keys would mean backwards incompatibility with settings. Which is fine of course - but just checking if you're ok with that? @mkolar

BigRoy · 2024-10-18T16:14:12Z

@mkolar Here's a backwards compatible change:

The server URI and repository URI are very related to LakeFs - would you like me to remove LakeFs from that label too?

Also, the only backwards incompatible change I'd need to make then is to change the settings key ayon+settings://usd/lakefs to e.g. ayon+settings://usd/distribution or ayon+settings://usd/binarydistribution. Thoughts/preferences @mkolar ?

antirotor · 2024-10-21T08:24:00Z

While not trying to wreak havoc on the settings, wouldn't be better to have:

USD
	| -- Binary Distributions
                | -- LakeFS  [X]
                          | -- Resolver Application LakeFS Paths
                | -- URL based  [X]
 ...

That way you communicate that LakeFS is just one of the possible distribution methods (we might then add more or remove some), URL basesd is basically Resolver Application Overwrites that IMO isn't very descriptive title anyway.

would be good to have LakeFS and URL mutually exclusive to simplify things.

client/ayon_usd/hooks/pre_resolver_init.py

client/ayon_usd/addon.py

Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>

BigRoy · 2024-10-21T21:22:18Z

That way you communicate that LakeFS is just one of the possible distribution methods (we might then add more or remove some), URL basesd is basically Resolver Application Overwrites that IMO isn't very descriptive title anyway.

Wait, what? That is currently for URL based distributions? I'm at loss maybe at what those settings do.
Anyway, I'd love to restructure that and clean that up - and am personally fine with this now being still in experimental to do the big backwards incompatible cleanup swoop of that now. However, I'm not entirely sure what all the settings are meant to do, etc.

Should we maybe do a call tomorrow to discuss?

Lypsolon · 2024-10-22T06:11:02Z

While not trying to wreak havoc on the settings, wouldn't be better to have:
USD
	| -- Binary Distributions
                | -- LakeFS  [X]
                          | -- Resolver Application LakeFS Paths
                | -- URL based  [X]
 ...                
That way you communicate that LakeFS is just one of the possible distribution methods (we might then add more or remove some), URL basesd is basically Resolver Application Overwrites that IMO isn't very descriptive title anyway.

would be good to have LakeFS and URL mutually exclusive to simplify things.

i just want to say i cant help but feel like we are about to over complicate this thing again.
the PR says it intends to make the Resolver download optional.
how about in this PR we do just that and nothing more ?

the PR is doing a few more things that its title says already maybe we should write an Issue for the Rest of the changes.

Lypsolon · 2024-10-22T06:12:57Z

@BigRoy
i think your TODO might be better of as an Issue and a separate PR so we can keep this one nice and small.

Disable the generation of the AYON USD Resolver Pinning File if the Resolver is not downloaded (or better yet, make that a checkbox of its own somewhere to enable/disable that plug-in)

BigRoy · 2024-10-22T07:13:47Z

I agree - this PR is currently a much needed improvement and backwards compatible. I'd be happy to merge this and follow up with another that restructures it to be backwards incompatible (e.g. refactoring the lakefs attribute itself in settings to more generic or into a binary distribution subgroup).

And same goes for the file pinning - I feel like it's not critical since it can be called just fine without the resolver in use (as far as I know?)

Lypsolon · 2024-10-22T07:17:43Z

I agree - this PR is currently a much needed improvement and backwards compatible. I'd be happy to merge this and follow up with another that restructures it to be backwards incompatible (e.g. refactoring the lakefs attribute itself in settings to more generic or into a binary distribution subgroup).

And same goes for the file pinning - I feel like it's not critical since it can be called just fine without the resolver in use (as far as I know?)

jap the pinning file env variable is not used elsewhere, it would just not be used.

Lypsolon · 2024-10-22T07:21:03Z

I would remove any mention of lakeFS from the UI and the keys completely. The distribution mechanism is irrelevant for the admin and users. We're configuring whether they will get AYON USD binaries or not, that's it.

i must say i would avoid this for now.
as roy mentioned currently the change is Backwards compatible.
also we cant / should not include the keys in source code (even GitHub will mark it as wrong)

the auth tool that connects with Ayon Cloud is not done and it might be more important to make it easy to use now than to have every feature in. the PR is named make it optional after all.

…ttings backwards incompatible)

…s://github.com/ynput/ayon-usd into enhancement/binary_distribution_behind_toggles # Conflicts: # client/ayon_usd/addon.py # client/ayon_usd/hooks/pre_resolver_init.py

BigRoy · 2024-10-23T23:34:52Z

The keys are changed and the labels too.

And it still works for me.
Ready for final review.
@mkolar @dee-ynput ?

Lypsolon

generally works.
test Almalinux 9 houdini20.5

but this function still is not cached even tho it behaves like it is.
this was marked before, i think we should remove the cache or make it cached.

ayon-usd/client/ayon_usd/config.py

Line 11 in e3158a6

def get_global_lake_instance(settings=None):

BigRoy · 2024-10-25T10:22:16Z

generally works. test Almalinux 9 houdini20.5

but this function still is not cached even tho it behaves like it is. this was marked before, i think we should remove the cache or make it cached.

ayon-usd/client/ayon_usd/config.py

Line 11 in e3158a6

def get_global_lake_instance(settings=None):

Thanks @Lypsolon - I agree we should fix that. I'd like to do that in a follow up PR if you're ok. I'm worried that caching it may suddenly introduce other issues like maybe the lakectl instance facing timeouts from it lingering around long enough or whatever.

If you could approve this, then I'll merge and set up a follow up PR.

Lypsolon · 2024-10-28T13:23:32Z

generally works. test Almalinux 9 houdini20.5
but this function still is not cached even tho it behaves like it is. this was marked before, i think we should remove the cache or make it cached.

ayon-usd/client/ayon_usd/config.py

Line 11 in e3158a6

def get_global_lake_instance(settings=None):

Thanks @Lypsolon - I agree we should fix that. I'd like to do that in a follow up PR if you're ok. I'm worried that caching it may suddenly introduce other issues like maybe the lakectl instance facing timeouts from it lingering around long enough or whatever.

If you could approve this, then I'll merge and set up a follow up PR.

sounds good i already approved it so lets merge it.

Stale review - if new comments come up we can resolve them later in a follow up PR

Make LakeFS distribution of USD Libs and AYON USD Resolver optional

03a3a11

+ Remove the experimental checkbox, which is now the "enable" button on the LakeFS distribution section. + Cleanup setting titles/docstrings

BigRoy added the type: enhancement Improvement of existing functionality or minor addition label Oct 18, 2024

BigRoy requested review from mkolar, antirotor and iLLiCiTiT October 18, 2024 14:11

BigRoy self-assigned this Oct 18, 2024

BigRoy requested a review from Lypsolon October 18, 2024 14:11

Lypsolon approved these changes Oct 18, 2024

View reviewed changes

mkolar previously requested changes Oct 18, 2024

View reviewed changes

Remove LakeFS from labels regarding the binary distribution

de90ddf

iLLiCiTiT reviewed Oct 21, 2024

View reviewed changes

client/ayon_usd/hooks/pre_resolver_init.py Outdated Show resolved Hide resolved

iLLiCiTiT reviewed Oct 21, 2024

View reviewed changes

client/ayon_usd/addon.py Outdated Show resolved Hide resolved

BigRoy and others added 2 commits October 21, 2024 11:26

Update client/ayon_usd/hooks/pre_resolver_init.py

08c4e8d

Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>

Update client/ayon_usd/addon.py

8c56f84

Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>

BigRoy requested a review from mkolar October 21, 2024 21:22

BigRoy added 4 commits October 24, 2024 01:05

Make labels less specific about distribution method (lakefs)

524221e

Refactor lakefs settings key to distribution (note: this makes se…

fec7046

…ttings backwards incompatible)

Merge branch 'enhancement/binary_distribution_behind_toggles' of http…

bac5f5d

…s://github.com/ynput/ayon-usd into enhancement/binary_distribution_behind_toggles # Conflicts: # client/ayon_usd/addon.py # client/ayon_usd/hooks/pre_resolver_init.py

Remove LakeFs from label + cosmetics

e3158a6

BigRoy requested review from Lypsolon, iLLiCiTiT and dee-ynput October 23, 2024 23:34

Lypsolon reviewed Oct 25, 2024

View reviewed changes

BigRoy merged commit b402efb into develop Oct 28, 2024
1 check passed

BigRoy deleted the enhancement/binary_distribution_behind_toggles branch October 28, 2024 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make LakeFS distribution of USD Libs and AYON USD Resolver optional #72

Make LakeFS distribution of USD Libs and AYON USD Resolver optional #72

BigRoy commented Oct 18, 2024 •

edited

Loading

Lypsolon left a comment

BigRoy commented Oct 18, 2024

mkolar left a comment

BigRoy commented Oct 18, 2024

BigRoy commented Oct 18, 2024 •

edited

Loading

antirotor commented Oct 21, 2024 •

edited

Loading

BigRoy commented Oct 21, 2024

Lypsolon commented Oct 22, 2024

Lypsolon commented Oct 22, 2024

BigRoy commented Oct 22, 2024

Lypsolon commented Oct 22, 2024

Lypsolon commented Oct 22, 2024

BigRoy commented Oct 23, 2024 •

edited

Loading

Lypsolon left a comment

BigRoy commented Oct 25, 2024

Lypsolon commented Oct 28, 2024

Make LakeFS distribution of USD Libs and AYON USD Resolver optional #72

Make LakeFS distribution of USD Libs and AYON USD Resolver optional #72

Conversation

BigRoy commented Oct 18, 2024 • edited Loading

Changelog Description

Additional info

TODO

Testing notes:

Lypsolon left a comment

Choose a reason for hiding this comment

BigRoy commented Oct 18, 2024

mkolar left a comment

Choose a reason for hiding this comment

BigRoy commented Oct 18, 2024

BigRoy commented Oct 18, 2024 • edited Loading

antirotor commented Oct 21, 2024 • edited Loading

BigRoy commented Oct 21, 2024

Lypsolon commented Oct 22, 2024

Lypsolon commented Oct 22, 2024

BigRoy commented Oct 22, 2024

Lypsolon commented Oct 22, 2024

Lypsolon commented Oct 22, 2024

BigRoy commented Oct 23, 2024 • edited Loading

Lypsolon left a comment

Choose a reason for hiding this comment

BigRoy commented Oct 25, 2024

Lypsolon commented Oct 28, 2024

BigRoy commented Oct 18, 2024 •

edited

Loading

BigRoy commented Oct 18, 2024 •

edited

Loading

antirotor commented Oct 21, 2024 •

edited

Loading

BigRoy commented Oct 23, 2024 •

edited

Loading