PR: Fix bug with environ handling

[larsoner]

I'm not 100% sure what the intention of modifying os.environ at all is, but assuming it's meant to set it so that e.g. new processes that spawn make use of the same framework, then there's a bug in the current implementation in master:

>>> import os
>>> import PyQt6
>>> 'QT_API' in os.environ
False
>>> import qtpy
>>> qtpy.API  # good
'pyqt6'
>>> 'QT_API' in os.environ  # okay...
True
>>> os.environ['QT_API']  # bad
'pyqt5'

This bug was found precisely because a pattern like this: have no QT_API set, import PyQt6, then import qtpy, then spawn a new process (using joblib), have it fail because in that process import qtpy fails because PyQt5 isn't installed, but os.environ['QT_API'] == 'pyqt5'.

This PR thus avoids changing os.environ at all until a suitable API has been chosen, and only then (in the else clause of the trys) set os.environ[QT_API].

An alternative would be not to modify os.environ at all, but this seems less than ideal because it will make it so that the spawned process will not necessarily end up with the same Qt binding, depending on whether or not that process does an import PyQt6 and import qtpy in the same order as the main process, at least in the case where a user has two bindings (e.g., PyQt5 and PyQt6).

This PR also promotes a assert statement to a proper and more informative raise ValueError(...) (that also will not be skipped in Python optimized mode), and changes some 'QT_API's to QT_API, as it seems like that's what the variable name is for. But if either of these is not actually desirable, I can revert!

[dalthviz]

Thank you @larsoner for finding the bug and checking into this! I left a comment regarding the ValueError exception (maybe we shoudl create a new custom exception following the spirit of PythonQtError and PythonQtWarning ).

Also, could be worthy to try to add a test to check that we now have the proper behavior (the env var QT_API is updated correctly when is not initially set). I think the test should go in qtpy/tests/test_main.py.

Besides that this LGTM 👍

[larsoner]

The only clean way I could think of to do it was to use subprocess. It might be possible to use monkeypatch.delenv but it seems less likely to work correctly.

[larsoner]

Green!

[ccordoba12]

Thanks for your work on this @larsoner! I left some small comments for you below.

[ccordoba12]

Thanks @larsoner for your help with this!

[dalthviz]

Thank you for all the work here @larsoner !

[larsoner]

Happy to contribute to this awesome package! We've successfully used it to seamlessly transition three packages from PyQt5 only to framework-agnostic code

[ccordoba12]

Pretty cool @larsoner! If possible, could let us know what packages exactly? That help us for grant applications.

[larsoner]

• MNE-Python
• PyVistaQt
• mne-qt-browser