Add logic to allow napari to know that plugin actions are being done

[dalthviz]

Supersedes #56
Related to napari/napari#8211
Part of #53

[codecov]

Codecov Report

❌ Patch coverage is 89.74359% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.10%. Comparing base (aaf109d) to head (d2eee7f).

Files with missing lines	Patch %	Lines
src/napari_plugin_manager/qt_plugin_dialog.py	78.26%	5 Missing ⚠️
src/napari_plugin_manager/base_qt_plugin_dialog.py	91.66%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
- Coverage   94.42%   94.10%   -0.32%     
==========================================
  Files          14       14              
  Lines        2152     2222      +70     
==========================================
+ Hits         2032     2091      +59     
- Misses        120      131      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jaimergp]

Is this "Done" but exit_code != 0 or "failed to start"? With non-subprocess tasks I think it's easy to tell, but I'm not sure what it means in a subprocess-based task.

In other words, how can we identify:

• Tasks that could start and then finished with exit code 0
• Tasks that could start and then finished with exit code != 0
• Tasks that failed to even start

[dalthviz]

I would say that a status DONE should only be used for successful finished task and otherwise a FAILED status should be set. So, taking into account the cases above:

Tasks that could start and then finished with exit code 0

That corresponds to a DONE status

Tasks that could start and then finished with exit code != 0
Tasks that failed to even start

Those cases correspond to a FAILED status. However, if a more granular set of statuses should be created then for the cases above maybe something like:

Tasks that could start and then finished with exit code != 0

FAILED status

Tasks that failed to even start

START_FAILED status

?

[jaimergp]

I think START_FAILED makes sense, but I'd like to hear from others too.

[TimMonko]

I like having these be more explicit variable names would be great.
for example COMPLETED instead of DONE implies more success to me.

[dalthviz]

So It would be better to have something like:

• PENDING
• BUSY
• COMPLETED
• FAILED
• START_FAILED

?

Are there are any other statuses/names that could be preferred? Or maybe removed/added? Let me know!

[jaimergp]

I was checking how Qt handles this and they have several enums:

• ExitStatus
• ProcessError
• ProcessState
• ... plus a method exitCode().

I think our enum is merging several of these, perhaps orthogonal, states.

• PENDING means ProcessState.NotRunning, plus our own notion that we are waiting for it in queue.
• BUSY means ProcessState.Starting || ProcessState.Running.
• COMPLETED means ProcessState.NotRunning && ExitStatus.NormalExit && .exitCode() == 0
• FAILED means ProcessState.NotRunning && ( (ExitStatus.NormalExit && .exitCode() != 0) || ExitStatus.CrashedExit || any ProcessError that is not FailedToStart).
• START_FAILED means ProcessState.NotRunning && ProcessError.FailedToStart.

Are we ok with FAILED meaning all those possible cases? I think it's ok and we don't further granularity, but I wanted to bring this to our attention to show how complex it can get if we start thinking about it more thoroughly.

[TimMonko]

Cool cool. This is definitely a bit hard of code for me to review since I haven't looked at this repo as much.

However, I'm just wondering if we can pull out a "simple" example from this to show how a plugin author could incorporate this logic into their own plugin. Sort of like my review comment here napari/napari#8211 (review)

[TimMonko]

I like having these be more explicit variable names would be great.
for example COMPLETED instead of DONE implies more success to me.

[TimMonko]

hmm this method name and _update_task_status are leaving me with confusion. Could we find some better names to help it not slide off my smooth brain so easily.

[dalthviz]

Rechecking the code I think it should be possible to only have the update_task_status public method defined. Could having only the public method make things more clear? Let me know if that could help with the confusion!

Edit: Or let me know if there is a naming that could be less confusing!

[dalthviz]

Do you have any suggestion for the naming here @TimMonko ? Or do you think that leaving only the public method could help make the logic easier to follow? Let me know!

[dalthviz]

I'm just wondering if we can pull out a "simple" example from this to show how a plugin author could incorporate this logic into their own plugin

The simpliest example I could think of looks something like this:

from napari.utils.task_status import (
    Status,
    register_task_status,
    update_task_status,
)

# Register a new task status via `register_task_status`.
# You can get an id to later on update the specific task status:

task_status_id = register_task_status(
    'MyPlugin',  # String to identify from where the task comes from (`provider`)
    Status.BUSY,  # One of the values of the `Status` enum (`task_status`)
    'My task',  # String description of the task (`description`)
    cancel_callback=None,  # Optional `Callable` to be triggered in case
                           # the task gets cancelled (for example when closing Napari)
)

...

# While the task is being done use `update_task_status`:

update_status_result = update_task_status(
    task_status_id,  # Task status id from when the task status got registered (`task_status_id`)
    Status.COMPLETED,  # New `Status` enum value (`status`)
    description='My task completed',  # Optional new string description
)

[jaimergp]

Btw, this issue is related #53, not sure if we should mark it as will-close. The conversation there also had some ideas that I'm not sure made it here.

[dalthviz]

Btw, this issue is related #53, not sure if we should mark it as will-close. The conversation there also had some ideas that I'm not sure made it here.

Checking the issue, maybe what is not being covered here is defining an expiration date/time? Not completly sure how useful that could be since the actual run of the task would be done by the plugin itself but let me know if that should be added here! For the moment I will add in the PR's description that this is part of #53