Skip to content
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.

Sign up for GitHub

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

Prepare process: Enhancements #849

Merged
merged 16 commits into from
Aug 21, 2024
Merged

Prepare process: Enhancements #849

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9331187
ProcessContext also has information about preparation state
iLLiCiTiT Aug 20, 2024
501cacc
change state of context when is done
iLLiCiTiT Aug 20, 2024
8e9c28a
'ensure_addons_are_process_ready' returns ProcessContext
iLLiCiTiT Aug 20, 2024
f3ccccc
do not return exception on fail
iLLiCiTiT Aug 20, 2024
2b46eee
added ProcessContext arguments to 'ensure_addons_are_process_ready'
iLLiCiTiT Aug 20, 2024
d983879
added comment to project name
iLLiCiTiT Aug 20, 2024
eb4407b
fix returns in docstrings
iLLiCiTiT Aug 20, 2024
92ce331
added 'is_headless_mode_enabled' function
iLLiCiTiT Aug 20, 2024
3bc9933
return bool all the time
iLLiCiTiT Aug 20, 2024
173b2f2
remove ProcessContext methods
iLLiCiTiT Aug 20, 2024
9260a80
require addon name and version
iLLiCiTiT Aug 20, 2024
e5eefc8
added todo
iLLiCiTiT Aug 20, 2024
e0339ae
require addons_manager and exit_on_failure as kwargs
iLLiCiTiT Aug 20, 2024
725584a
require addon name/version
iLLiCiTiT Aug 21, 2024
128ff7a
Merge branch 'develop' into enhancement/process-context-is-returned
iLLiCiTiT Aug 21, 2024
c1d7bb8
Merge branch 'develop' into enhancement/process-context-is-returned
iLLiCiTiT Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 44 additions & 2 deletions client/ayon_core/addon/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,9 @@ class ProcessContext:
addon_version (Optional[str]): Addon version which triggered process.
project_name (Optional[str]): Project name. Can be filled in case
process is triggered for specific project. Some addons can have
different behavior based on project.
headless (Optional[bool]): Is process running in headless mode.
different behavior based on project. Value is NOT autofilled.
headless (Optional[bool]): Is process running in headless mode. Value
is filled with value based on state set in AYON launcher.

"""
def __init__(
Expand All @@ -116,6 +117,47 @@ def __init__(
if kwargs:
unknown_keys = ", ".join([f'"{key}"' for key in kwargs.keys()])
print(f"Unknown keys in ProcessContext: {unknown_keys}")
self._prepared: bool = False
self._exception: Optional[Exception] = None

def is_prepared(self) -> bool:
"""Preparation of process finished.

Returns:
bool: Preparation is done.
"""
return self._prepared

def set_prepared(self):
"""Mark process as prepared."""
self._prepared = True

def preparation_failed(self) -> bool:
"""Preparation failed.

Returns:
bool: Preparation failed.

"""
return self._exception is not None

def get_exception(self) -> Optional[Exception]:
"""Get exception that occurred during preparation.

Returns:
Optional[Exception]: Exception that caused preparation fail.

"""
return self._exception

def set_exception(self, exception: Exception):
"""Set exception that occurred during preparation.

Args:
exception (Exception): Exception that caused preparation fail.

"""
self._exception = exception


# Inherit from `object` for Python 2 hosts
Expand Down
33 changes: 26 additions & 7 deletions client/ayon_core/addon/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def ensure_addons_are_process_context_ready(
process_context: ProcessContext,
addons_manager: Optional[AddonsManager] = None,
exit_on_failure: bool = True,
) -> Optional[Exception]:
):
iLLiCiTiT marked this conversation as resolved.
Show resolved Hide resolved
"""Ensure all enabled addons are ready to be used in the given context.

Call this method only in AYON launcher process and as first thing
Expand Down Expand Up @@ -134,6 +134,8 @@ def ensure_addons_are_process_context_ready(
failed = True
break

process_context.set_prepared()
process_context.set_exception(exception)
output_str = output.getvalue()
# Print stdout/stderr to console as it was redirected
print(output_str)
Expand All @@ -150,23 +152,33 @@ def ensure_addons_are_process_context_ready(
detail = output_str

_handle_error(process_context, message, detail)
if not exit_on_failure:
return exception
sys.exit(1)
if exit_on_failure:
sys.exit(1)


def ensure_addons_are_process_ready(
addon_name: Optional[str] = None,
addon_version: Optional[str] = None,
project_name: Optional[str] = None,
headless: Optional[bool] = None,
addons_manager: Optional[AddonsManager] = None,
exit_on_failure: bool = True,
**kwargs,
) -> Optional[Exception]:
) -> ProcessContext:
"""Ensure all enabled addons are ready to be used in the given context.
BigRoy marked this conversation as resolved.
Show resolved Hide resolved

Call this method only in AYON launcher process and as first thing
to avoid possible clashes with preparation. For example 'QApplication'
should not be created.

Args:
addon_name (Optional[str]): Addon name which triggered process.
addon_version (Optional[str]): Addon version which triggered process.
project_name (Optional[str]): Project name. Can be filled in case
process is triggered for specific project. Some addons can have
different behavior based on project. Value is NOT autofilled.
headless (Optional[bool]): Is process running in headless mode. Value
is filled with value based on state set in AYON launcher.
addons_manager (Optional[AddonsManager]): The addons
manager to use. If not provided, a new one will be created.
exit_on_failure (bool, optional): If True, the process will exit
Expand All @@ -178,7 +190,14 @@ def ensure_addons_are_process_ready(
preparation, if any.

"""
context: ProcessContext = ProcessContext(**kwargs)
return ensure_addons_are_process_context_ready(
context: ProcessContext = ProcessContext(
addon_name,
addon_version,
project_name,
headless,
**kwargs
)
ensure_addons_are_process_context_ready(
context, addons_manager, exit_on_failure
)
return context
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does whoever receive this object have any meaningful reason to set the exception and or set the prepared state - I assume not? I have a feeling if maybe we should just be returning a (static?) data object instead to avoid confusion? We're really just returning the prepared state and the exception - that's it?

Copy link
Member Author

@iLLiCiTiT iLLiCiTiT Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my usecase the context has more value as it fills headless based on logic I should not know about at that specific case. I did want to be able to get exception to be able to handle the error in custom way (e.g. send report to server or store to a file).

For me ensure_addons_are_process_ready is just helper wrapper for exactly the content of the function when I don't want to encapsulate the logic to e.g. UI splash screen, but only run preparation and then get context information that was filled automatically.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm - I see. I suppose the change is made to fit your use case and there are reasons to do it as such.

Copy link
Member Author

@iLLiCiTiT iLLiCiTiT Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unresolving. Those are relevant comments and questions. This is not yet in any release and I would rather not touch it in future, so criticism is welcome.

Full context:
The main reason this functionality is necessary is to give addons option to prepare them -> what would happen in tray if you would use it, like fill ftrack credentials, fill environment variables that are e.g. filled only in tray_start etc. They can stop the process if their need is not matched. Main usage is for webactions, they can be started without ever running tray.

This cannot happen automatically, addon that might need it has to request it specifically.

Function ensure_addons_are_process_ready was added to simplify

from ayon_core.addon import ProcessContext, ensure_addons_are_process_context_ready

def some_func():
    context = ProcessContext(
        "myaddon",
        "1.1.1",
        "myProject"
    )
    ensure_addons_are_process_context_ready(context)
    if context.headless:
        _headless_stuff()
    else:
        _ui_stuff()
from ayon_core.addon import ensure_addons_are_process_ready

def some_func():
    context = ensure_addons_are_process_ready(
        "myaddon",
        "1.1.1",
        "myProject"
    )
    if context.headless:
        _headless_stuff()
    else:
        _ui_stuff()

Goal was to make it somewhat reusable, so it is not tight only for applications but can be used for use-cases.

Copy link
Collaborator

@BigRoy BigRoy Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not entirely sure about the exact use case. I think if we're simplifying it makes more sense to simplify the need to pass the addon version (isn't that e.g. based on the current active bundle for your context?) Why do we need to explicitly pass it the version?

The fact that you need access to any exceptions or whether it's headless or not - I suppose makes sense. I'm also not entirely confident about the need for the prepared state.

I'd expect a ensure_addons_are_process_ready to just return something along the lines of a "boolean" to tell me whether it succeeded or not, potentially with the exception if it's not (which could also be an actual raised exception? Since we're requesting it to ensure it?)

Again, take my comment with a grain of salt since I feel I'm quite on a different track than you and still not 100% sure.

Just here proposing an API that may seem more reasonable:

context = ProcessContext(project_name="myProject")
result = context.initialize_for_addon("myaddon")
if not result:
    print(result.exception)

if context.is_headless:
    _headless_stuff()
else:
    _ui_stuff()

But maybe it's the docstring of ProcessContext that needs improving - because it doesn't seem to be 'generic context' but a process context specific to a particular addon? So it's more of a AddonProcessContext?

Like:

addon_context = AddonProcessContext(addon="myaddon")
addon_context.ensure_dependent_addons_are_ready()

Copy link
Member Author

@iLLiCiTiT iLLiCiTiT Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First idea was to expect addon_name, addon_version and project_name in ensure_addons_are_process_ready. Then I realized that we will in future need to add more data and changing signature of a function is PITE, thus ProcessContext was created, meant as "future-proof data holder".

I would like to keep it as functions instead of methods. And it's not meant as preparation for addon, the addon information is meant as "metadata" that might or might not be relevant for preparation.