Move `pyface.ui.qt4` to `pyface.ui.qt` #1223

corranwebster · 2023-03-15T10:45:04Z

This moves pyface.ui.qt4.* to pyface.ui.qt and helps with gradually deprecating all uses of "qt4". The current state is internal consistency, optional hooks for backwards compatibility for applications that depend on Pyface.

Changes for downstream libraries and applications should be fairly simple: replacing qt with qt4 in appropriate import statements.

But there are some circumstances where this may not be feasible:

where the end-user doesn't control the code-base (eg. someone who pip-installs Mayavi and gets the new version of Pyface automatically)
where an application developer needs to make a newer version of pyface work with code they don't control that imports from pyface.ui.qt4.*
where a library developer wants to be compatible with newer and older versions of pyface

This PR solves two of these problems by providing opt-in import hooks that replace imports of pyface.ui.qt4.* with corresponding imports of pyface.ui.qt.* so that sys.modules points to the new location (avoiding issues with duplicated objects and modules which would happen with some other strategies). These hooks are not available by default, but can be accessed in a number of ways:

end-users can set environment variables to turn it on: ETS_QT4_IMPORTS to directly turn on the import hooks, and ETS_TOOLKIT=qt4 (as opposed to ETS_TOOLKIT=qt) with the assumption that this is an older environment
application developers can either set ETSConfig.toolkit = "qt4" (as opposed to ETSConfig.toolkit = "qt") with the assumption that this is flagging that this is an application made with older assumptions about import locations; or if they need more control, they can install the import hooks into sys.meta_path in a way that works best for them and any other import hooks that they might be using.

The third case of a library developer who wants to support newer and older versions should be done through the usual mechanisms (eg. try importing pyface.ui.qt.foo and if it fails import pyface.ui.qt4.foo; or looking at pyface version numbers). An alternative which will work in almost all cases is to instead use toolkit_object() to get the thing that they want, which will automatically get it from the right place.

Done:

check about pickle compatibility with mementos: Tasks and Workbench mementos are independent of toolkit
migration documentation (added to toolkits section)

Note:

This ended up taking about a day longer than expected because the original working version of the code was using a deprecated API that would be removed in Python 3.12, so it needed re-implementation after digging in to understand what the new importlib API was doing. In doing so it has been generalized a bit, since we expect to also do this in TraitsUI at a minimum.

Fixes #560

pyface/ui/__init__.py

…lity.

…isions.

mdickinson

We've already discussed the approach here, and I agree that this seems like a reasonable approach to the problem.

I'm not yet convinced that the Loader subclass is implementing the Loader interface in the way that the import machinery expects; but judging by the tests this is working in practice.

docs/source/toolkits.rst

mdickinson · 2023-03-17T10:57:44Z

pyface/image/library/icons.zip

Changes to icons.zip and std.zip again; not sure if you want to keep these.

mdickinson · 2023-03-17T11:00:11Z

pyface/tasks/tests/test_dock_pane_toggle_group.py

@@ -19,7 +19,7 @@
 from traits.etsconfig.api import ETSConfig


-USING_WX = ETSConfig.toolkit not in ["", "qt4"]
+USING_WX = ETSConfig.toolkit not in {"", "qt", "qt4"}


This is for a separate issue/PR since the issue predates this PR, but this seems a bit odd - we're implicitly assuming that if it's not Qt (and it's not null), then it must be wx.

Opened #1224

mdickinson · 2023-03-17T11:01:56Z

pyface/ui/__init__.py

+from importlib.util import find_spec
+from importlib.machinery import ModuleSpec


Swap these around for 🔤 reasons? (Not sure how much we care. isort would do something different here anyway, since it likes to put all the from imports below the import imports in any given section.)

mdickinson · 2023-03-17T11:11:01Z

pyface/ui/__init__.py

+    """
+
+    def __init__(self, fullname, new_name, new_spec):
+        self.fullname = fullname


Could we add a docstring (either here, or at class level)? fullname and new_name are somewhat clear, but it's not clear to me without consulting external sources what the type or contents of new_spec are expected to be.

Done - I added to the class docstring.

mdickinson · 2023-03-17T11:44:10Z

pyface/ui/__init__.py

+
+    def create_module(self, spec):
+        if self.new_name not in sys.modules:
+            import_module(self.new_name)


I'm not sure that this is right - from my reading of the docs, it should be exec_module that's doing the work of actually populating the module, while create_module should just be providing the unpopulated module object. And I believe neither create_module nor exec_module should be causing changes to sys.modules - I think the import machinery takes care of that.

I'm not sure under what circumstances this would matter. I guess we lose the ability to reload (since that would presumably be re-executing exec_module but not create_module), but that doesn't seem like a big loss. Having create_module potentially alter sys.modules worries me a bit more - I'm not sure what the ramifications of that could be.

As you noted exec_module has to be safe to make changes to sys.modules because the code being executed might import things. Agree I'm not 100% sure about create_module but I didn't see any admonitions against it and potentially a user implementation of the create_module method could itself have lazy imports in it for things it needs. ie.

def create_module(self, spec): import whatever_crazy_thing ...

But yes, I messed around with a number of different approaches, and settled on this one. The outcome that we want is that the module produced by this loader is the module in sys.modules for the corresponding pyface.ui.qt.* name. This is straightforward if it's already imported, but if it isn't we need to create it somehow, and just using the standard import machinery seemed safer than trying to do it manually (and correctly) and then inject it into sys.modules.

And unfortunately the current API helpfully takes care of adding things to sys.modules for you (and adding the __spec__) so the result of create_module is I think the last place where we can change what is put into sys.modules.

Reloading should work, since it will end up running the exec_module of the original module that actually does the execution (but I don't care if it doesn't, I think).

mdickinson · 2023-03-17T11:49:53Z

pyface/ui/qt/util/gui_test_assistant.py

-            self.traitsui_raise_patch = mock.patch(
-                "traitsui.qt4.ui_base._StickyDialog.raise_"
-            )
+            try:


It feels as though we should just duplicate the TraitsUI helper in this repository to avoid the traitsui imports. But not in this PR.

Opened #1225

mdickinson · 2023-03-17T12:06:38Z

And I believe neither create_module nor exec_module should be causing changes to sys.modules

Okay - that's nonsense, of course, since exec_module could be executing imports for other modules. It still seems odd to have create_module potentially altering sys.modules.

mdickinson · 2023-03-17T12:45:07Z

I'm not sure that this is right [...]

Please ignore this. I was operating under the wrong mental model of how this was working. Let me go away and think and play a bit before I come back and embarrass myself further.

mdickinson · 2023-03-17T13:15:36Z

Okay, back. I was falsely imagining that we'd be creating new modules for the qt4 imports that were shallow copies of the existing modules, but instead we're making those qt4 imports point to the existing qt modules (and caching in sys.modules).

In which case, this all seems to work.

There does seem to be a minor oddity with packages (as opposed to modules), which as far as I can tell aren't shared, though I'm not sure why.

(pyface) mdickinson@mirzakhani pyface % export ETS_TOOLKIT="qt4"
(pyface) mdickinson@mirzakhani pyface % python
Python 3.11.2 (main, Feb 10 2023, 08:20:03) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyface.ui.qt4.data_view as dv1
<stdin>:1: DeprecationWarning: The pyface.ui.qt4.* modules have moved to pyface.ui.qt.*

Backward compatibility import hooks have been automatically applied.
They will be removed in a future release of Pyface.

>>> import pyface.ui.qt.data_view as dv2
>>> dv1 is dv2  # expecting `True` here
False

I guess this would be problematic if we were in the habit of putting classes or functions in our __init__.py files, but we're not.

corranwebster · 2023-03-17T13:32:19Z

There does seem to be a minor oddity with packages (as opposed to modules), which as far as I can tell aren't shared, though I'm not sure why.

Yes, this is probably safe because of our coding practices; no, I have no idea why it's happening - for a time I had the problem that modules immediately under pyface.ui.qt4 were shallow copies, but deeper ones were not. I might look at it briefly, but this is probably close enough for what we need as an optional, temporary thing.

Co-authored-by: Mark Dickinson <mdickinson@enthought.com>

corranwebster · 2023-03-17T14:35:17Z

This latest set of changes seems to resolve the issue with packages being duplicated: the solution was to tell the ModuleSpec that it is in fact a package by adding:

is_package=(new_spec.submodule_search_locations is not None),

Recently a decision to move pyface.ui.qt4 to pyface.ui.qt has been made in pyface (enthought/pyface#1223) since versions newer than qt4 have become increasingly popular. However, this move can lead to import errors in other packages that are utilizing pyface when they try to import from qt4. For instance, in issue #1026 , we found that the example cannot find the enable.savage.trait_defs.ui.qt module since the default ETS_TOOLKIT is qt while the folder for qt is still enable.savage.trait_defs.ui.qt. The first way I proposed to solve this is by adapting the similar ShadowedModuleFinder approach in pyface (https://github.com/enthought/pyface/blob/main/pyface/ui/qt4/__init__.py). Now, Corran also proposed a simpler solution in the comment which is to use a simple check on import. This may be a simpler solution than using ShadowedModuleFinder. Closes issue #1026

corranwebster added 2 commits March 15, 2023 08:23

Move pyface.ui.qt4 to pyface.ui.qt

09413d1

Replace usage of pyface.ui.qt4 with pyface.ui.qt

bb369b7

corranwebster mentioned this pull request Mar 15, 2023

Rename qt4 to qt throughout #1201

Closed

3 tasks

corranwebster commented Mar 15, 2023

View reviewed changes

pyface/ui/__init__.py Show resolved Hide resolved

corranwebster added 3 commits March 15, 2023 10:56

Update docs for pyface.ui.qt4 -> pyface.ui.qt.

8c8e80e

Fix a test so it works with ETSConfig.toolkit == qt4

c1c908f

Add optional backward compatibility import hooks.

216cbef

corranwebster commented Mar 15, 2023

View reviewed changes

pyface/ui/__init__.py Outdated Show resolved Hide resolved

corranwebster added 3 commits March 15, 2023 16:37

Test all the permutations of pyface.ui.qt4.* import hooks.

f7686ec

Add documentation; completely refactor import code for 3.12 compatibi…

c2cb70a

…lity.

Fix codestyle.

854b332

corranwebster marked this pull request as ready for review March 16, 2023 13:42

Add trailing dots to package names to avoid accidental namespace coll…

f52d4cd

…isions.

mdickinson self-requested a review March 16, 2023 15:40

Merge branch 'main' into enh/move-qt4-qt

32ac8b0

mdickinson reviewed Mar 17, 2023

View reviewed changes

mdickinson approved these changes Mar 17, 2023

View reviewed changes

corranwebster and others added 4 commits March 17, 2023 13:33

Update docs/source/toolkits.rst

321b6b7

Co-authored-by: Mark Dickinson <mdickinson@enthought.com>

Fix duplication of package modules between qt4 and qt namespaces.

d1c4918

Rever image zip files (*sigh*)

750c13a

Improve docstrings a bit.

6025e6d

corranwebster merged commit a9bf026 into main Mar 18, 2023

corranwebster deleted the enh/move-qt4-qt branch March 18, 2023 11:34

homosapien-lcy mentioned this pull request Apr 14, 2023

Move *.trait_defs.ui.qt4 to *.trait_defs.ui.qt enthought/enable#1028

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `pyface.ui.qt4` to `pyface.ui.qt` #1223

Move `pyface.ui.qt4` to `pyface.ui.qt` #1223

corranwebster commented Mar 15, 2023 •

edited

Loading

mdickinson left a comment

mdickinson Mar 17, 2023

corranwebster Mar 17, 2023

mdickinson Mar 17, 2023

corranwebster Mar 18, 2023

mdickinson Mar 17, 2023

corranwebster Mar 17, 2023

mdickinson Mar 17, 2023

corranwebster Mar 17, 2023

mdickinson Mar 17, 2023

corranwebster Mar 17, 2023

mdickinson Mar 17, 2023

corranwebster Mar 18, 2023

mdickinson commented Mar 17, 2023

mdickinson commented Mar 17, 2023

mdickinson commented Mar 17, 2023

corranwebster commented Mar 17, 2023

corranwebster commented Mar 17, 2023

		from importlib.util import find_spec
		from importlib.machinery import ModuleSpec

Move pyface.ui.qt4 to pyface.ui.qt #1223

Move pyface.ui.qt4 to pyface.ui.qt #1223

Conversation

corranwebster commented Mar 15, 2023 • edited Loading

mdickinson 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

mdickinson commented Mar 17, 2023

mdickinson commented Mar 17, 2023

mdickinson commented Mar 17, 2023

corranwebster commented Mar 17, 2023

corranwebster commented Mar 17, 2023

Move `pyface.ui.qt4` to `pyface.ui.qt` #1223

Move `pyface.ui.qt4` to `pyface.ui.qt` #1223

corranwebster commented Mar 15, 2023 •

edited

Loading