Editable install: do not show any output in verbose mode when there is no work to do #579

lesteve · 2024-02-22T09:24:16Z

Why the current behaviour is problematic

The "ninja: no work to do" output can be confusing as noted for example by @ogrisel in scikit-learn/scikit-learn#28040 (comment). @eli-schwartz mentioned that it could potentially be changed in meson-python scikit-learn/scikit-learn#28040 (comment).

Personally, when working on scikit-learn code, I find it reassuring to see some output if the sklearn import takes a while and get the feed-back that some compilation is happening. In the case when there is nothing to do I would rather have nothing printed. One thing I realised recently is that the "no work to do" output can be printed multiple times if you use subprocesses (makes complete sense since sklearn is imported in each subprocess). This can happen easily in scikit-learn for example with this snippet.

# /tmp/test.py
from sklearn.linear_model import LogisticRegression
from sklearn.datasets import make_classification
from sklearn.model_selection import cross_val_score

model = LogisticRegression()
X, y = make_classification()

print(cross_val_score(model, X, y, n_jobs=4))

Output is:

❯ python /tmp/test.py
+ /home/lesteve/micromamba/envs/scikit-learn-dev/bin/ninja
ninja: no work to do.
+ /home/lesteve/micromamba/envs/scikit-learn-dev/bin/ninja
+ /home/lesteve/micromamba/envs/scikit-learn-dev/bin/ninja
+ /home/lesteve/micromamba/envs/scikit-learn-dev/bin/ninja
+ /home/lesteve/micromamba/envs/scikit-learn-dev/bin/ninja
ninja: no work to do.
ninja: no work to do.
ninja: no work to do.
ninja: no work to do.
[1.   0.85 0.9  0.95 0.95]

Current implementation

The simplest thing I could find was to first execute the ninja command in dry-run mode and to check if "no work to do" is in the captured stdout. This may be a bit brittle in case the ninja output changes. Maybe there is a better way to check with ninja that there is no work to do?

There does not seem to be any test right now for the editable verbose behavior, but I can try to add some if you think this is worth it.

Alternative implementation

If you think the overhead of having a ninja dry-run command is too much overhead, one possible alternative implementation would be to capture stdout with p = subprocess.Popen(..., stdout=subprocess.PIPE) and look at the first two lines of output. In my experience, this kind of manipulation are always more complicated than you would hope, but maybe I have been doing it wrong all this time ...

The kind of complications I have in mind:

p.stdout.read is blocking so in the case that there is no output for some reason, it will wait for ever. Maybe we know there will always be some output with the ninja command. You can also put you can use fcntl to put stdout in non-blocking mode.
displaying the captured stdout live (i.e. without waiting for the command to finish) is also always more complicated than you would hope:
- decide whether to read line by line, a few lines, a number of bytes
- decide whether to sleep between two reads (I have seen some cases where not sleeping had a high CPU usage)
- you also need to remember to redisplay all the remaining stdout once the command finishes otherwise you miss some stdout.

… work to do

ogrisel · 2024-02-22T14:06:39Z

I think the lines:

+ /home/lesteve/micromamba/envs/scikit-learn-dev/bin/ninja
ninja: no work to do.

would be ok, if they were prefixed by some user friendlier message:

meson-python: importing sklearn installed in editable mode
+ /home/lesteve/micromamba/envs/scikit-learn-dev/bin/ninja
ninja: no work to do.

But I agree that the repeated message is a bit annoying when using parallelism that spawn Python workers that import sklearn concurrently.

ogrisel · 2024-02-22T14:09:25Z

But I am also fine with what @lesteve suggests in this PR because it fixes both problems at once.

mesonpy/_editable.py

rgommers · 2024-02-22T20:55:20Z

Thanks! I agree that this message can be confusing. I hadn't seen it printed multiple times yet, but that makes it worse. So looks like a good idea to suppress it.

ogrisel · 2024-02-23T11:07:27Z

mesonpy/_editable.py

+            dry_run_build_cmd = self._build_cmd + ['-n']
+            p = subprocess.run(dry_run_build_cmd, cwd=self._build_path, env=env, capture_output=True)
+            if b'ninja: no work to do.' not in p.stdout and b'samu: nothing to do' not in p.stdout:
+                print('+ ' + ' '.join(self._build_cmd))


I find the build command not very informative because it does not reflect which editable install is being rebuilt, nor does it state that meson-python's editable install is in charge of making this build happen.

So maybe consider:

Suggested change

print('+ ' + ' '.join(self._build_cmd))

module_names = ", ".join(sorted(self._top_level_modules))

print(f"meson-python: building {module_names}")

Here is the resulting user experience:

$ touch sklearn/ensemble/_gradient_boosting.pyx $ python -c "import sklearn" meson-python: building sklearn [3/3] Linking target sklearn/ensemble/_gradient_boosting.cpython-311-darwin.so $ python -c "import sklearn" # nothing to rebuild => no further output $

Two questions:

do we want to keep the ninja build command for troubleshooting or are we fine not showing it?

do we want to keep the + at the beginning of the first line? I guess maybe this was a marker to make it stand out in the output?

I don't have an opinion on those points as long as the first message mentions "meson-python" and the name of the package(s) being rebuilt prior to displaying build outputs.

dnicolodi · 2024-02-23T12:14:46Z

mesonpy/_editable.py

-            stdout = None
+            # We do not want any output if there is no work to do. Code is adapted from:
+            # https://github.com/mesonbuild/meson/blob/a35d4d368a21f4b70afa3195da4d6292a649cb4c/mesonbuild/mtest.py#L1635-L1636
+            dry_run_build_cmd = self._build_cmd + ['-n']


This does not work on Windows, where the compilation command is meson compile, which does not accept a -n argument. This demonstrates that this feature needs tests.

tests/test_editable.py

mesonpy/_editable.py

lesteve · 2024-02-23T16:18:57Z

It would be great if someone with the necessary rights could enable the Github actions workflow 🙏!

I can reproduce the Alpine failure in a Docker container and the bad news is that ninja -n seems to segfault (ninja alone works fine). The ninja version is 1.9 (released January 2019) and I have seen at least one test in meson-python that is skipped if the ninja version < 1.10, so maybe this could be an option?

My basic gdb skills only allowed me to get this information:

(gdb) r
Starting program: /usr/bin/ninja -C build/cp312 -n
warning: Error disabling address space randomization: Operation not permitted
ninja: entering directory 'build/cp312'

Program received signal SIGSEGV, Segmentation fault.
0x00007b65464a9458 in strlen (s=<optimized out>, 
    s@entry=0x8 <error: Cannot access memory at address 0x8>) at src/string/strlen.c:17
warning: 17	src/string/strlen.c: No such file or directory

Suggestions how to get more info more than welcome ...

eli-schwartz · 2024-02-23T16:25:59Z

I can reproduce the Alpine failure in a Docker container and the bad news is that ninja -n seems to segfault (ninja alone works fine). The ninja version is 1.9 (released January 2019) and I have seen at least one test in meson-python that is skipped if the ninja version < 1.10, so maybe this could be an option?

This is:

michaelforney/samurai#66
michaelforney/samurai#81
michaelforney/samurai@fb61f22

Possibly something Alpine should backport...

lesteve · 2024-02-23T17:42:52Z

I have skipped the test on Alpine for now based on detecting musllinux.

To know whether the test pass on Windows it would be nice that someone enable Github workflows. Full disclosure: locally the Windows tests don't pass and I don't really know why yet.

rgommers · 2024-02-23T17:49:27Z

To know whether the test pass on Windows it would be nice that someone enable Github workflows.

Done. this can be a pain because the approve button has to be pressed after every push. If you want to submit a trivial PR to side-step this problem, I'm happy to merge that straight away (e.g, in the readme, change to use Meson to using Meson).

dnicolodi · 2024-02-26T07:41:10Z

mesonpy/_editable.py

+            if self._build_cmd[0] == 'meson':
+                # On Windows meson compile is used so to do a dry run you need
+                # --ninja-args
+                dry_run_build_cmd = self._build_cmd + ['--ninja-args="-n"']


The double quotes around -n are wrong, if the command is not interpreted by a shell (and it is not when run with subprocess.run()). Also, what happens when self._build_cmd already contains a --ninja-args argument?

Good catch the quoting was the issue indeed, now the test passes locally on a Windows VM.

About multiple --ninja-args, you probably know a lot more than me about this, so any suggestions welcome!

I don't know whether meson compile accepts multiple --ninja-args options. This is why I am asking. If I knew I would not have posed this as a question. You are the proponent of this feature, thus it is on you to verify and to find a solution.

I just tested this and meson compile does accept multiple --ninja-args arguments where the last argument overrides all the others.

For example, meson compile --ninja-args="-n" --ninja-args="-h" does the same as meson compile --ninja-args="-h".

It is not acceptable to discard the user supplied argument and keep only the -n.

Point taken, my current plan for this is to fine the last arguments in self._build_cmd that starts with --ninja-args and if there is one to append ,-n to it. This does seem a bit complicated though, let me know if you have a better suggestion!

In order to test this (at least locally), do you have an example in mind on Windows where ninja arguments would be already in the build command (i.e. before adding -n)?

My assumption is this would happen when users specify -C... options to the build frontend.

As @eli-schwartz says, you can pass install with pip install -e . -Ccompile-args=-j1 for example.

As for the implementation, it is ok to check if the component of the command starts with --ninja-args, then you need to parse the value passed to the option and add -n to the list. You can take some inspiration for how to do it from the code in this test

meson-python/tests/test_project.py

Lines 49 to 56 in b337334

# intercept and filter out test arguments and forward the call

if cmd[:2] == ['meson', 'compile']:

# when using meson compile instead of ninja directly, the

# arguments needs to be unmarshalled from the form used to

# pass them to the --ninja-args option

assert cmd[-1].startswith('--ninja-args=')

cmds.append(cmd[:2])

args.append(ast.literal_eval(cmd[-1].split('=')[1]))

I have changed the code to deal with existing and multiple --ninja-args in self._build_cmd but I haven't written a test for it yet.

mesonpy/_editable.py

tests/test_editable.py

…o editable-verbose-no-output-when-no-work-to-do

lesteve · 2024-02-29T06:19:35Z

If you want to submit a trivial PR to side-step this problem, I'm happy to merge that straight away (e.g, in the readme, change to use Meson to using Meson).

@rgommers I have opened #585 with this change.

rgommers · 2024-02-29T06:40:54Z

@rgommers I have opened #585 with this change.

Sounds good - merged now. Should fix the issue with CI running here.

dnicolodi · 2024-02-29T09:24:24Z

This is:

michaelforney/samurai#66 michaelforney/samurai#81 michaelforney/samurai@fb61f22

Possibly something Alpine should backport...

@eli-schwartz Are you implying that Alpine uses samurai but calls it ninja? I wonder if we can pull in a fixed package in our CI image...

dnicolodi · 2024-02-29T09:39:56Z

Yes, it seems that installing ninja on Alpine Linux puts the executable in a location outside $PATH and that the samuari package install a symlink that makes ninja actually be samurai. samurai seems to be installed with the build-base package, otherwise I don't understand why we get it in our CI images. Anyhow, installing ninja-is-really-ninja should make the ninja command actually be what is expected.

The bug @eli-schwartz mentions was fixed in samurai back in 2021, but samurai hasn't seen a release since 2020.

lesteve · 2024-03-04T11:10:37Z

The CI failures for the development meson version are also main and do not seem to be related to this PR.

Edit: the development meson builds seem to have been fixed. I don't really know what the freebsd issue is about but it does not seem related to this PR either, see build log

Failed to start an instance: INVALID_ARGUMENT: Not Found 404 Not Found POST https://compute.googleapis.com:443/compute/v1/projects/cirrus-ci-community/zones/us-central1-c/instances
{ "error": { "code": 404, "message": "The resource 'projects/freebsd-org-cloud-dev/global/images/family/freebsd-13-2' was not found", "errors": [ { "message": "The resource 'projects/freebsd-org-cloud-dev/global/images/family/freebsd-13-2' was not found", "domain": "global", "reason": "notFound" } ] } }

mesonpy/_editable.py

…o editable-verbose-no-output-when-no-work-to-do

dnicolodi · 2024-03-05T16:26:19Z

.github/workflows/tests.yml

@@ -116,7 +116,7 @@ jobs:

      - name: Run tests
        run: >-
-          python -m pytest --showlocals -vv --cov --cov-report=xml
+          python -m pytest --showlocals -vv --cov --cov-report=xml -s


This needs to be reverted

Yep I was about to do it 😉, done in my last commit.

dnicolodi · 2024-03-05T21:13:27Z

Please don't merge the main branch into your branch the next time: it makes rebasing the commits harder without reason.

dnicolodi · 2024-03-05T22:09:18Z

I added the missing tests and fixed up the last issue and submitted as #594

ENH: do not show any output in editable verbose mode when there is no…

6bc67f6

… work to do

lesteve force-pushed the editable-verbose-no-output-when-no-work-to-do branch from 85fb89b to 6bc67f6 Compare February 22, 2024 09:26

eli-schwartz reviewed Feb 22, 2024

View reviewed changes

mesonpy/_editable.py Outdated Show resolved Hide resolved

rgommers added the maintenance Regular code improvements that are not new features nor end-user-visible bugs label Feb 22, 2024

Reuse same code as in meson and add origin in comment

b73ad7e

ogrisel reviewed Feb 23, 2024

View reviewed changes

dnicolodi requested changes Feb 23, 2024

View reviewed changes

lesteve added 5 commits February 23, 2024 14:31

Add test

f7b3c00

Tweak output and test

779205f

Fix test on OSX

804f434

Simple quotes

875eeb5

More simple quotes

78a5ea9

dnicolodi reviewed Feb 23, 2024

View reviewed changes

tests/test_editable.py Outdated Show resolved Hide resolved

dnicolodi reviewed Feb 23, 2024

View reviewed changes

tests/test_editable.py Outdated Show resolved Hide resolved

dnicolodi reviewed Feb 23, 2024

View reviewed changes

mesonpy/_editable.py Outdated Show resolved Hide resolved

dnicolodi reviewed Feb 23, 2024

View reviewed changes

mesonpy/_editable.py Outdated Show resolved Hide resolved

lesteve added 4 commits February 23, 2024 15:11

Only check first two lines of output

69c0523

Tweak message and adapt test

ab97efa

Touching the file is enough to trigger compilation

0ae0795

Naive Windows attempt

4b67b30

lesteve added 2 commits February 23, 2024 18:22

Skip test on Alpine

d1eb4d0

Skip test on Alpine

e88169b

Check exit code in dry-run command

8cb12d7

dnicolodi reviewed Feb 26, 2024

View reviewed changes

mesonpy/_editable.py Outdated Show resolved Hide resolved

dnicolodi reviewed Feb 26, 2024

View reviewed changes

mesonpy/_editable.py Outdated Show resolved Hide resolved

dnicolodi reviewed Feb 26, 2024

View reviewed changes

tests/test_editable.py Outdated Show resolved Hide resolved

dnicolodi reviewed Feb 26, 2024

View reviewed changes

tests/test_editable.py Outdated Show resolved Hide resolved

lesteve added 2 commits February 29, 2024 07:07

Tackle comments

e2177e3

Merge branch 'main' of https://github.com/mesonbuild/meson-python int…

c4a60d4

…o editable-verbose-no-output-when-no-work-to-do

lesteve mentioned this pull request Feb 29, 2024

Minor README tweak #585

Merged

Touching the file is enough

2721e88

dnicolodi reviewed Mar 4, 2024

View reviewed changes

mesonpy/_editable.py Show resolved Hide resolved

lesteve added 8 commits March 4, 2024 14:25

Check platform rather than first command argument

89df78e

Merge branch 'main' of https://github.com/mesonbuild/meson-python int…

a6e336f

…o editable-verbose-no-output-when-no-work-to-do

Debug CI

8f025a6

Debug CI

1e310d5

Add test with verbose=True in main process

00680d0

Do not override existing ninja-args arguments on Windows

bc2f63c

Fix mypy

70e32ac

Fix Windows

674f813

dnicolodi reviewed Mar 5, 2024

View reviewed changes

lesteve added 2 commits March 5, 2024 17:27

Remove debug code

1557f58

Tweak module unloading in case of import error

ee8048e

dnicolodi closed this Mar 5, 2024

lesteve mentioned this pull request Mar 6, 2024

ENH: no output in editable verbose mode when there is no work to do #594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editable install: do not show any output in verbose mode when there is no work to do #579

Editable install: do not show any output in verbose mode when there is no work to do #579

lesteve commented Feb 22, 2024 •

edited

Loading

ogrisel commented Feb 22, 2024

ogrisel commented Feb 22, 2024 •

edited

Loading

rgommers commented Feb 22, 2024

ogrisel Feb 23, 2024 •

edited

Loading

rgommers Feb 23, 2024

lesteve Feb 23, 2024

ogrisel Feb 23, 2024

dnicolodi Feb 23, 2024

lesteve commented Feb 23, 2024

eli-schwartz commented Feb 23, 2024

lesteve commented Feb 23, 2024

rgommers commented Feb 23, 2024

dnicolodi Feb 26, 2024 •

edited

Loading

lesteve Feb 29, 2024

dnicolodi Feb 29, 2024

lesteve Mar 4, 2024

dnicolodi Mar 4, 2024

lesteve Mar 4, 2024

eli-schwartz Mar 4, 2024

dnicolodi Mar 4, 2024 •

edited

Loading

lesteve Mar 5, 2024

lesteve commented Feb 29, 2024

rgommers commented Feb 29, 2024

dnicolodi commented Feb 29, 2024

dnicolodi commented Feb 29, 2024

lesteve commented Mar 4, 2024 •

edited

Loading

dnicolodi Mar 5, 2024

lesteve Mar 5, 2024

dnicolodi commented Mar 5, 2024

dnicolodi commented Mar 5, 2024

	print('+ ' + ' '.join(self._build_cmd))
	module_names = ", ".join(sorted(self._top_level_modules))
	print(f"meson-python: building {module_names}")

	# intercept and filter out test arguments and forward the call
	if cmd[:2] == ['meson', 'compile']:
	# when using meson compile instead of ninja directly, the
	# arguments needs to be unmarshalled from the form used to
	# pass them to the --ninja-args option
	assert cmd[-1].startswith('--ninja-args=')
	cmds.append(cmd[:2])
	args.append(ast.literal_eval(cmd[-1].split('=')[1]))

Editable install: do not show any output in verbose mode when there is no work to do #579

Editable install: do not show any output in verbose mode when there is no work to do #579

Conversation

lesteve commented Feb 22, 2024 • edited Loading

Why the current behaviour is problematic

Current implementation

Alternative implementation

ogrisel commented Feb 22, 2024

ogrisel commented Feb 22, 2024 • edited Loading

rgommers commented Feb 22, 2024

ogrisel Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve commented Feb 23, 2024

eli-schwartz commented Feb 23, 2024

lesteve commented Feb 23, 2024

rgommers commented Feb 23, 2024

dnicolodi Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnicolodi Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve commented Feb 29, 2024

rgommers commented Feb 29, 2024

dnicolodi commented Feb 29, 2024

dnicolodi commented Feb 29, 2024

lesteve commented Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnicolodi commented Mar 5, 2024

dnicolodi commented Mar 5, 2024

lesteve commented Feb 22, 2024 •

edited

Loading

ogrisel commented Feb 22, 2024 •

edited

Loading

ogrisel Feb 23, 2024 •

edited

Loading

dnicolodi Feb 26, 2024 •

edited

Loading

dnicolodi Mar 4, 2024 •

edited

Loading

lesteve commented Mar 4, 2024 •

edited

Loading