Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow editable install #155

Merged
merged 10 commits into from
May 1, 2024

Conversation

stefanv
Copy link
Member

@stefanv stefanv commented Jan 22, 2024

With this change, spin should be able to detect and work with existing editable installs.
It also checks that the editable install is from the current directory.

@lagru, can you give it a try?

@stefanv stefanv added the type: Enhancement New feature or request label Jan 22, 2024
@stefanv stefanv force-pushed the allow-editable-install branch 3 times, most recently from dc93bc3 to 3709d74 Compare January 23, 2024 01:11
@stefanv
Copy link
Member Author

stefanv commented Jan 23, 2024

Hold before merge; I should be able to get this working on Windows. Done.

import importlib_metadata

try:
dist = importlib_metadata.Distribution.from_name(package)
Copy link
Member

@lagru lagru Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Distribution.from_name(...) expects the distribution name and not the package name!

As is, this line always raises a PackageNotFoundError despite the editable install clearly working otherwise (installed with pip install --no-build-isolation -e .). Running spin test ... etc. still builds as if the editable install is not present.

However, when I set a breakpoint here and use

importlib_metadata.Distribution.from_name("scikit-image")

instead of package which is "skimage" it seems to work! Do you have access to the distribution name at this level?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this is working for you? Maybe my setup is weird?

@stefanv
Copy link
Member Author

stefanv commented Jan 25, 2024

It works for packages where the distribution and package names are the same (example_pkg, numpy, etc.). Fortunately, I think we can get hold of the distribution name too, so will fix.

@stefanv
Copy link
Member Author

stefanv commented Jan 25, 2024

@lagru How about now?

Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great. However, there will be a few issues in scikit-image we need to address before we can use this.

The skimage module of the editable install seems to have a empty __path__ which makes apigen.py and spin docs fail. But that's something we can fix in skimage with

Details

diff --git a/doc/tools/apigen.py b/doc/tools/apigen.py
--- a/doc/tools/apigen.py	(revision f4c1b34ac968d9fda332d7d9a63c83499aaac1f6)
+++ b/doc/tools/apigen.py	(date 1706272296049)
@@ -20,6 +20,7 @@
 # Stdlib imports
 import os
 import re
+from pathlib import Path
 
 from types import BuiltinFunctionType, FunctionType, ModuleType
 
@@ -91,7 +92,7 @@
         # It's also possible to imagine caching the module parsing here
         self._package_name = package_name
         root_module = self._import(package_name)
-        self.root_path = root_module.__path__[-1]
+        self.root_path = str(Path(root_module.__file__).parent)
         self.written_modules = None
 
     package_name = property(

Also spin test is behaving weird. spin test works, spin test -- skimage/ as well, but spin test -- --doctest-plus skimage/ runs into a heaps of ModuleNotFoundError. Even worse spin test --doctest-plus without a target seems to execute the gallery examples and is effectively a DOS attack on the window manager because of plotting windows. Not sure what's going on yet, but I don't think it's the fault of spin.

Edit: Might be solved by moving the test suite into its own directory.

@rgommers
Copy link
Contributor

The skimage module of the editable install seems to have a empty __path__

This came up on the meson-python issue tracker too, and in scipy/sklearn; there's a problem with __path__, you should avoid using it completely indeed. __file__ can also be problematic sometimes when, for example, loading data files - the correct way that's robust to editable path shenenigans is to access files with importlib.resources rather than assume a certain directory tree layout.

@rgommers
Copy link
Contributor

Editable installs are by design incomplete installs, other things that can be missing:

  • headers and shared/static libraries
  • things installed as entrypoints (e.g., a script that goes into site-packages/bin normally)

@lagru
Copy link
Member

lagru commented Feb 5, 2024

Thanks @rgommers! That will be very useful to know when we eventually get to updating that in skimage.

@stefanv stefanv force-pushed the allow-editable-install branch from 0d90393 to bf036f4 Compare May 1, 2024 00:31
@stefanv stefanv force-pushed the allow-editable-install branch from f71492d to 9cc70b4 Compare May 1, 2024 17:40
@stefanv stefanv force-pushed the allow-editable-install branch from 9cc70b4 to 844db07 Compare May 1, 2024 18:02
@stefanv
Copy link
Member Author

stefanv commented May 1, 2024

Also spin test is behaving weird. spin test works, spin test -- skimage/ as well, but spin test -- --doctest-plus skimage/ runs into a heaps of ModuleNotFoundError. Even worse spin test --doctest-plus without a target seems to execute the gallery examples and is effectively a DOS attack on the window manager because of plotting windows. Not sure what's going on yet, but I don't think it's the fault of spin.

I think it is unrelated, since I see the same using pytest --doctest-plus skimage.

@stefanv
Copy link
Member Author

stefanv commented May 1, 2024

At least in 2018, doctests could not be run against an installed package. Has that changed since?

pytest-dev/pytest#2042 (comment)

@stefanv
Copy link
Member Author

stefanv commented May 1, 2024

This seems to work:

pytest --import-mode=importlib --doctest-modules skimage

@stefanv
Copy link
Member Author

stefanv commented May 1, 2024

I'm a bit hesitant to make this the default, given what I read at https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html:

importlib: new in pytest-6.0, this mode uses importlib to import test modules. This gives full control over the import process, and doesn’t require changing sys.path.

For this reason this doesn’t require test module names to be unique.

One drawback however is that test modules are non-importable by each other. Also, utility modules in the tests directories are not automatically importable because the tests directory is no longer added to sys.path.

Initially we intended to make importlib the default in future releases, however it is clear now that it has its own set of drawbacks so the default will remain prepend for the foreseeable future.

@stefanv
Copy link
Member Author

stefanv commented May 1, 2024

I think this is now close-enough to ready to release into the wild for testing. We may need to make a point release as issues are uncovered.

@jarrodmillman jarrodmillman merged commit d3a339e into scientific-python:main May 1, 2024
17 checks passed
@jarrodmillman jarrodmillman added this to the 0.9 milestone May 1, 2024
@lesteve
Copy link
Contributor

lesteve commented May 14, 2024

Thanks for working on this feature and releasing 0.9!

FYI I have opened a WIP PR to gather feed-back on using spin in scikit-learn scikit-learn/scikit-learn#29012.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants