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

ENH: implement editable wheels without having to run meson install #279

Merged
merged 7 commits into from
Mar 11, 2023

Conversation

dnicolodi
Copy link
Member

Exploring the idea of using a custom loader to avoid having to run meson install. This works for Python modules. Supporting data files is possible but requires some more work.

@dnicolodi dnicolodi force-pushed the editable branch 15 times, most recently from 9d1d646 to b0059c7 Compare January 29, 2023 02:39
@dnicolodi
Copy link
Member Author

Working on this one question arises: what should we do with the Python bytecode cache files? Should we allow them and store them next to the source files, either in the source or build directory, or should we disallow them? Having them in the build directory does not bother me, but I'm not sure we should have them stored in the source directory. Implementing a loader that diverts the bytecode cache in a place disjoint from the source file is also an option, but seems a lot of work.

@lithomas1
Copy link
Member

lithomas1 commented Jan 29, 2023

Exploring the idea of using a custom loader to avoid having to run meson install. This works for Python modules. Supporting data files is possible but requires some more work.

Is there a reason we're avoiding meson install here? Seems a bit like we're re-inventing the wheel.

@lithomas1
Copy link
Member

lithomas1 commented Jan 29, 2023

Working on this one question arises: what should we do with the Python bytecode cache files? Should we allow them and store them next to the source files, either in the source or build directory, or should we disallow them? Having them in the build directory does not bother me, but I'm not sure we should have them stored in the source directory. Implementing a loader that diverts the bytecode cache in a place disjoint from the source file is also an option, but seems a lot of work.

I would prefer excluding them. They have caused trouble in the past in causing a bad magic number ImportError
(This was a bit of an uncommon situation, though, where a vendored module was deleted in favor a package on PyPI, but the .pyc file was still there).

@rgommers
Copy link
Contributor

Is there a reason we're avoiding meson install here?

I kind of have the same question, but from a slightly different perspective. The two things that are known to be problematic right now are running tests and setting breakpoints directly in an IDE. For both, my impression was that installing symlinks and pointing the IDE at the install dir is the one thing that will help. This rewrite is orthogonal to that, right? I don't have a good feel for these custom loaders, but they can't fix the IDE issue was my impression.

@dnicolodi
Copy link
Member Author

dnicolodi commented Jan 29, 2023

We avoid meson install because it creates more problems that it solves. There is nothing to reinvent here: the code that matches files from the source directory and the build directory to the module structure is trivial. Installing symlinks is not an option because it is not supported by the wheel format. What we can install is a module finder that invokes a symlink factory, but that has more complexity of the approach in this PR and most of the ones of the current install based approach. Note that the approach in this PR does not use a custom module loader, but a custom module finder, which then uses the regular Python module loader to load the module content.

I don't know how IDEs set breakpoints in source code, but assuming they work similarly as how plain pdb works, the symlink solution does not solve the problem with breakpoints, unless the IDE loads the file via the symlink name. The reason is that for setting a breakpoint on a file location, the file path needs to be relative to the Python path.

However, with this approach, when you step through the code in a debugger, you step through the real code and not a copy of it that is going to be obliterated at the next reload.

@dnicolodi
Copy link
Member Author

It would be very useful to have some input from IDE developers about what's their idea about supporting setting breakpoints in the source code of editable wheels.

@rgommers
Copy link
Contributor

Installing symlinks is not an option because it is not supported by the wheel format

That's not relevant here - the idea was to install symlinks directly; the wheel only contains the loader.

unless the IDE loads the file via the symlink name.

Indeed, that's the one thing that will work - edit the installed directory in the IDE (which then also edits the original sources, because symlink). Still not great, but should work.

It would be very useful to have some input from IDE developers about what's their idea about supporting setting breakpoints in the source code of editable wheels.

I'm not sure how to get that. IDE developers probably won't care. This is only a problem for editable wheels + out-of-place builds, which is rare.

@lithomas1
Copy link
Member

We avoid meson install because it creates more problems that it solves.

Can you elaborate more on this?

@dnicolodi
Copy link
Member Author

We avoid meson install because it creates more problems that it solves.

Can you elaborate more on this?

Well, the biggest problem I see is that it breaks the fundamental promise of editable wheels, namely that the code that is execute is the code that you are editing inside your project. With editable installations based on an installed tree, the code being executed is a copy of the code you are editing. One major consequence is that if you step through the execution in a debugger in your IDE the source code you see is not the source code in your project. If you quit the debugger and modify the code that you are staring at it in the IDE, your edits will be lost the next time you execute your code.

Another class of problems is keeping the installed tree and the source tree synchronized. Either you wipe the installed tree at every rebuild as you propose in #282 or you need some more complex form of file tracking. For small projects this is maybe not an issue, but for things like SciPy is a major slowdown: a Python code only change takes a few hundred of milliseconds to be rebuild (just the time for ninja to recognize that there is nothing to do) but installing takes several seconds.

Finally, the current code that uses the installed tree for the editable install is much more complex than the code that does not use it and does not suffer form the two class of problems above.

If you can live with the two class of problems above, you maybe don't even need editable wheels: simply integrate building a regular wheel and installing it into your test runner. Quickly clobbered together and untested make solution:

PYTHON ?= python
test:
    $(PYTHON) -m build --no-isolation --wheel -Cbuilddir=build
    $(PYTHON) -m pip install dist/*.whl
    $(PYTHON) -m pytest tests/

@dnicolodi dnicolodi force-pushed the editable branch 2 times, most recently from fcdf3f7 to aa61dad Compare February 28, 2023 15:44
@dnicolodi dnicolodi mentioned this pull request Mar 1, 2023
@dnicolodi dnicolodi force-pushed the editable branch 4 times, most recently from 91a452f to b61cc9e Compare March 1, 2023 12:58
@dnicolodi
Copy link
Member Author

I don't think releasing 0.13 with one implementation of the editable wheel support and switch to another implementation for 0.14 makes much sense, also considering that editable wheels support is the highlight of 0.13. Everyone agrees that this approach is the one that should be taken. Still I would like someone else to have a look at the implementation.

@dnicolodi dnicolodi added this to the v0.13.0 milestone Mar 1, 2023
Copy link
Member

@lithomas1 lithomas1 left a comment

Choose a reason for hiding this comment

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

LGTM. This works like a charm for pandas now and tests seem to be comprehensive enough.
Thanks for putting this up!

Maybe @rgommers or @FFY00 can comment more on the implementation, though.

@FFY00
Copy link
Member

FFY00 commented Mar 4, 2023

Okay, I'll try to finish my review this week, sorry for the delay.

@dnicolodi
Copy link
Member Author

@lithomas1 did you have a chance to try this is combination with setting break-points in an IDE?

@lithomas1
Copy link
Member

@lithomas1 did you have a chance to try this is combination with setting break-points in an IDE?

Yeah, looks like it works perfectly there.

mesonpy/__init__.py Outdated Show resolved Hide resolved
mesonpy/_editable.py Outdated Show resolved Hide resolved
mesonpy/_editable.py Show resolved Hide resolved
mesonpy/_editable.py Show resolved Hide resolved
mesonpy/_editable.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/test_editable.py Outdated Show resolved Hide resolved
tests/test_editable.py Outdated Show resolved Hide resolved
mesonpy/_editable.py Outdated Show resolved Hide resolved
mesonpy/_editable.py Outdated Show resolved Hide resolved
Running with Python 3.9 allows mypy to check our importlib.resource
interfaces. mypy 0.991 does a better job in understanding typing in
the editable support code.
@dnicolodi dnicolodi force-pushed the editable branch 2 times, most recently from 78fb86e to 23e275c Compare March 11, 2023 21:20
This implementation uses a .pth file to install a custom module
finder.  The module finder uses the Meson installation plan
introspection data to build a virtual view of how files would be laid
out in a wheel and looks up the modules from there.

Not having to copy files from the source or build directory to the
installation directory, rebuilds are faster. Executing directly from
the build and source directory is more true to the spirit of editable
wheels. There is no risk for the user to accidentally edit the code in
the installation directory and have their edits to be overwritten at
the next reload.

I haven't verified this, but this approach has also the potential to
allow to set debugger breakpoints from the code in the source
directory and have them work at execution time (this should work if
the code is imported before being executed).
We support typing only with Python >= 3.9.
This avoids cluttering the source directory with bytecode cache. It
should also avoid surprising behavior when the source is removed from
version control but the bytecode cache is still present in the working
directory.
@FFY00
Copy link
Member

FFY00 commented Mar 11, 2023

@dnicolodi please do not merge pull requests after a review before at least giving the reviewers a chance to reply to feedback and/or re-review the parts that changed.

@dnicolodi
Copy link
Member Author

Sorry. The last time you said that one round of review was enough. I felt like all your comments have been addressed. I must have misunderstood.

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

Successfully merging this pull request may close these issues.

5 participants