ogrisel
commented
8 months ago /cc @lesteve @rgommers @dnicolodi.
Closed ogrisel closed 7 months ago
ogrisel
commented
8 months ago /cc @lesteve @rgommers @dnicolodi.
rgommers
commented
8 months ago Thanks @ogrisel! That's a nicely concise fix. I'm not too familiar with this code, but the description at https://docs.python.org/3/library/importlib.html#importlib.machinery.ModuleSpec.submodule_search_locations seems clear enough about this being correct.
I can confirm that it fixes the issue with the SciPy test - it now correctly checks all submodules rather than the test silently passing without doing anything.
dnicolodi
commented
8 months ago NAK. This does not work. There is a reason why I decided to do not populate the __path__ module attribute.
ogrisel
commented
8 months ago What is the reason? There is no inline comment , nor any failing test when i changed that line of code.
dnicolodi
commented
8 months ago __path__ is defined to be "the list of locations where the package’s submodules will be found", where "locations" is interpreted by most code to be a list of directories. However, in editable installations there is not a correspondence between the filesystem location of sources and their place in the module structure. Not even the file names need to match the module names. Therefore, setting __path__ in the way this patch does violates the definition of what __path__ is. You omitted testing __path__ for the extension module in the test package. If you would have tested it, you would have discovered that it points to a different directory than for the other modules in the same package.
ogrisel
commented
8 months ago You omitted testing path for the extension module in the test package. If you would have tested it, you would have discovered that it points to a different directory than for the other modules in the same package.
I don't understand. The extension module (I assume you mean complex.test which stems from complex/test.pyx) is not a package, hence it should not have a __path__ attribute at all, right?
dnicolodi
commented
8 months ago Yes, you are right. However, you cannot find the complex.test module in complex.__path__. This breaks the definition of path.
ogrisel
commented
8 months ago However, you cannot find the complex.test module in complex.path. This breaks the definition of path.
We could populate the __path__ list with the two folders: the regular one with the Python source tree and the one with the compiled extensions, right?
dnicolodi
commented
8 months ago We could populate the
__path__list with the two folders: the regular one with the Python source tree and the one with the compiled extensions, right?
There is no defined relation between the organization of the modules on the filesystem and the logical structure in the package. The files can be rename on installation, moved from one folder to another, etc. Code that assumes it can use the location of the modules on the filesystem or their name to infer the structure of the package is broken.
rgommers
commented
8 months ago The files can be rename on installation, moved from one folder to another, etc. Code that assumes it can use the location of the modules on the filesystem or their name to infer the structure of the package is broken.
It's possible indeed to rename/move on install, but it's rare. So this does fix things for the common case, without downsides I think (?). It's worrying that code in SciPy that I thought was working was actually silently broken before with an empty list, so this does seem like an improvement. Whether it works or not depends on the package's build definitions right, as far as I can tell it isn't possible to break it via some build/install setting.
dnicolodi
commented
8 months ago It does not work for extension modules. I assume that scipy wants pkgutil.walk_packages() to return extension modules too.
rgommers
commented
8 months ago It does not work for extension modules. I assume that scipy wants
pkgutil.walk_packages()to return extension modules too.
Hmm yeah, you are correct. My comment about it being rare doesn't hold when walking the build dir to pick up extension modules - it's quite a bit more common for those to land in a different install location.
So this really won't work, and we're back to the iter_modules conversation on gh-557 I guess.
ogrisel
commented
8 months ago @dnicolodi I iterated on this PR to address the problem of the discoverability of compiled submodules in editable mode. Let me know what you think. In particular, I am not sure if submodules of namespace "packages" should be made discoverable by pkgutil.walk_packages one way or another but I am not sure it's possible in general.
dnicolodi
commented
8 months ago All extension modules are built into the root of the build directory, thus this approach still does not work if there are extension modules installed in different sub-packages. There also can be other files that have an extension that matches the one used to load python modules or python extension modules in the build directory. With this approach all these would be interpreted as being part of the package. As I mentioned in #557, there is a solution to make pkgutil.walk_packages() work, but I haven't hat time yet to implement it. Listing filesystems locations in __path__ is not a good starting point.
ogrisel
commented
8 months ago All extension modules are built into the root of the build directory, thus this approach still does not work if there are extension modules installed in different sub-packages.
This is not true. The source folder tree structure is still present to avoid name collisions in case compiled modules with the same local name exist in two different subpackages of the same top level package. For instance, here is the filesystem structure of scikit-learn when built in editable mode with meson:
And here is the output of walk_packages on the sklearn top level package:
so both Python and compiled extensions are recursively discovered successfully using this strategy.
There also can be other files that have an extension that matches the one used to load python modules or python extension modules in the build directory. With this approach all these would be interpreted as being part of the package.
I agree in theory but will that ever be the case under the build/ folder of a meson-python editable build?
ogrisel
commented
8 months ago But I can try to update this PR to add iter_modules method to the MesonpyMetaFinder class instead.
EDIT: the proper way to do it seems to implement this iter_modules method on a path entry finder for mesonpy and register it via path_hook class method in sys.path_hooks similarly to was is done by default for the regular FileFinder class. pkgutil.get_importer (used indirectly by pkgutil.walk_packages/iter_modules) would then be able to look it up for a given package file system path.
Still, I find the definition of empty __path__ package attributes a potential source of surprise for many code-base, beyond the objective of making pkgutil.walk_packages work as intended.
EDIT2: but looking again at the code of pkgutil.walk_packages, customizing iter_modules alone would not be enough, as walk_packages still relies on the __path__ attribute of a subpackage to do its recursive call. But customizing iter_modules should make it possible to filter out files that should not be considered submodules of a given package.
dnicolodi
commented
8 months ago This is not true.
It depends on how the build definition is structured. If all extension modules are defined in a meson.build in the root project, the build files are all created in the root of the build directory. What we need is not something that works for sklearn but a general solution.
It would be polite if you could at least consider that the maintainer of a project to which you are contributing spent a bit more time than you thinking about the problem space a bit more than you. If they tell you that an approach to solve an issue does not work, it would be nice if you could at least try to understand the objections.
dnicolodi
commented
8 months ago Still, I find the definition of empty
__path__package attributes a potential source of surprise for many code-base, beyond the objective of makingpkgutil.walk_packageswork as intended.
__path__ makes sense only within the python import system implementation. Relaying on it to point to any specific filesystem folder (or to a filesystem folder at all) is bogus and always will be. It breaks zip imports, bundled python applications, and whatnot. I see the fact that meson-python editable installs breaks this wrong expectation as a positive thing that will move developers away from using the __path__ attribute for things for which it is not designed.
dnicolodi
commented
8 months ago I agree in theory but will that ever be the case under the
build/folder of a meson-python editable build?
Sure. For example if a project generates a version.py module with the project version, or a shared library on platforms where both extension modules and shared libraries have the same filename extension.
ogrisel
commented
8 months ago It depends on how the build definition is structured. If all extension modules are defined in a meson.build in the root project, the build files are all created in the root of the build directory. What we need is not something that works for sklearn but a general solution.
Alright, sorry I had not considered it was possible to do otherwise.
It would be polite if you could at least consider that the maintainer of a project to which you are contributing spent a bit more time than you thinking about the problem space a bit more than you. If they tell you that an approach to solve an issue does not work, it would be nice if you could at least try to understand the objections.
Sorry.
eli-schwartz
commented
8 months ago Still, I find the definition of empty
__path__package attributes a potential source of surprise for many code-base, beyond the objective of makingpkgutil.walk_packageswork as intended.
As a dunder attribute it is formally reserved by the implementation. The only thing you are allowed to do with pure semantics here is to use __path__ according to its documentation, to mark a package vs a module and to optionally have it as an empty list OR use it to instruct the import machinery where to search for submodules.
Any code that takes this dunder attribute and doesn't fully implement the semantics of the import machinery in question is automatically broken. This includes detecting cases where it is empty and recognizing that a different import machinery finder must be used if available.
Of course, using this dunder attribute is a convenient hack, but many things are a convenient hack. Emphasis on "hack".
ogrisel
commented
8 months ago The only thing you are allowed to do with pure semantics here is to use path according to its documentation, to mark a package vs a module and to optionally have it as an empty list OR use it to instruct the import machinery where to search for submodules.
Any code that takes this dunder attribute and doesn't fully implement the semantics of the import machinery in question is automatically broken. This includes detecting cases where it is empty and recognizing that a different import machinery finder must be used if available.
According to the above interpretation pkgutil.walk_packages, which is part of the standard library, would be considered broken: indeed it only relies on __path__ to recursively find n+1-level sub-packages. A custom iter_modules implementation alone would not fix this as iter_modules is meant to return direct submodules, not submodules of subpackages.
As far as I know the import machinery does not specify any other way to discover subpackages transitively.
I checked that pkgutil.walk_packages works as expected on the following test zip package structure:
$ zipinfo level0_package.zip
Archive: level0_package.zip
Zip file size: 1304 bytes, number of entries: 6
drwxr-xr-x 2.0 unx 0 bx stor 24-Jan-23 18:16 level0_package/
-rw-r--r-- 1.0 unx 0 bX stor 24-Jan-23 18:15 level0_package/level1_module.py
-rw-r--r-- 1.0 unx 0 bX stor 24-Jan-23 18:15 level0_package/__init__.py
drwxr-xr-x 2.0 unx 0 bx stor 24-Jan-23 18:17 level0_package/level1_package/
-rw-r--r-- 1.0 unx 0 bX stor 24-Jan-23 18:16 level0_package/level1_package/__init__.py
-rw-r--r-- 1.0 unx 0 bX stor 24-Jan-23 18:17 level0_package/level1_package/level2_module.py
6 files, 0 bytes uncompressed, 0 bytes compressed: 0.0%>>> import sys
>>> sys.path.insert(0, "level0_package.zip")
>>> import level0_package
>>> level0_package.__path__
['level0_package.zip/level0_package']
>>> from pkgutil import walk_packages
>>> for info in walk_packages(level0_package.__path__, prefix="level0_package."):
... print(info.name)
...
level0_package.level1_module
level0_package.level1_package
level0_package.level1_package.level2_module
>>> import level0_package.level1_package
>>> level0_package.level1_package.__path__
['level0_package.zip/level0_package/level1_package']and furthermore to confirm that iter_modules only iterates on direct descendants:
>>> from pkgutil import iter_modules
>>> for info in iter_modules(level0_package.__path__, prefix="level0_package."):
... print(info.name)
...
level0_package.level1_module
level0_package.level1_packageIf you agree with my edited comment above (https://github.com/mesonbuild/meson-python/pull/562#issuecomment-1900568972), I think it's possible to compute __path__ attribute for meson-python editable installs as done in this PR to get pkg.walk_packages to work as expected while also registering a custom file entry finder with a specific iter_modules implementation that would avoid introducing false positive submodules in case all compiled extensions of meson-python installed project are located in a shared folder (assuming no name collisions).
I can try to update this PR accordingly.
dnicolodi
commented
7 months ago The proper fix is in #569.
This should solve the problem of empty
__path__attributes for packages imported from an editable install.Fixes https://github.com/mesonbuild/meson-python/issues/557.