Examples gallery fails on case insensitive/case preserving systems when napari installed as editable

[lucyleeow]

🐛 Bug

This problem has been discussed in #209 (comment) and #207 (comment) (and following comments).

@aganders3 has been doing all the investigation, just combining information so it's all in one place and so there is a place to discuss this issue.

On case-preserving but case-insensitive filesytem (e.g., macOS) when napari is installed in editable mode, examples gallery will fail.

With scraper added in #207, following error is seen:

Traceback (most recent call last):
  File "/Users/melissa/mambaforge/envs/napari-dev/lib/python3.11/site-packages/sphinx_gallery/scrapers.py", line 340, in save_figures
    rst = scraper(block, block_vars, gallery_conf)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/melissa/projects/napari-docs/docs/conf.py", line 236, in napari_scraper
    napari.Viewer.close_all()
    ^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'napari.Viewer' has no attribute 'close_all'

Similar napari.viewer/napari.Viewer (module/class) confusion has been noted before, notably when executing example custom_key_bindings.py which uses the fixture: @napari.Viewer.bind_key('w') (more details here).

This is due to combination of:

• sphinx-gallery does some module importing when looking for backreferences (done in _get_short_module_name, for background on why it does this: Specify Target of reference_url sphinx-gallery/sphinx-gallery#947 (comment)), and this can leave new imports in sys.modules.
• setuptools allows case-insensitive imports on certain systems (macOS) with editable installs. @aganders3 has opened as issue there: github.com/[BUG] certain editable installs allow case-insensitive imports pypa/setuptools#3994

Together these create our problem in the docs build. Here are the steps to failure:

1. custom_key_bindings.py (maybe others) access napari.Viewer.bind_key()
2. sphinx-gallery searches for backreferences and in the process tries from napari.Viewer import bind_key
3. The above import fails, but the damage is done - napari.Viewer is now in sys.modules and points to napari/viewer.py, this part of the import succeeds due to the setuptools issue with case-sensitivity
4. The next time napari.Viewer is accessed, it bypasses the getattr lazy-loading in napari/__init__.py and instead gives back the module napari.viewer (with the wrong capitalized name). This can happen in the scraper where it accesses napari.Viewer.close_all(), or in the next example that uses napari.Viewer().

We could fix this in Sphinx-Gallery by avoiding importing - @aganders3 suggested to use importlib.util's find_spec/module_from_spec/exec_module, but this only avoids one layer of side-effects as exec_module will mean anything imported inside the module is still imported. I think this fix will at least our error though. Importing in a subprocess will be safest but is slow see: #209 (comment)
Cleaning up sys.modules was also explored but there are potential problems if all references are not cleaned up and we have different module objects existing at the same time (details here).

We could also fix this if setuptools avoids case-insensitive imports.

Thanks to @aganders3 for all the work on this issue 🙏 If there are any inaccuracies, please let me know and I can amend it (as I don't think you can)!

[lucyleeow]

@aganders3 if you can fix setuptools case-insensitive imports, do you think we should still amend the importing in Sphinx Gallery?

Also we can open an issue over at Sphinx Gallery, and offer possible solutions, Eric will probably be a good judge of what changes would be appropriate in Sphinx Gallery.

[aganders3]

I don't have super strong feelings, but I'd still favor a small change in sphinx-gallery. The issue here comes from the case-insensitive imports (and that is probably more common), but consider this package structure:

pkg
├── __init__.py  # contains "mod = 1"
└── mod.py

To be clear this is pretty clearly bad practice, but please humor me. This is what importing from that package does:

❯ python
Python 3.10.12 (main, Jul 28 2023, 20:50:58) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pkg
>>> pkg.mod
1
>>> import pkg.mod
>>> pkg.mod
<module 'pkg.mod' from '/Users/aandersoniii/src/bad-ideas/pkg/mod.py'>

I played around (probably too much) with the backreferences code in sphinx-gallery trying to clean up after these imports, but cleaning up sys.modules feels like a fool's errand for reasons we've discussed. The safest way to do this (as noted above) would be to use a subprocess, but this is too slow.

Since the examples are run in the same process, I think my current best idea is to only import modules that are already imported when looking at backreferences. This is super simple and safer (I confirmed it prevents the issue that spawned this discussion) at the cost of being slightly less aggressive at finding shorter names for backreferences. A patch would look something like this:

 def _from_import(a, b):
+    if a not in sys.modules:
+        raise ImportError(f"not importing {a}, as it was not previously imported")
     imp_line = f'from {a} import {b}'
     scope = dict()
     with warnings.catch_warnings(record=True):  # swallow warnings

I'd be happy to discuss this on an issue or PR at sphinx-gallery.

[lucyleeow]

I think we can definitely raise an issue in sphinx gallery - and I think Eric will be much better placed to judge.

I am not sure we want to raise an error as this would cause a lot of existing users builds to fail. I also don't want to change behaviour (for existing builds), especially as I am not sure how common this problem is. We could add (yet another) config that will allow users to optionally only import if previously imported. I am always conscious of adding to our extensive config but Eric may feel differently!

[aganders3]

Ah yeah to be clear that fn is already called in a try/except that catches all errors so that change shouldn't actually break builds. The specific exception it raises doesn't really matter.

I'm also typically anti new configs but will defer to the maintainers on that 😄.

[lucyleeow]

It's hard to know how much the behaviour change would affect users, but probably safer to be able to opt out...

(But I think this is a very good solution!)

[aganders3]

Thanks! And yeah I'm not familiar enough with the actual use of the feature to have an instinct for it, haha. Maybe I'll find some example projects that use it and do a diff w/ and w/o this change.

[lucyleeow]

Feel free to just open an issue, most sphinx-gallery contributors are not half as thorough as you!

[lucyleeow]

This gives some background on why it looks for short modules: sphinx-gallery/sphinx-gallery#947 (comment)

[lucyleeow]

Now the fix in Sphinx Gallery and setuptools is in, I think we can close this. @aganders3 feel free to re-open if you wish.