ENH: no output in editable verbose mode when there is no work to do #594
Conversation
dnicolodi
force-pushed
the
pr579
branch
6 times, most recently
from
0059471 to
27c5e3f
Compare
dnicolodi
force-pushed
the
pr579
branch
4 times, most recently
from
5bc6509 to
cc4d315
Compare
|
I had a quick look and it looks good, thanks for this! I had some small changes on top of #579 but it looks like your changes are simpler. |
|
I extended the tests further and improved the log message. I think logging the package name instead of the top level module names is much better. It requires passing one more argument to the loader instance, but I don't see any draw back in doing so. |
There was a problem hiding this comment.
From an end user point of view, the feedback looks good:
$ touch sklearn/ensemble/_gradient_boosting.pyx
$ python -c "import sklearn"
meson-python: building scikit-learn: /Users/ogrisel/miniforge3/envs/dev/bin/ninja
[3/3] Linking target sklearn/ensemble/_gradient_boosting.cpython-311-darwin.so
$ python -c "import sklearn" # no output when no work to do
Thanks very much for getting this through.
|
This one seems ready to be merged, cc @rgommers. Let me know is someone else is likely to review! Once this PR is merged it would be great if a release of meson-python could be made. There is not a huge rush, but together with #569 this would make it easier for scikit-learn to transition to Meson as our main build backend, see scikit-learn/scikit-learn#28506 for more details. |
|
The unwritten policy is that "significant" PRs by project maintainers need review by another maintainer. I suspect @rgommers is very busy with the upcoming NumPy 2.0 release. This is not a critical bug fix (from our point of view, at least), thus you'll need to be patient for a bit longer 🙂 |
|
Thanks for your answer, and indeed I completely understand that people have other priorities (I should have been more explicit about this in my previous message) ! |
|
@rgommers After thinking a bit more abut it, I'll be merging this PR before releasing 0.16.0. Unless you object, I'll be doing this before the end of the week. |
There was a problem hiding this comment.
Agreed, sorry for the long delay @dnicolodi & all. I have actually been using this PR for a while and everything seems to work like a charm. I had one pending review comment regarding line length, but that is certainly non-blocking and will likely be fine as is. So let's get this in.
Thanks again everyone!
| # We want to show some output only if there is some work to do | ||
| if self._work_to_do(env): | ||
| build_command = ' '.join(self._build_cmd) | ||
| print(f'meson-python: building {self._name}: {build_command}', flush=True) |
There was a problem hiding this comment.
I noticed that this line can get very long, with a longer package name plus a long absolute path. I think it's fine though; the absolute path is the least important part here. Only with something like Nix which has hashes in the path this may look a bit off. But let's leave it as is for now and see if anyone notices.
|
Great to see this one merged, thanks 🎉 |
No description provided.