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.

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

Conversation

iLLiCiTiT
Copy link
Member

@iLLiCiTiT iLLiCiTiT commented Aug 20, 2024

Changelog Description

Functions ensure_addons_are_process_ready and ensure_addons_are_process_context_ready returns True/False if failed or not.
Object of ProcessContext expects addon name and version and are not optional.
Added new function is_headless_mode_enabled to ayon_core.lib.

Testing notes:

The functionality is not used anywhere at this moment so we don't have to worry about backwards compatibility.

  1. Validate the changes made make sense.
  2. Only addon using this function is applications addon in this PR Define WebActions ayon-applications#1 which needs these changes to be able handle errors.

@ynbot ynbot added type: enhancement Improvement of existing functionality or minor addition size/XS labels Aug 20, 2024
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

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

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

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

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.

Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

Didn't test - but the code changes visually make sense

@iLLiCiTiT iLLiCiTiT changed the title Prepare process: Return process context object Prepare process: Enhancements Aug 20, 2024
@iLLiCiTiT iLLiCiTiT merged commit f87a433 into develop Aug 21, 2024
1 check passed
@iLLiCiTiT iLLiCiTiT deleted the enhancement/process-context-is-returned branch August 21, 2024 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS type: enhancement Improvement of existing functionality or minor addition
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants