Editable installs improvements #569

dnicolodi · 2024-02-04T20:14:09Z

No description provided.

lesteve · 2024-02-05T13:04:33Z

Thanks for this PR!

I gave in a go to see whether that would fix the issue we were seeing in scikit-learn where some of our tests are using pkgutil.walk_packages as I mentioned in #557 (comment).

Right now if I try to run the problematic test with pytest, I get an error AttributeError: 'MesonpyPathFinder' object has no attribute 'find_spec', see below for full stack-trace.

Let me know if I can provide additional information!

Full stack-trace

❯ pytest sklearn/tests/test_common.py -k are_importable
+ /home/lesteve/micromamba/envs/del/bin/ninja
ninja: no work to do.
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/pytest/__main__.py", line 5, in <module>
    raise SystemExit(pytest.console_main())
                     ^^^^^^^^^^^^^^^^^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 197, in console_main
    code = main()
           ^^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 155, in main
    config = _prepareconfig(args, plugins)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 337, in _prepareconfig
    config = pluginmanager.hook.pytest_cmdline_parse(
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/pluggy/_hooks.py", line 501, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/pluggy/_manager.py", line 119, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/pluggy/_callers.py", line 138, in _multicall
    raise exception.with_traceback(exception.__traceback__)
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/pluggy/_callers.py", line 121, in _multicall
    teardown.throw(exception)  # type: ignore[union-attr]
    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/helpconfig.py", line 104, in pytest_cmdline_parse
    config = yield
             ^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/pluggy/_callers.py", line 102, in _multicall
    res = hook_impl.function(*args)
          ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 1094, in pytest_cmdline_parse
    self.parse(args)
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 1446, in parse
    self._preparse(args, addopts=addopts)
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 1320, in _preparse
    self.pluginmanager.consider_preparse(args, exclude_only=False)
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 727, in consider_preparse
    self.consider_pluginarg(parg)
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 753, in consider_pluginarg
    self.import_plugin(arg, consider_entry_points=True)
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/config/__init__.py", line 801, in import_plugin
    __import__(importspec)
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1322, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1262, in _find_spec
  File "/home/lesteve/micromamba/envs/del/lib/python3.12/site-packages/_pytest/assertion/rewrite.py", line 104, in find_spec
    spec = self._find_spec(name, path)  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap_external>", line 1524, in find_spec
  File "<frozen importlib._bootstrap_external>", line 1498, in _get_spec
AttributeError: 'MesonpyPathFinder' object has no attribute 'find_spec'

dnicolodi · 2024-02-05T15:07:03Z

Thanks for testing. I cut some corners in the implementation. It seems that pytest uses one of the interfaces I haven't implemented. I'll add it when I find a moment.

lesteve · 2024-02-05T15:34:54Z

Great, let me know if there is something I can help with!

Add support for the exclude_dirs and exclude_files arguments of install_subdir() Meson function. Extend editable install tests to cover this functionality and a more complex package layout.

dnicolodi · 2024-02-05T21:30:36Z

@lesteve Can you try again, please?

lesteve · 2024-02-05T21:48:23Z

Great! This seems to fix both #557 (comment) and #568.

lesteve · 2024-02-19T08:05:30Z

I just saw that this was added on the 0.16.0 milestone, which is great news, let me know if there is anything I can do to help move it forward!

rgommers · 2024-02-19T09:14:15Z

Yes, I hope we can get this in and released soon. I need to finish testing and reviewing; holidays interfered a bit. I'll try to finalize my review in the next couple of days.

rgommers

This is very close. I did a fair bit of testing; it's definitely a large improvement. For PyWavelets it fixed an issue, for NumPy and SciPy the tests got more accurate. There is one hiccup with pkgutil.walk_packages, I commented inline about what I think the problem is.

rgommers · 2024-02-27T20:53:07Z

mesonpy/_editable.py

+                        ispkg = True
+            if modname and '.' not in modname:
+                yielded.add(modname)
+                yield prefix + modname, ispkg


I've found one issue where pkgutil.walk_packages returns directories with files in it that get installed with install_subdir but don't have an __init__.py. According to the docs, only modules should be returned.

It looks like the condition here should include an is_dir part, and if it's a directory then ispkg must be True for this to be yielded. Although not sure if that will mess with namespace packages; this is handled in the find_spec function higher up, but not in this iter_modules.

I ran into this in both NumPy and SciPy, where the test_public_api.py::test_all_modules_are_expected tests turn up a few of these non-package directories.

You're right. My understanding was that namespace packages should be returned too. I even added test to check that this was the case. However, I've just tested and they are indeed not returned. Let me fix this.

Thanks for the review @rgommers. The issue with namespace packages should be fixed now.

The test failures are due to mesonbuild/meson#12920

Thanks! I can confirm that it works now with both numpy and scipy, and from some more debug printing it seems to function exactly as expected.

Test failures with Meson master shouldn't be blocking here, so let's get this in.

Set the __path__ module attribute to a placeholder path. Because how editable installs are implemented, this cannot correspond to the filesystem path for the package. And add a sys.path_hook that recognizes this paths and returns a path loader that implements iter_modules(). This allows pkgutil.iter_packages() to work properly. Fixes mesonbuild#557. Fixes mesonbuild#568.

Since the mechanism for editable installations was improved in mesonbuild/meson-python#569 and subsequently released in meson-python v0.16.0, this bumps the minimum version for it for use during compilation.

dnicolodi marked this pull request as draft February 4, 2024 20:15

dnicolodi force-pushed the editable-module-path branch 5 times, most recently from bf80201 to b989bae Compare February 4, 2024 21:39

dnicolodi mentioned this pull request Feb 4, 2024

BUG: empty __spec__.submodule_search_locations and __path__ values for editable packages #562

Closed

dnicolodi marked this pull request as ready for review February 4, 2024 21:40

dnicolodi force-pushed the editable-module-path branch from b989bae to 3092827 Compare February 4, 2024 21:55

dnicolodi added 4 commits February 5, 2024 00:04

MAINT: resolve Meson warning in test package

e241dae

MAINT: minor simplification

4f7a168

MAINT: mark internal method as such with a leading underscore

351890b

MAINT: reduce indentation

9d0592a

dnicolodi force-pushed the editable-module-path branch 2 times, most recently from 075cd39 to 54dc58c Compare February 4, 2024 23:31

dnicolodi force-pushed the editable-module-path branch 4 times, most recently from 5b21a86 to c9e3058 Compare February 5, 2024 21:26

ENH: respect exclude_dirs and exclude_files in editable installs

c74bc13

Add support for the exclude_dirs and exclude_files arguments of install_subdir() Meson function. Extend editable install tests to cover this functionality and a more complex package layout.

dnicolodi force-pushed the editable-module-path branch from c9e3058 to 8f19aec Compare February 5, 2024 21:27

dnicolodi force-pushed the editable-module-path branch 2 times, most recently from 48f8a5f to e29e9a3 Compare February 6, 2024 21:27

rgommers added the enhancement New feature or request label Feb 15, 2024

rgommers self-requested a review February 15, 2024 09:55

rgommers added this to the v0.16.0 milestone Feb 15, 2024

rgommers mentioned this pull request Feb 22, 2024

Bug: cannot run tests with an editable installation of pyWavelets, possible pytesttester test collection discrepancy? PyWavelets/pywt#700

Closed

agriyakhetarpal mentioned this pull request Feb 22, 2024

BLD: Test editable installations for PyWavelets in CI PyWavelets/pywt#702

Merged

lesteve mentioned this pull request Feb 24, 2024

BUILD/CI Switch to Meson as main build backend scikit-learn/scikit-learn#28506

Merged

rgommers reviewed Feb 27, 2024

View reviewed changes

dnicolodi force-pushed the editable-module-path branch from e29e9a3 to 063e6b7 Compare February 27, 2024 22:07

dnicolodi force-pushed the editable-module-path branch from 063e6b7 to 4924a5a Compare February 27, 2024 22:10

rgommers approved these changes Feb 28, 2024

View reviewed changes

rgommers merged commit d377bf7 into mesonbuild:main Feb 28, 2024
37 of 39 checks passed

lesteve mentioned this pull request Mar 18, 2024

ENH: no output in editable verbose mode when there is no work to do #594

Merged

lesteve mentioned this pull request Apr 4, 2024

Reduce ninja's verbosity from subprocesses scikit-learn/scikit-learn#28758

Closed

agriyakhetarpal mentioned this pull request Apr 17, 2024

meson-python does not append editable keyword before ABI tag(s) with --editable flag #620

Closed

lesteve mentioned this pull request May 14, 2024

spin test does not work with an editable install scientific-python/spin#185

Closed

lesteve mentioned this pull request May 22, 2024

MAINT Require meson-python >= 0.16 and remove temporary work-arounds scikit-learn/scikit-learn#29081

Merged

lagru mentioned this pull request Jul 17, 2024

Rework installation instructions scikit-image/scikit-image#7434

Merged

lesteve mentioned this pull request Oct 28, 2024

Assorted cleanups around recording the version number #691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editable installs improvements #569

Editable installs improvements #569

dnicolodi commented Feb 4, 2024

lesteve commented Feb 5, 2024

dnicolodi commented Feb 5, 2024

lesteve commented Feb 5, 2024

dnicolodi commented Feb 5, 2024

lesteve commented Feb 5, 2024

lesteve commented Feb 19, 2024

rgommers commented Feb 19, 2024

rgommers left a comment

rgommers Feb 27, 2024

dnicolodi Feb 27, 2024

dnicolodi Feb 27, 2024

dnicolodi Feb 27, 2024

rgommers Feb 28, 2024

Editable installs improvements #569

Editable installs improvements #569

Conversation

dnicolodi commented Feb 4, 2024

lesteve commented Feb 5, 2024

dnicolodi commented Feb 5, 2024

lesteve commented Feb 5, 2024

dnicolodi commented Feb 5, 2024

lesteve commented Feb 5, 2024

lesteve commented Feb 19, 2024

rgommers commented Feb 19, 2024

rgommers left a comment

Choose a reason for hiding this comment

rgommers Feb 27, 2024

Choose a reason for hiding this comment

dnicolodi Feb 27, 2024

Choose a reason for hiding this comment

dnicolodi Feb 27, 2024

Choose a reason for hiding this comment

dnicolodi Feb 27, 2024

Choose a reason for hiding this comment

rgommers Feb 28, 2024

Choose a reason for hiding this comment