-
-
Notifications
You must be signed in to change notification settings - Fork 454
Add a tasks manager status for plugins actions and napari processes #8211
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
Add a tasks manager status for plugins actions and napari processes #8211
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8211 +/- ##
==========================================
+ Coverage 92.98% 93.05% +0.06%
==========================================
Files 703 705 +2
Lines 63365 63474 +109
==========================================
+ Hits 58920 59063 +143
+ Misses 4445 4411 -34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
_tasks: dict[uuid.UUID, TaskStatusItem] | ||
|
||
def __init__(self) -> None: | ||
self._tasks: dict[uuid.UUID, TaskStatusItem] = {} |
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.
Any concerns about concurrent access here? Would it be better to have a Queue of small dataclasses that bundle the uuid and the status?
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.
For the moment not much since to add and update an item uuids are being used and I think the only point where this dict gets iterated is when calling is_busy
, get_status
and cancel_all
(logic that currently only gets triggered when wanting to close the whole app which I think is a point where not other task can get added? 🤔). However, if using a queue to handle the addition of tasks still makes sense let me know!
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.
Fair enough. I'll let others chime in in case they share these concerns, but for now a comment would suffice. Something along the lines "note we are using a dict here that may not be thread-safe; however given the XXX conditions, it should be ok as long as we only perform XXXX tasks in XXX endpoints".
class Status(StringEnum): | ||
PENDING = auto() | ||
BUSY = auto() | ||
DONE = auto() | ||
CANCELLED = auto() | ||
FAILED = auto() |
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.
This is also present in https://github.com/napari/napari-plugin-manager/pull/174/files#diff-845746c74ed93a04bf7677b33b197c4a3086400268ab1f18e2223e6c73730ea7R66 (as a fallback?). Same comments apply.
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.
Yep, over napari-plugin-manager
a sort of fallback of this enum is in place. Over the comment there, I left this comment regarding the match between different scenarios and the statuses: napari/napari-plugin-manager#174 (comment)
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.
Note: This enum should be updated following the discussion at napari/napari-plugin-manager#174
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.
Cursory review of the code looks good to me.
Functionality I definitely approve of.
This requires the napari-plugin-manager PR, correct? If so, we need to make a plugin-manager release and update pins after this is merged.
Also, is this task status something we can expose to public API / document for this reason? That way someone could have a plugin tell napari that its busy doing some process, and for this dialog to show (especially important if they use multi-threading for their plugin) on close. I think that's what you intend here, so I'd love to see a follow-up in docs, and maybe a suggested blurb that we can add to release notes highlights.
src/napari/_qt/qt_main_window.py
Outdated
task_status = task_status_manager.get_status() | ||
if ( | ||
task_status | ||
and ConfirmCloseDialog( | ||
self, | ||
close_app=False, | ||
extra_info='\n'.join(task_status), | ||
display_checkbox=False, | ||
).exec_() | ||
!= QDialog.Accepted | ||
) or ( |
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.
Just confirming that this means the tasks busy dialog box will always be shown regardless of the user's preference to show the warning on a regular close
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.
Yep, as long as there are tasks with a PENDING
or BUSY
status the dialog asking to confirm the application close (with a description of the tasks being processed) will be shown
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.
And it means that it is shown even if action is connected to another napari window?
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.
If the napari windows are in the same process I think they could end up sharing the task manager instance indeed 🤔 It is possible to have multiple napari windows launched from the same process? Do you have a code snippet I could use to check this?
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.
Note: the multiple_viewers_.py
is an example of this possibility
To be able to see the behavior described in the OP yes!
I would say so 👍 |
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.
Hm. In the context of examples. I still think that we should add somehow the teardown to the examples. Either in the script or in a separate script that matches the file name.
Without this, we need to make a PR to docs too.
src/napari/_qt/qthreading.py
Outdated
cancel_callback=worker.quit, | ||
) | ||
worker.started.connect( | ||
lambda: update_task_status( |
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.
Partial?
In general, connecting lambda to signals is not a best practice, and we may think how to avoid it in codebase to not inspire other devs.
src/napari/_tests/test_examples.py
Outdated
monkeypatch.setattr( | ||
ConfirmCloseDialog, | ||
'exec_', | ||
lambda *args: ConfirmCloseDialog.DialogCode.Accepted, | ||
) |
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.
I prefer to patch get_status
as it will close the path triggered by the update from this PR and not hide other problems.
Co-authored-by: Grzegorz Bokota <[email protected]>
…pecific examples instead. Add partial usage when connecting with worker signals
I think this is ready for another review @Czaki , let me know if there is something else to do here! |
@Czaki do you think this is close enough to go in by the end of the week? Otherwise, we'll have to postpone to the next release. |
# prevent showing `ConfirmCloseDialog` even when there are task being done | ||
# needed to prevent napari CI (tests and docs building) from getting stuck | ||
def no_status(): | ||
return [] | ||
|
||
task_status_manager.get_status = no_status |
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.
I don't like this. It shows patching of internals in a bad way...
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.
Another approach that occurs to me is to add some sort of kwarg to the thread_worker
decorator to prevent registration of the task and use it for the examples 🤔 Does that make sense to you? If you have any other idea let me know!
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.
My idea.
- In a separate PR, modify the
Viewer.close
andViewer.close_all
methods to accept theforce
argument. It will disable all questions about closing. - Modify docs conf to use
Viewer.close_all(force=True)
https://github.com/napari/docs/blob/00aefd3a7b9134f72e296a5d15f1cbbcf0e7b473/docs/conf.py#L368 - Come back to this PR and reimplement the check to don't ask questions on force quit.
If you implement 1. I will review it and merge fast.
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.
I would also support this change to viewer.close
and close_all
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.
Thanks for the idea @Czaki ! However, while implementing it, I noticed that it is not necessary to do changes to the viewer.close
and viewer.close_all
methods! Seems like those methods already have the capability to close things without showing the confirm dialog even when the option to show it is enabled. What was missing here, and I just noticed while checking the validation logic to add the force flag 😅, is to take into account the event.spontaneous()
value. If that value is used also over the task status check no dialog will be shown. Last commit takes into account that and seems like it makes possible for the docs and the tests to run without showing the confirm dialog (no need to patch anything over the tests either) 🎉
Anyhow, let me know if that is good enough or if we should actually go with the force flag idea (and probably recheck the usage of the event.spontaneous()
value as well as give a general check to the window close logic)
src/napari/utils/task_status.py
Outdated
item.cancel() | ||
|
||
|
||
task_status_manager = TaskStatusManager() |
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.
I do not have a feeling like introducing this as a part of the public API with a global variable does not feel like a proper solution.
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.
So if I'm understanding correctly, having this as module constant doesn't feel appropiate, right? Maybe is better to instanciate a task manager per window? That makes sense to me (and more if it is possible to launch multiple napari windows at the same time from the same process) but let me know what you think!
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.
Yes. It is possible to launch multiple napari windows in one process. And when providing such a public API, it needs to be ready for a multi-window environment.
We may introduce some private API for the meantime if we want to iterate over to provide a solution.
There is no need to provide a public API for such a feature at the beginning. And even possible future API may be functional.
src/napari/_qt/qt_main_window.py
Outdated
task_status = task_status_manager.get_status() | ||
if ( | ||
task_status | ||
and ConfirmCloseDialog( | ||
self, | ||
close_app=False, | ||
extra_info='\n'.join(task_status), | ||
display_checkbox=False, | ||
).exec_() | ||
!= QDialog.Accepted | ||
) or ( |
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.
And it means that it is shown even if action is connected to another napari window?
…pdate calls to register/update task status
…e without showing confirm close dialog with task status info
Is there already a docs PR to update the docs with this feature? |
While working on the Edit: Or maybe you meant like adding some docs explaining that now the napari window has methods to register task status ( |
At least documentation that will explain that currently we register tasks executed by |
We may think about adding a public API. But I want this API to enforce providing to which viewer the thread is connected. Even if we do not use this information, we would like to use this in the future. |
Here a PR over the docs repo adding a note about the workers task registration and the possibility of a confirm close dialog appearing if a worker is still running when closing the GUI via the window close button: napari/docs#851 |
…ialog when closing napari GUI via close button (#851) # References and relevant issues <!-- What relevant resources were used in the creation of this PR? If this PR addresses an existing issue on the repo, please link to that issue here as "Closes #(issue-number)". If this PR adds docs for a napari PR please add a "Depends on <napari PR link>" --> Depends on napari/napari#8211 # Description <!-- What does this pull request (PR) do? Does it add new content, improve/fix existing context, improve/fix workflow/documentation build/deployment or something else? <!-- If relevant, please include a screenshot or a screen capture in your content change: "An image is worth a thousand words!" --> <!-- You can use https://www.cockos.com/licecap/ or similar to create animations. --> <!-- You can also see a preview of the documentation changes you are submitting by clicking on "Details" to the right of the "Check the rendered docs here!" check on your PR.--> Adds a note over the threading page (`Graceful exit` section) mentioning that starting with napari 0.6.5 workers get registered as task and a confirm close dialog is shown if they are still running when closing napari via the GUI close button <!-- Previewing the Documentation Build When you submit this PR, jobs that preview the documentation will be kicked off. By default, they will use the `slimfast` build (`make` target), which is fast, because it doesn't build any content from outside the `docs` repository and doesn't run notebook cells. You can trigger other builds by commenting on the PR with: @napari-bot make <target> where <target> can be: html : a full build, just like the deployment to napari.org html-noplot : a full build, but without the gallery examples from `napari/napari` docs : only the content from `napari/docs`, with notebook code cells executed slimfast : the default, only the content from `napari/docs`, without code cell execution slimgallery : `slimfast`, but with the gallery examples from `napari/napari` built --> <!-- Final Checklist - If images included: I have added [alt text](https://webaim.org/techniques/alttext/) If workflow, documentation build or deployment change: - My PR is the minimum possible work for the desired functionality - I have commented my code, to let others know what it does --> Co-authored-by: Lorenzo Gaifas <[email protected]>
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: |
References and relevant issues
Supersedes #6993
Related to napari/napari-plugin-manager#174
Part of napari/napari-plugin-manager#53
Description
Add functionality to allow plugins and napari itself to register and handle tasks status as a way to provide a GUI to inform users that there are tasks in progress while closing napari
Preview
Warning dialog

General behavior:
