-
Notifications
You must be signed in to change notification settings - Fork 129
Conversation
Hmm - I wouldn't push this direction. We're now basically maintaining our own vendorized copy of Honestly if this is the hack we'd allow I'd say that maybe we should put a
try:
import click
except ImportError as exc:
# Import our own vendorized version, e.g. adding it on path
import_our_click() I recall there being a feature where you could do that so that that file acts as if it was whatever it imported, it can act like I'd consider that less maintenance likely. |
The wrapper is not even 5% of click logic. I don't think this wrapper needs more maintainance, only what could need maintainance in future is
To be honest, that is more complicated than this wrapper. Also more complicated to understand what is used when. I would rather explicitly name it differently and use always only the one, or use proposed option 2.
I'll bet my life, it would be more maintainance than this wrapper. |
If I need to write CLI code for an addon and I see I feel like if we're differentiating we might as well just put this into If anyone then wants to instead expose API using So if what you're saying is just using a thin command line interface that we just put in |
Oh wait, no - this does still Are we just saying that now we're not passing on the Is that just still missing from this PR? |
The difference is that it does not need click to define cli commands, groups, options etc. it needs click only when you want to convert it for click. Look at change in ftrack addon, specifically to With that, we can remove |
Yes, I was missing this part - thanks. Then I suppose the approach is fine with me. Would still love if we can improve on the dependency hell in a better way; and even better, drop all Py2 support too. |
We have to keep Python 2 in OpenPype (not in AYON 🙏 ). |
Not sure, if my comment related to this PR or not but here are my 2 cents. I'm not able to launch Houdini 20 without #5932 In essence, the problem with these:
Full log:
|
This PR is not fixing the issue, it is first step to be able to fix it... |
Actually - it's worse. As Mustafa's logs show there is also a conflict with the PIL library from OpenPype. |
One at a time, please add it to the issue, not related to this PR. |
Side note: Just heard from @fabiaserra that the PIL lib is only used for the TVPaint and Photoshop integration. For their case they'd just removed it as a required dependency to OpenPype and it fixed the PIL thing for them. @fabiaserra could you check out this proposal and provide your own feedback? Then maybe @iLLiCiTiT can finish the PR so it actually fixes the issue, because last he said:
I suppose the bug is still there and would need more input to continue. |
Just a note, we're AYON focused now, and in AYON is Pillow already not added to PYTHONPATH, so it is resolved for us. Change of dependencies would require to bump minor version of OpenPype. This PR needs final decisions: Where to put the wrapper, and how to name it.
|
Isn't it here https://github.com/ynput/OpenPype/blob/develop/pyproject.toml#L50 ? that will be picked up by the dependency package for Ayon as well? I removed it from there and confirmed that I no longer get the Pillow error running latest OP minor (haven't tested in Ayon because I was lazy to add H20 and update the dependency package but I'd assume it's the same). I now only get the As for my preferred solution, I think here's where |
I think the target of this PR should be openpype.I opened Houdini 20 from develop branch and faced no issues in Ayon. then I checked out this PR and didn't face any errors in Ayon.
|
First of all, target of this PR is to fix
No, it is not. That is only OpenPype build related. OpenPype addon has different pyproject.toml.
The target is AYON and could be used in OpenPype too (with another PR). This PR is to avoid import of The PIL is NOT handled by this PR, and won't be, please stop to mention it here as it's not relevant. First crash in your traceback in because of click, that is what this PR is about.
Did you have enabled an addon that is using click, like ftrack or kitsu, when you've tried?
That's a long run task. This PR is solution for current issue without reimplementing whole applications launch logic and all addons and figuring out how to deploy rez to workstations and many other issues that must be solved. |
Oh nice, I hadn't seen that other pyproject, good to know
Yeah I understand, that's why I think as a first stop solution it's better if we just hack it by prioritizing the DCC click than creating the wrapper, both should be short-term solutions until we come up with a better plan |
Thanks for the explanation.
I have Kitsu enabled in both Openpype standalone and Ayon server.
Today.. now. I don't know why the click error doesn't show up any more in H20 Ayon Mode. |
We can't determine the path in reliable way. We have clients which use custom launch scripts instead of houdini executable in application settings. |
is $HHP resolved too late? |
You'd need to set the Then there might also be a case where |
Yeah makes sense. On the interim I was able to hack it in our env on the H20 entry, pretty straightforward: |
As long as the bug is documented and the different possible solutions on how to hack it... I think it would be perfectly fine as a temporary solution until we can tackle |
For what it's worth - we ended up doing the same thing as it felt most reliable since there were multiple conflicts we had to manage where |
20af276
to
ade8d4d
Compare
exactly, adding the wrapper is just a tiny bandaid that doesn't fix the actual root of the problem... |
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.
tested it a little bit,but the code looks fine
Changelog Description
This is a proposal how to resolve issues with
click
python module. Issue #5921 reported that in Houdini 20+ is our click clashing with click in houdini, where is expected higher version. We can't update our version to support older pythons (NOTE older Python 3).Possible solutions
click
approach how to define commands and their options. This gives a little bit more options to enhance cli callbacks.This PR solution
Created wrapper to mimic click behavior, with small enhancement to be able to use chain function calls which simplify registering methods as cli functions, that might be helpfull in cli commands that need to create
ModulesManage
. Used the wrapper in ftrack addon and it works...Now the question is if we should use option n1, option n2 or we should look for option n3.
Note: This PR does not resolve the issue fully, we would still need to remove
click
from dependencies and move it next to pyside.[cuID:OP-7416]