-
Notifications
You must be signed in to change notification settings - Fork 66
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
Editable install: raise ImportError when rebuilding fails #648
Conversation
Not raising an exception on failing to recompile is definitely not intended. I think it is a regression, actually. Most likely I missed this when reviewing the last patch that touched this code: I was sure that |
Thanks for your feed-back, indeed an |
This is what the error message looks like in my last commit:
|
The test is in a reasonable shape now I think. Something a bit not so great, you see the compilation error only if you are in verbose editable mode, so I added a note about this in the ImportError ... Not sure there is a straightforward way to improve the situation. It seems like you want both:
Maybe I miss something obvious but each time I have tried to do this in the past for both stdout and stderr, I ended up in tricky places. Maybe this is a new reason to have another stab at it. The kind of tricky things I remember off the top of my head, For completeness, right now the error looks like this. First part is the meson editable verbose output, second part in the exception raised by meson-python.
|
Hi @dnicolodi let me know if you have any further inputs on this! Of course I completely understand if you have other higher-priority things on your plate, no worries. I think this PR is already an improvement by making the exit code is non-zero when rebuilding fails. There is definitely some room for improvement, in particular the fact that the compilation error is only visible if Looking at the CI error they come from CirrusCI credit limits and don't seem to be related to this PR. |
I'm on holiday for a couple more weeks. I'll have a closer look when I'm back. |
c97aaff
to
9043123
Compare
I've squashed the commits, simplified the exception message, and rewritten the test to be more accurate. Please have a look. |
Thanks a lot, this looks fine to me! I quite liked the fact that there was some guidance in the ImportError message about being in editable verbose mode to have more details about the compilation error, but I guess you found it a bit messy maybe? To give more details: if the recompilation fails and I am not in editable verbose mode, the error with the current code in the PR is not very helpful. In case of error I do want to see the compilation error to figure out how to fix it. Having said that, I am always in editable verbose mode (except when I forget to set it), because I want to see some output when recompilation happens rather than wait without any output and wondering what is happening. The context is mostly scikit-learn, when you switch between branches the recompilation can take very easily tens of seconds to a 1-2 minutes. |
I don't like error messages that try to be little tutorials. Editable builds are a developer oriented feature and I would expect users of this feature to have at least a basic understanding of how it works. How to get compilation process output is described prominently in our documentation. The wording of the error message was also messy, indeed. But what is maybe more important is that it assumed that the exception message is displayed on the same output stream where the compilation error would be printed if editable wheel verbose mode is enabled. This is may not be true for a number of reasons. Python exceptions are printed on standard error, but ninja prints all its output on standard output, and the execution environment may be setup in such a way that the two are not displayed together. The python module may be imported into a web application and the developer observes the exception on a web framework error page. In these circumstances enabling verbose mode does nothing to show the compilation error to the developer. Having the exception message claim that enabling verbose mode will display the exception would be misleading. |
Use the correct build command to setup the editable package import and enable editable install verbose mode to make package rebuild errors easier to debug.
OK makes sense, thanks for the explanations! |
When using meson editable install, this can be very suprising that there is no obvious error when recompilation fails.
One use case I bumped into:
pytest sklearn/.../test_bla.py
that imports scikit-learn and should recompile when neededWhat happens:
The test run so I thought everything was fine, but actually the recompilation failed and I did not notice because
pytest
captures the output. If I had donepytest -s
and looked carefully at the output I may have noticedWhat I would expect:
There is an explicit error that tells me that recompilation failed.
With the current change, the tests run fine locally but there may be a side-effect I have not thought of?
This is how the output currently looks:
I will look at adding a test once I get some feed-back that this is something that looks reasonable.