REF/FIX: Fix Python 3.11 support and test suite-focused cleanup #573

klauer · 2023-08-28T23:14:41Z

Description

ophyd.Kind enum.Enum, being an IntFlag is a bit of a special beast. Its functionality in Python 3.11 differs in enumeration of items - resulting in typhos only picking up a couple of component kinds.
Secondarily, multiprocessing to run a caproto IOC ended up being problematic for Python 3.11, so I restructured the test suite a bit to run IOCs by name (simply regenerating the test cases in the parent process and in the IOC)
Profiling code is skipped on 3.11
"All QMenu escape hatch" remains for only one test (all screenshots test). Unclear why this is but we get a green checkmark.
Vendored pydm load_ui_file and modified it so we can always get our Display instance back and clean it up if desirable.
Ignore deleted qt objects on SignalConnection.remove_connection
Avoid hundreds of warnings during profiling
Avoid creating subdisplays during "hide_subdisplays"
Add a big pytest configuration helper to aid in finding dangling widgets
Avoid failing screenshot taking when widgets are garbage collected at the same time

Motivation and Context

Python 3.11 support would be nice eventually

This PR manifested itself like this:

Python 3.11 had some obvious incompatibilities with enums, so I fixed that
Once that was fixed, the benchmark/profiling test suite was causing major issues, so I refactored it to avoid multiprocessing and isolate the test IOC in its own script
That still wasn't enough to get the test suite to pass. I saw widgets leaking from one test to another, so I added a conftest pytest hook that aimed to remove all stragglers
Then I found that Python 3.11 benchmarking still wasn't working great, so I disabled it at moved on
... Then I found a bunch of stragglers and some weird things (listed above as 'avoid' or 'fix')
And finally, there was one failing test I couldn't resolve (test_take_top_level_widget_screenshots sees hundreds of QMenu instances at the top level)
And now, green checkmark

Overall, I think this is in a better spot than before. It's absolutely not perfect, though.

How Has This Been Tested?

Locally and test suite

Where Has This Been Documented?

Screenshots (if appropriate):

    def dump(obj, file, protocol=None):
        '''Replacement for pickle.dump() using ForkingPickler.'''
>       ForkingPickler(file, protocol).dump(obj)
E       _pickle.PicklingError: Can't pickle <class 'ophyd.device.FlatConnect'>: attribute lookup FlatConnect on ophyd.device failed

klauer · 2023-08-28T23:23:35Z

Well it failed spectacularly on GitHub Actions, whereas it passed locally so 🤷
Going to mark profiling code as xfail temporarily (PR is still draft)

klauer · 2023-09-07T17:23:12Z

OK, this is a bit of a mess and there are a ton of changes. I did this over a long period of time with (sometimes) limited attention being paid to what I was doing.

Each change should be really scrutinized!

klauer · 2023-09-07T17:28:56Z

typhos/tests/conftest.py

+
+    if final_widgets:
+        if all(isinstance(widget, QtWidgets.QMenu) for widget in final_widgets):
+            logger.error("%s: Top level QMenu widgets were not cleaned up. Not failing the test suite.", item.nodeid)


This actually only happens in one test!
test_take_top_level_widget_screenshots
It doesn't make sense to me why this is the case.
Still, this escape hatch is what is allowing the test suite to pass for all versions of Python.

ZLLentz

I'm very 👍 on this
I probably left a bunch of questions as I went through this

ZLLentz · 2023-09-08T00:25:39Z

typhos/display.py

+        except Exception as ex:
+            display: Optional[pydm.Display] = getattr(ex, "pydm_display", None)
+            if display is not None:
+                display.setObjectName("_typhos_test_suite_ignore_")


What's the functional effect of renaming these display objects after a failed load?

Functionally, setting the object name for a pydm.Display does nothing - it's a big of a magic string for the test suite and for us when looking in with debug tools is all.

The context of this block is:

The new template fails to load

typhos notices that and tries to reload the prior template

In typhos master,

A pydm.Display is left dangling as a top-level widget and TyphosDeviceDisplay never even sees a reference to it

In this PR,

That pydm.Display is seen by TyphosDeviceDisplay and intercepted as a failed load

We mark it for deletion and wave goodbye

The test suite still picks it up because it's a top-level widget and somehow doesn't get destroyed in time for it to be happy (see below)

The test suite sees the "magic" object name and says "OK this is known and expected; let's not fail the test"

Now to be honest, the marked step is fishy and it'd be nice to understand why that's happening. Is there a reference we're missing? It's worth investigating at some point.

ZLLentz · 2023-09-08T00:29:25Z

typhos/panel.py

+            "hinted": True,
+            "config": True,
+            "omitted": True,
+        }


Feels bad but this definitely is unlikely to ever change at this point so it's probably totally fine and there doesn't seem to be an appropriate more direct way to inspect these now

as far as I can tell, the only way to get the names programatically is to do something similar to this ugly mess:

for val in range(8): print(val, Kind(val).name) 0 omitted 1 normal 2 config 3 normal|config 4 None 5 hinted 6 config|4 7 normal|config|hinted

But there's actually no way to know how many bits the bitflag is supposed to have so there's no way to know when to stop. The len returns 2 which is just "there are names for slots 1 and 2". Wild.

Ref: https://docs.python.org/3/whatsnew/3.11.html#enum

Changed Flag to only consider primary values (power of two) canonical while composite values (3, 6, 10, etc.) are considered aliases; inverted flags are coerced to their positive equivalent.

dir(Kind) / inspect.getmembers / ... yeah, for something simple like this I think typing it out is sadly the best option.

Do we rely on this iterability in our other code bases, I wonder?

neither of those standard functions actually gives you the results which is wild to me

>>> 'omitted' in dir(Kind) False >>> Kind.omitted <Kind.omitted: 0>

(I forgot to install ipython in my py311 test branch)

ZLLentz · 2023-09-08T00:54:40Z

typhos/tests/conftest.py

+    app = QtWidgets.QApplication.instance()
+    if app is None:
+        return []
+    return [weakref.ref(widget) for widget in app.topLevelWidgets()]


You know a PR is going to be good when we bust out the weakref

ZLLentz · 2023-09-08T00:58:03Z

typhos/tests/conftest.py

+    return res
+
+
+@pytest.hookimpl(hookwrapper=True, trylast=True)


Is there any functional difference for this being implemented as a pytest hook wrapper vs an always-include fixture?

I tried this as a fixture first without success.
For one, the teardown order of all the fixtures (especially qtbot) is important.
Most importantly, however, a fixture that fails at teardown causes the next test to fail (!). This was really surprising to me.

Does a test failing at teardown make all the subsequent tests fail?

test passes (fail at teardown) -> test fails -> test passes
or

test passes (fail at teardown) -> test fails -> test fails....

Fixtures failing at teardown cause only the subsequent test to fail

For the record:

The reason why this PR doesn't include a fixture to do the same check is due to the above: fixture teardown exceptions don't affect the current test but the (single) subsequent test

The pytest hook as implemented of this PR functions as we would hope: the assertion in this hook applies to the active test (and does not interfere with the subsequent test)

ZLLentz · 2023-09-08T01:03:02Z

typhos/tests/conftest.py

+            logger.error(failure_text)
+
+    try:
+        assert not final_widgets, failure_text


I like the methodology you used here to track these issues down

ZLLentz · 2023-09-08T01:04:09Z

I notice that you left some of the cleanup for another day- I think that's completely appropriate. You've cleaned up almost all of it already and the last QMenus seem to be lurking from upstream code.

tangkong

The results speak for themselves, "cleanup" has never belonged more in a PR title.

Morbid curiosity prompts me to look more closely at the test failures, but that shouldn't really hold this up, particularly since we're trying to tag in the near future

tangkong · 2023-09-07T18:33:35Z

typhos/benchmark/cases.py

+    start_ioc: bool,
+    full_test_name: str,
+    auto_exit=True,
+    request=None,


As someone looking at this benchmark code for the first time I'd love a type hint here

tangkong · 2023-09-07T18:44:38Z

typhos/benchmark/ioc.py

@@ -0,0 +1,78 @@
+"""
+Helpful functions that don't belong in a more specific submodule.


It's probably possible to more accurately describe this module right? Now that it's mostly IOC related things.

tangkong · 2023-09-08T00:23:53Z

typhos/plugins/core.py

@@ -94,6 +95,10 @@ def __init__(self, channel, address, protocol=None, parent=None):
        # Add listener
        self.add_listener(channel)

+    def __dtor__(self) -> None:


Google searches fail me when finding this, is there a docs link you could share?

Sadly no... this is a weird (undocumented?) internal part of sip which, right before the C++ object gets deleted, this Python method gets called: https://github.com/search?q=__dtor__+language%3APython&type=code

tangkong · 2023-09-08T00:39:34Z

typhos/tests/conftest.py

+
+        try:
+            widget.isVisible()
+            widget.windowTitle()


Why is calling both of these necessary?

I think calling one may be sufficient and my reasoning here is a bit hand-wavy/anecdotal:

It seems like simple boolean attributes may not trigger the "is the object still alive check; if not raise RuntimeError to alert the user"

So I added windowTitle() as a second one, since it requires an actual string to Python string conversion under the hood

It'd be good to dig into the details one day, but for today I think double method call is good enough for me...

I'm totally good with being doubly-sure. Just curious if there was some portion of the widget that doesn't get caught by widget.isVisible() or something

typhos/tests/conftest.py

tangkong · 2023-09-08T15:57:38Z

typhos/tests/test_utils.py

@@ -276,5 +276,12 @@ def test_take_top_level_widget_screenshots(qtbot: pytestqt.qtbot.QtBot):
    widget = QWidget()
    qtbot.addWidget(widget)
    screenshots = list(utils.take_top_level_widget_screenshots(visible_only=False))


A potentially dumb question. Why is this generating any QMenus at all? A naive (and honestly idealistic) reading of this test is that we're creating a single QWidget, then taking a screenshot of it. Knowing that widgets aren't really getting cleaned up between tests, I can see why QMenus get involved, but it shouldn't generate any... right?

I have repeated this ~~dumb~~ question to myself many times over the course of this PR.

I have no answers 😦
That said, there's one thing I didn't try: what if we load up gammaray when this test fails and we poke around with it there? It'd be worthwhile I think

klauer · 2023-09-08T16:19:46Z

🤞

klauer · 2023-09-08T17:41:38Z

2 green checkmarks in a row indicate it's probably good enough as-is.
Before this gets any larger, if you guys are good with it - I'd say let's merge @tangkong @ZLLentz

We may revisit and adjust how strict this is in the future based on how many intermittent failures we get (due to gc/teardown slowness). Increasing the wait time may also suffice...

ZLLentz · 2023-09-08T19:48:44Z

I'll click merge now to make sure it happens

klauer added 2 commits August 28, 2023 16:06

FIX: Python 3.11 + ophyd Kind support

c86ba5b

TST: rearrange benchmarks to avoid pickle error in Python 3.11

0c0d115

klauer added 25 commits August 28, 2023 16:24

TST: skip benchmarks on Python 3.11 for now

8b1f231

MNT: try dtor on signalconnection to avoid emit-after-destruct

9f3f165

MNT: notes about screenshots

447cafa

TST: add_widget prior to context

4fba138

TST: pytest hook for verifying all widgets are captured

6bebd81

MNT: fix poor clean-up attempt

55696bc

MNT: one more time

b570146

BLD: remove line profiler pin for 3.11 on conda

0068658

FIX: widgets may be gc'd before screenshot; ignore them

d88fa85

MNT: widgets are no longer weakref

e25ba21

Merge remote-tracking branch 'origin/master' into fix_py311

c8be05f

TST: one last try for cleanup

852c1f6

MNT: show referrers

f72890b

MNT: better gc during test suite

cc9912e

FIX: avoid keyerror when cache is externally cleared

471573b

MNT: benchmark tests poisoning cache?

6c3fa7f

FIX: hide_subdisplays inadvertently creates subdisplays

8958edf

FIX: teardown deleteLater runtimeerrors

ed16e74

TST/FIX: add suite to qtbot

2e388df

TST: our test suite holds onto a list of widgets

438ae5a

TST: less references

8273be9

TST: less references

70a25af

FIX: remove profiler warnings for QOL improvement

580a94e

REF: overly complicated failed load mechanism

6abd0df

TST: just qmenus? probably ok

84ce792

klauer marked this pull request as ready for review September 7, 2023 17:22

klauer changed the title ~~WIP: Fix Python 3.11 support~~ REF/FIX: Fix Python 3.11 support Sep 7, 2023

klauer requested review from ZLLentz and tangkong September 7, 2023 17:24

klauer commented Sep 7, 2023

View reviewed changes

klauer linked an issue Sep 7, 2023 that may be closed by this pull request

Test Suite Fails Explosively on GHA py3.11 #564

Closed

klauer changed the title ~~REF/FIX: Fix Python 3.11 support~~ REF/FIX: Fix Python 3.11 support and test suite-focused cleanup Sep 7, 2023

ZLLentz approved these changes Sep 8, 2023

View reviewed changes

tangkong approved these changes Sep 8, 2023

View reviewed changes

tangkong reviewed Sep 8, 2023

View reviewed changes

Merge branch 'master' into fix_py311

a9e7aea

klauer added 3 commits September 8, 2023 09:26

MNT: type hint the request fixture

99dc019

MNT: document + make the benchmark IOC easier to use

01c8840

DOC: pre-release notes

70cd4db

ZLLentz merged commit 45331c1 into pcdshub:master Sep 8, 2023
9 checks passed

klauer mentioned this pull request Sep 8, 2023

Include benchmark test suite in Python 3.11 #581

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF/FIX: Fix Python 3.11 support and test suite-focused cleanup #573

REF/FIX: Fix Python 3.11 support and test suite-focused cleanup #573

klauer commented Aug 28, 2023 •

edited

Loading

klauer commented Aug 28, 2023

klauer commented Sep 7, 2023

klauer Sep 7, 2023

ZLLentz left a comment

ZLLentz Sep 8, 2023

klauer Sep 8, 2023

ZLLentz Sep 8, 2023

ZLLentz Sep 8, 2023

ZLLentz Sep 8, 2023

klauer Sep 8, 2023

ZLLentz Sep 8, 2023

ZLLentz Sep 8, 2023

ZLLentz Sep 8, 2023

klauer Sep 8, 2023

tangkong Sep 8, 2023

klauer Sep 8, 2023

klauer Sep 8, 2023

ZLLentz Sep 8, 2023

ZLLentz commented Sep 8, 2023

tangkong left a comment

tangkong Sep 7, 2023

tangkong Sep 7, 2023

tangkong Sep 8, 2023

klauer Sep 8, 2023

tangkong Sep 8, 2023

klauer Sep 8, 2023

tangkong Sep 8, 2023

tangkong Sep 8, 2023

klauer Sep 8, 2023

klauer commented Sep 8, 2023

klauer commented Sep 8, 2023

ZLLentz commented Sep 8, 2023

		@@ -0,0 +1,78 @@
		"""
		Helpful functions that don't belong in a more specific submodule.

REF/FIX: Fix Python 3.11 support and test suite-focused cleanup #573

REF/FIX: Fix Python 3.11 support and test suite-focused cleanup #573

Conversation

klauer commented Aug 28, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Screenshots (if appropriate):

klauer commented Aug 28, 2023

klauer commented Sep 7, 2023

Choose a reason for hiding this comment

ZLLentz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz commented Sep 8, 2023

tangkong left a 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

klauer commented Sep 8, 2023

klauer commented Sep 8, 2023

ZLLentz commented Sep 8, 2023

klauer commented Aug 28, 2023 •

edited

Loading