Add option to check if plugin try to set viewer attr outside main thread #5195

Czaki · 2022-10-09T19:40:11Z

Description

When reading #5142 I remind that in the past, we met problems that may lead to application crashes if the viewer attribute is modified outside the main thread.
In this PR I add NAPARI_ENSURE_PLUGIN_MAIN_THREAD environment variable that, if set, enables the check of the thread from which the object is modified. This modification works only for objects wrapped with PublicOnlyProxy, which is how the viewer is passed to plugins in napari. Not all modifications outside the main thread may be caught. It is possible, that use of some napari function may lead to this (as the wrapper is removed when moving to the napari code).

This PR introduces dependency of napari.utils._proxies from napari._qt but only if the environment variable is set.

Add this PR to the main will allow the plugin creator to check if they do such an operation. For example, when they observe a random crash of napari. Example of such crash could be found here: #4982

This PR also warns to set private attributes in PublicOnlyProxy wrapped object. Currently, we only warn about getting its value.

This PR requires to add documentation in `docs/plugins/debug_plugins.md] from #5142. So it may require an update if #5142 is merged first.

Type of change

Bug-fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

References

How has this been tested?

example: the test suite for my feature covers cases x, y, and z
example: all tests pass with my change
example: I check if my changes works with both PySide and PyQt backends
as there are small differences between the two Qt bindings.

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
If I included new strings, I have used trans. to make them localizable.
For more information see our translations guide.

codecov · 2022-10-09T20:20:47Z

Codecov Report

Merging #5195 (9492984) into main (6ba16c1) will increase coverage by 0.19%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##             main    #5195      +/-   ##
==========================================
+ Coverage   88.84%   89.03%   +0.19%     
==========================================
  Files         579      579              
  Lines       49109    49009     -100     
==========================================
+ Hits        43629    43634       +5     
+ Misses       5480     5375     -105

Impacted Files	Coverage Δ
napari/utils/_testsupport.py	`63.63% <83.33%> (+2.02%)`	⬆️
napari/_qt/_tests/test_proxy_fixture.py	`100.00% <100.00%> (ø)`
napari/_qt/_tests/test_qt_utils.py	`100.00% <100.00%> (ø)`
napari/_qt/utils.py	`80.19% <100.00%> (+0.19%)`	⬆️
napari/plugins/_tests/test_npe2.py	`100.00% <100.00%> (ø)`
napari/utils/_proxies.py	`94.93% <100.00%> (+1.28%)`	⬆️
napari/utils/_tests/test_proxies.py	`100.00% <100.00%> (ø)`
napari/components/experimental/chunk/_pool.py	`85.71% <0.00%> (-7.94%)`	⬇️
napari/utils/io.py	`72.00% <0.00%> (-6.95%)`	⬇️
napari/layers/tracks/_track_utils.py	`87.50% <0.00%> (-3.41%)`	⬇️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

andy-sweet

This looks good, though I'd like to see if we could do this without Qt and there may be an opportunity simplify a few things in the implementation and tests. None of my comments are strong, so feel free to push back and I'll likely approve.

Ideally, this would be two PRs - one for the private __setattr__ warning and one for the non-main thread __setattr__ exception. I guess the non-main thread code is only executed if an environment variable is defined, so I'd be OK bringing this altogether.

napari/_qt/utils.py

andy-sweet · 2022-10-10T14:15:05Z

napari/utils/_proxies.py

+            typ = type(self.__wrapped__).__name__
+            self._private_attr_warning(name, typ)
+
+            # raise AttributeError


Is this a leftover?

same as in __getattr__. When we remove private access there should be raised an exception about this,

napari/utils/_proxies.py

napari/utils/_tests/test_proxies.py

andy-sweet · 2022-10-10T14:50:23Z

napari/_qt/utils.py

+    thread_flag : bool
+        True if we are in the main thread, False otherwise.
+    """
+    return QCoreApplication.instance().thread() == QThread.currentThread()


Could we rely on Python's threading.current_thread and threading.main_thread to implement this function in a Qt-independent way and define it outside of _qt?

I do not think earlier about main_thread.

In general it is not good solution (as main thread in python is thread in which start interpreter, and Qt main threat is thread where Application object is created.

But as it is for debugging purpose, then, with proper explanation it could be done in this way. I need to rethink this.

If we expect that the interpreter's main thread to sometimes be different from the thread in which the application is created, then maybe this should be called in_qt_main_thread or in_gui_thread?

I do not expect but this is possible. But I'm not sure if we need to support it.

I second Andy's sentiments here. Mostly, it'd be nice to new Qt dependancies in otherwise non-graphical code.

So you suggest implementing this in pure python? So, where put the documentation that is required to create a QApplication object in the python main thread?

I don't know. I don't have strong feelings about it, especially with this sitting in a qt directory.

andy-sweet · 2022-10-10T14:57:27Z

napari/utils/_proxies.py

+        if os.environ.get("NAPARI_ENSURE_PLUGIN_MAIN_THREAD", False):
+            from napari._qt.utils import check_if_in_main_thread
+
+            if not check_if_in_main_thread():


Given this class is named PublicOnlyProxy, I'm not sure this is the right place for this check. But it is convenient and I can't think of a better approach right now. I also can't think of a good name that would describe both checks, so I think it's fine to leave it as is for now - especially since it's guarded with the environment variable.

napari/utils/_tests/test_proxies.py

perlman · 2022-10-10T20:30:47Z

napari/utils/_proxies.py

@@ -73,6 +92,33 @@ def __getattr__(self, name: str):
            # )
        return self.create(super().__getattr__(name))

+    def __setattr__(self, name: str, value: Any):
+        if os.environ.get("NAPARI_ENSURE_PLUGIN_MAIN_THREAD", False):


Is this something that could go into settings instead?
(e.g. how experimental octtree is configured)

I'm not a fan of having behavior deep into napari changed based on external configurations. (But, I don't have a strong conviction here. :)

Is this something that could go into settings instead?

But this is for debugging purposes for the plugin creators. Not something that a common user may want to use.

Perhaps a dev section in the settings? The target -- devs or users -- doesn't change my feeling one continuous querying of the environment.

Checking documentation https://docs.python.org/3/library/os.html#os.environ, and it is a pure dict query. It will be faster than querying settings.

Czaki · 2022-10-11T08:22:58Z

I have implemented an alternative approach when Qt check will be used only if available, but the code will work even if Qt is unavailable.

andy-sweet · 2022-10-12T14:56:21Z

napari/_qt/_tests/test_qt_utils.py

+        a = 1
+
+    monkeypatch.setenv('NAPARI_ENSURE_PLUGIN_MAIN_THREAD', 'True')
+    single_threaded_executor = ThreadPoolExecutor(max_workers=1)


Probably best to make this a fixture that calls shutdown on teardown (credit to @kcpevey):

@pytest.fixture() def single_threaded_executor(): executor = ThreadPoolExecutor(max_workers=1) yield executor executor.shutdown()

Or use the executor in a context when it's needed:

with ThreadPoolExecutor() as executor: f = executor.submit(x.__setattr__, 'a', 2) f.result() assert x.a == 2 f = executor.submit(x_proxy.__setattr__, 'a', 3) with pytest.raises(RuntimeError): f.result()

andy-sweet · 2022-10-12T15:05:54Z

napari/utils/_proxies.py

+        if self._is_private_attr(name):
+            if self._is_called_from_napari():


I love how readable this is now!

andy-sweet · 2022-10-12T15:29:08Z

napari/utils/_proxies.py

+        True if we are in the main thread, False otherwise.
+    """
+
+    global in_main_thread


Why do we need the global? Is this so that we only execute this function once for performance reasons?

This function is only called when an env variable is set, so I don't think we need to worry to much about performance. So if that's the only reason to do this, I'd suggest removing the global and just defining this function as in_main_thread.

Yes, this is for performance reasons.

Then I'd definitely suggest simplify this to:

def in_main_thread() -> bool: try: from napari._qt.utils import in_qt_main_thread return in_qt_main_thread() except ImportError: return in_main_thread_py() except AttributeError: warnings.warn( "Qt libs are available but no QtApplication instance is created" ) return in_main_thread_py()

Does that make sense?

andy-sweet · 2022-10-12T15:30:25Z

The warning for setattr looks great and should definitely come into v0.4.17. How strongly do you feel about bringing in the thread checks? I still feel like splitting this PR in two could be the best way to go here.

…thread

andy-sweet

Looks good - thanks for the updates!

andy-sweet · 2022-10-17T15:29:08Z

napari/utils/_testsupport.py

@@ -261,6 +261,38 @@ def actual_factory(
                warnings.warn(msg)


+@pytest.fixture
+def make_napari_viewer_proxy(make_napari_viewer, monkeypatch):


Is this used anywhere? If not, did you use it to help debug this PR?

If it's not used, but you think it's useful, you might want to add a comment to clarify that this is useful for debugging and should not be deleted.

this is a replacement for make_napari_viewer for plugins creators and is referred in my PR to docs https://github.com/napari/docs/pull/7/files

Currently, plugins during tests do not have warnings about private attribute access, only during actual execution.

Czaki · 2022-10-27T15:13:00Z

merge this requires napari/docs#7 to be merged also

andy-sweet · 2022-11-01T16:05:04Z

@Czaki : as this has two PRs to coordinate (this + docs), feel free to merge both when you'r ready.

Czaki · 2022-11-01T21:30:36Z

@Czaki : as this has two PRs to coordinate (this + docs), feel free to merge both when you'r ready.

@andy-sweet I have added a test. Could you check it?

andy-sweet

New test looks good - except for the name of it! I'll approve after you change that.

I also suggested an alternate more concise implementation of the test. That's optional.

napari/utils/_tests/test_proxies.py

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

for more information, see https://pre-commit.ci

andy-sweet

Looks great! Feel free to merge when you want.

…ead (#5195) Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

lucyleeow · 2023-07-05T06:24:12Z

napari/plugins/_tests/test_npe2.py

@@ -19,6 +19,9 @@

 @pytest.fixture
 def mock_pm(npe2pm: 'TestPluginManager'):
+    from napari.plugins import _initialize_plugins
+
+    _initialize_plugins.cache_clear()


@Czaki could you please explain why this is needed (I can't work it out)? Thank you!

Czaki added 2 commits October 9, 2022 13:31

add warning about set private attr when using proxy

f9ab224

add proxy test for exception on settatr from the wrong thread

81e0571

github-actions bot assigned Czaki Oct 9, 2022

github-actions bot added qt Relates to qt tests Something related to our tests labels Oct 9, 2022

Czaki added 2 commits October 9, 2022 21:55

fix setting private attribute in napari code.

d61a3f6

skip test if no qt

aa7ac77

Czaki changed the title ~~Add option to check if pluugin try to set viewer attr outside main thread~~ Add option to check if plugin try to set viewer attr outside main thread Oct 9, 2022

jni approved these changes Oct 10, 2022

View reviewed changes

andy-sweet requested changes Oct 10, 2022

View reviewed changes

Czaki added 3 commits October 10, 2022 21:13

extract check of call from napari to separate function

50beed1

improve test

239a9fa

add commented exception

28e6e12

Czaki requested a review from andy-sweet October 10, 2022 19:27

perlman reviewed Oct 10, 2022

View reviewed changes

Czaki added 2 commits October 11, 2022 10:20

alternative approach for checking main thread

fc41611

remove test guard

0caab14

Czaki added 2 commits October 12, 2022 15:15

mark warning as expected

384a97b

add test when qappliacation avaliable

6bfcac6

andy-sweet reviewed Oct 12, 2022

View reviewed changes

make single_threaded_executor fixture

f25e98f

andy-sweet reviewed Oct 12, 2022

View reviewed changes

andy-sweet added this to the 0.4.17 milestone Oct 12, 2022

Czaki mentioned this pull request Oct 12, 2022

Add warning about set private attr when using proxy #5209

Merged

12 tasks

Czaki added 2 commits October 13, 2022 01:40

Merge branch 'main' into feature/debug_main_thread

c740058

add fixture for testing plugin

dbb785a

Czaki added 2 commits October 15, 2022 00:24

restore keep reference to viewer proxy

81c3285

Merge remote-tracking branch 'origin/master' into feature/debug_main_…

58831cc

…thread

Czaki reopened this Oct 14, 2022

github-actions bot added qt Relates to qt tests Something related to our tests labels Oct 14, 2022

fix test

006fc8d

andy-sweet approved these changes Oct 17, 2022

View reviewed changes

merge

1bc2b39

add tests

0efb137

Czaki added feature New feature or request and removed documentation labels Nov 1, 2022

andy-sweet requested changes Nov 1, 2022

View reviewed changes

napari/utils/_tests/test_proxies.py Outdated Show resolved Hide resolved

Czaki and others added 4 commits November 1, 2022 23:09

Update napari/utils/_tests/test_proxies.py

36bc5d7

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

6304920

for more information, see https://pre-commit.ci

clean cache of _initialize_plugins

c93ce82

move tests to _qt

9492984

Czaki requested a review from andy-sweet November 2, 2022 00:05

andy-sweet approved these changes Nov 2, 2022

View reviewed changes

Czaki merged commit 505df71 into napari:main Nov 2, 2022

Czaki deleted the feature/debug_main_thread branch November 2, 2022 09:38

beniroquai mentioned this pull request Dec 13, 2022

Bug: Segfaults appearing with "QObject::setParent: Cannot set parent, new parent is in a different thread" #4982

Closed

Czaki modified the milestones: 0.5.0, 0.4.18 Jun 23, 2023

Czaki mentioned this pull request Jun 24, 2023

v0.4.18 - release process #5911

Closed

lucyleeow reviewed Jul 5, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to check if plugin try to set viewer attr outside main thread #5195

Add option to check if plugin try to set viewer attr outside main thread #5195

Czaki commented Oct 9, 2022 •

edited

Loading

codecov bot commented Oct 9, 2022 •

edited

Loading

andy-sweet left a comment

andy-sweet Oct 10, 2022

Czaki Oct 10, 2022 •

edited

Loading

andy-sweet Oct 10, 2022

Czaki Oct 10, 2022

andy-sweet Oct 10, 2022

Czaki Oct 10, 2022

perlman Oct 10, 2022

Czaki Oct 10, 2022

perlman Oct 10, 2022

andy-sweet Oct 10, 2022

perlman Oct 10, 2022

Czaki Oct 10, 2022

perlman Oct 10, 2022

Czaki Oct 10, 2022

Czaki commented Oct 11, 2022

andy-sweet Oct 12, 2022

andy-sweet Oct 12, 2022

andy-sweet Oct 12, 2022

Czaki Oct 12, 2022

andy-sweet Oct 12, 2022 •

edited

Loading

Czaki Oct 12, 2022

andy-sweet commented Oct 12, 2022

andy-sweet left a comment

andy-sweet Oct 17, 2022

Czaki Oct 17, 2022

Czaki commented Oct 27, 2022

andy-sweet commented Nov 1, 2022

Czaki commented Nov 1, 2022

andy-sweet left a comment

andy-sweet left a comment

lucyleeow Jul 5, 2023 •

edited

Loading

		if self._is_private_attr(name):
		if self._is_called_from_napari():

Add option to check if plugin try to set viewer attr outside main thread #5195

Add option to check if plugin try to set viewer attr outside main thread #5195

Conversation

Czaki commented Oct 9, 2022 • edited Loading

Description

Type of change

References

How has this been tested?

Final checklist:

codecov bot commented Oct 9, 2022 • edited Loading

Codecov Report

andy-sweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet commented Oct 12, 2022

andy-sweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki commented Oct 27, 2022

andy-sweet commented Nov 1, 2022

Czaki commented Nov 1, 2022

andy-sweet left a comment

Choose a reason for hiding this comment

andy-sweet left a comment

Choose a reason for hiding this comment

lucyleeow Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Czaki commented Oct 9, 2022 •

edited

Loading

codecov bot commented Oct 9, 2022 •

edited

Loading

Czaki Oct 10, 2022 •

edited

Loading

andy-sweet Oct 12, 2022 •

edited

Loading

lucyleeow Jul 5, 2023 •

edited

Loading