Catch Numpy "Could not locate executable" warnings #980

maresb · 2022-06-06T20:42:49Z

Closes conda-forge/aesara-feedstock#54 by capturing stderr while reading blas_info from Numpy.

Specifically, the suppressed errors appear on Windows and look like

>>> import aesara
WARN: Could not locate executable g77
WARN: Could not locate executable f77
WARN: Could not locate executable ifort
WARN: Could not locate executable ifl
WARN: Could not locate executable f90
WARN: Could not locate executable DF
WARN: Could not locate executable efl

I am having trouble running the pytest suite locally. Additionally, I have no means of testing this locally since I don't have Windows.

I have not yet written any tests, but I'm open to advice.

I did this a bit rushed, so apologies in advance in case I missed some guideline.

Thank you for opening a PR!

Here are a few important guidelines and requirements to check before your PR can be merged:

There is an informative high-level description of the changes.
The description and/or commit message(s) references the relevant GitHub issue(s).
pre-commit is installed and set up.
The commit messages follow these guidelines.
The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

codecov · 2022-06-06T22:36:15Z

Codecov Report

Merging #980 (7a5b404) into main (be222f0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7a5b404 differs from pull request most recent head 6f3fadf. Consider uploading reports for the commit 6f3fadf to get more accurate results

@@            Coverage Diff             @@
##             main     #980      +/-   ##
==========================================
- Coverage   79.25%   79.25%   -0.01%     
==========================================
  Files         152      152              
  Lines       47882    47965      +83     
  Branches    10909    10923      +14     
==========================================
+ Hits        37949    38014      +65     
- Misses       7436     7445       +9     
- Partials     2497     2506       +9

Impacted Files	Coverage Δ
aesara/link/c/cmodule.py	`55.56% <100.00%> (+1.22%)`	⬆️
aesara/sparse/type.py	`72.11% <0.00%> (-2.66%)`	⬇️
aesara/link/numba/dispatch/tensor_basic.py	`97.93% <0.00%> (-2.07%)`	⬇️
aesara/tensor/shape.py	`90.93% <0.00%> (-1.29%)`	⬇️
aesara/tensor/subtensor_opt.py	`86.32% <0.00%> (-0.80%)`	⬇️
aesara/link/c/lazylinker_c.py	`65.95% <0.00%> (-0.71%)`	⬇️
aesara/link/c/cutils.py	`68.18% <0.00%> (-0.71%)`	⬇️
aesara/sparse/basic.py	`82.47% <0.00%> (-0.43%)`	⬇️
aesara/graph/features.py	`64.59% <0.00%> (-0.10%)`	⬇️
aesara/tensor/math.py	`89.72% <0.00%> (-0.08%)`	⬇️
... and 20 more

maresb · 2022-06-07T07:50:42Z

The CI looks good so far (except for that expected coverage error).

I don't get the impression that any of the CI tests are using Windows runners, and I don't have Windows myself, so I would be grateful if someone with Windows could check to see if my changes successfully suppress the warnings.

lucianopaz

I had a few questions about this that I did in the middle of the code. My suggestion for testing is that you mock numpy.distutils.system_info.get_info("blas_opt"). Something like:

def mocked_blas_opt(*args, **kwargs):
    print("This line won't be caught", sys.stdout)
    print("This line will be caught and re-printed", sys.stderr)
    print("Could not locate executable: this line will be caught and filtered", sys.stderr)
    return {'libraries': [], 'library_dirs': [], 'define_macros': [], 'include_dirs': []}


@patch(
    "numpy.distutils.system_info.get_info",
    new_callable=mocked_blas_opt
)
def test_blas_opt_warnings(mocked_sys):
    # Call the default_blas_ldflags and test that you get the expected warnings etc

lucianopaz · 2022-06-07T09:24:20Z

aesara/link/c/cmodule.py

+        @contextmanager
+        def catch_numpy_warnings():
+            with warnings.catch_warnings(record=True):
+                numpy.distutils.system_info.system_info.verbosity = 0  # side-effect


Should the previous value be cached and then reset after exiting the context?

I was wondering this myself. I think that resetting after exiting the context would be cleaner, but for the first step I wanted to preserve the current behavior of the code. (The code currently leaves this set to 0, so resetting it could lead to other previously suppressed warnings resurfacing.)

This is a really good point, but

fixing this would require changing the existing behavior, which risks breaking lots of other stuff, and

I don't need to touch this line in order to filter these "Could not locate executable" warnings.

Thus I propose we keep the code as-is for now, and address this in a subsequent PR. So that we don't forget, I opened #1005.

lucianopaz · 2022-06-07T09:28:03Z

aesara/link/c/cmodule.py

+            with warnings.catch_warnings(record=True):
+                numpy.distutils.system_info.system_info.verbosity = 0  # side-effect
+                sio = io.StringIO()
+                with redirect_stderr(sio):


Are we sure that numpy is printing to stderr and not to stdout?

I'm pretty sure, but not 100%. That's why I need someone with Windows to test this! 😂

Indeed, see #980 (comment).

Now fixed by filtering both stdout and stderr.

brandonwillard · 2022-06-07T09:54:57Z

Additionally, I have no means of testing this locally since I don't have Windows.

As mentioned in #974, we should probably use this as an opportunity to set up a Windows image run that performs these tests and limited subset of others.

maresb · 2022-06-10T22:14:37Z

I just tested this patch using Conda-Forge infrastructure, and unfortunately it doesn't work. 😞

Maybe it will work with stdout instead of stderr...

maresb · 2022-06-10T22:40:31Z

Oh, interesting... on Linux it goes to stderr, but on Windows it goes to stdout.

It should work if I filter both...

maresb · 2022-06-11T22:15:24Z

I managed to test this on the Conda-Forge infrastructure which includes Windows, and it appears to be working.

Passing tests: conda-forge/aesara-feedstock#72
Xfailing tests: conda-forge/aesara-feedstock#73

I'm ready for the next round of review. My thoughts:

I put the test inside test_printing.py. I'm not sure if this is the correct place.
Due to both Windows weirdness and some internals of Numpy, this ended up being more complicated than I originally expected. I'm curious if anyone has ideas to simplify things.

Thanks so much @lucianopaz for writing the tricky part of the test for me!

brandonwillard

We can add a Windows VM run to our test config and use that to test this, as well. See my comment here.

brandonwillard · 2022-06-15T10:51:45Z

aesara/link/c/cmodule.py

+        @contextmanager
+        def filter_numpy_missing_executable_warnings():


A contextmanager isn't necessary if it's only going to be used once/here (i.e. the yield can be replaced with the numpy.distutils.system_info.get_info call).

I agree the contextmanager is unnecessary. From a style perspective I find that it helps me to distinguish between the relevant action and scaffolding: I'm saying "here's the filter_numpy_missing_executable_warnings context for the following command" and then I run that command with the context manager.

But if you prefer without the contextmanager that's easy to fix.

I eliminated the contextmanager in d48f056. (It can be easily reverted in case you change your mind.)

brandonwillard · 2022-06-15T10:55:35Z

aesara/link/c/cmodule.py

+                # In case there are messages printed to both stdout and stderr,
+                # they may be incorrectly interspersed. But in the normal case
+                # there should be no output to either, so this should not pose
+                # a problem.
+                for sio, file in ((stdout_sio, sys.stdout), (stderr_sio, sys.stderr)):
+                    lines = sio.getvalue().splitlines()
+                    for line in lines:
+                        if all(
+                            f"Could not locate executable {exe}" not in line
+                            for exe in executables
+                        ):
+                            print(line, file=file)


I'm not entirely clear on what's happening here, but it reminds me that we should only suppress specific output messages and not all of them.

Hmm, taking a look at this with fresh eyes I see that it's indeed confusing.

To make the comment more accurate, I think I should say

In case there are messages ~~printed to~~ captured from both stdout and stderr, they may be print incorrectly interspersed. But in the normal case there should be no output to either, so this should not pose a problem.

I should probably also rename sio → captured_buffer and file → print_dest...

I'm not entirely clear on what's happening here

Does 0b89bc4 help? I improved the variable names and comments.

it reminds me that we should only suppress specific output messages and not all of them.

Agreed, I opened #1005 for this

tests/test_printing.py

brandonwillard · 2022-06-15T11:03:54Z

tests/test_printing.py

+
+
+@patch("numpy.distutils.system_info.get_info", new=mocked_blas_opt)
+def test_blas_opt_warnings():


There's a pytest fixture for catching stdin and stdout.

Thanks for the tip! Implemented in 6c6f615. That's a nice improvement.

maresb · 2022-06-15T12:19:11Z

We can add a Windows VM run to our test config and use that to test this, as well. See my comment here.

Sounds good, but I'm a bit confused about the conclusion...

It seems that the Windows VM isn't ready yet. Did you want me to look into something particular? Or are you implying that we should wait on that before merging?

Thanks for the review!!! I'll try to get to this in a few days.

maresb · 2022-06-19T10:47:23Z

The CI was running my tests on Ubuntu instead of Windows because I forgot to set the runs-on: variable to ${{ matrix.os }}. Fixed in d38b753.

maresb · 2022-06-19T10:48:24Z

I believe I have addressed all the issues from the previous review round. Any further thoughts?

Also, could you please approve the CI run?

brandonwillard

It looks like every matrix.include needs an explicit os entry now.

maresb · 2022-06-20T18:53:30Z

@brandonwillard let's see if e3d2feb fixes it

This reverts commit e3d2feb.

maresb · 2022-06-20T22:38:07Z

The scope of this PR is only conda-forge/aesara-feedstock#54, not #1005. (This PR got erroneously linked as closing the follow-up issue #1005.)

Setting a default for matrix.os didn't work, so I added a matrix.os entry for each include.

@brandonwillard could you please restart CI?

brandonwillard · 2022-06-21T01:23:14Z

The scope of this PR is only conda-forge/aesara-feedstock#54, not #1005. (This PR got erroneously linked as closing the follow-up issue #1005.)

This PR should necessarily address #1005.

brandonwillard · 2022-06-21T01:35:34Z

It looks like the shell scripting for the Windows cases needs to be adjusted.

maresb · 2022-06-24T21:09:20Z

I thought this PR would be a quick UX win for the poor souls stuck on Windows. My current proposal accomplishes my goal while making minimal changes to the existing behavior.

It looks like the shell scripting for the Windows cases needs to be adjusted.

It's very hard for me to add Windows to your CI testing framework given that I barely know Powershell (or whatever the CI runs with windows-latest), I don't have Windows myself, and I'm disallowed from triggering the CI myself as a "first-time contributor."

This PR should necessarily address #1005.

Fixing this seems to me like a major change to existing behavior which would reenable currently disabled Numpy warnings not only for Aesara itself, but also for all downstream projects which import Aesara.

Given how both of these points drastically expand the scope of this PR, I'm going to stop here on this one. I hope someone else can take it over, but if not, feel free to close it.

Moving on, I'm actually quite interested in delving into the mathematical side. @brandonwillard, I see that you have some interesting blog posts regarding DLMs and such, and you had the hope that with PyMC4+Aesara that this stuff would become easier. I was wondering if you have any examples which are working on the current stack? Or is this still work-in-progress?

maresb · 2022-07-02T10:46:24Z

In order to leave this PR in a clean state, I split out my CI attempt into #1029. Now all checks should be green.

Status update:

In order to make this easier for someone else to pick up, I want to summarize the current state.

The goal of this PR is to solve conda-forge/aesara-feedstock#54. This PR solves that goal, and adds the tests to prove it.

At the moment none of the CI tests for this repo run under Windows (#1028). However Conda-Forge does have a working Windows CI, and I have used that to confirm that these tests pass under Windows with my changes to cmodule.py (conda-forge/aesara-feedstock#72), and fail without those changes (conda-forge/aesara-feedstock#73).

@brandonwillard has made it clear that this PR won't be merged until #1005 and #1028 are solved. I'm stopping because in my view, those issues are independent of this PR, and I don't feel like I can effectively solve either of them.

Please let me know if there's anything further I can do to help here, short of solving #1005 or #1028.

brandonwillard · 2022-07-12T21:08:04Z

Now that #1050 is merged, let's see if the issue is still present.

maresb · 2022-07-13T14:23:52Z

Based on the tests at conda-forge/aesara-feedstock#92, it looks like that resolved this issue. Thanks @brandonwillard for the superior solution! Closing this one.

Catch Numpy "Could not locate executable" warnings

b6b80ee

This was referenced Jun 6, 2022

Fix 54 conda-forge/aesara-feedstock#68

Closed

NumPy compiler warnings on Windows conda-forge/aesara-feedstock#54

Closed

Fix skipped test for pymc package conda-forge/pymc-feedstock#42

Merged

twiecki requested a review from brandonwillard June 6, 2022 21:32

lucianopaz reviewed Jun 7, 2022

View reviewed changes

brandonwillard added CI Involves continuous integration Windows labels Jun 7, 2022

maresb added 4 commits June 10, 2022 22:20

Test warning suppression for default_blas_ldflags

46c1f28

Make filter more specific

adb0328

Set numpy verbosity=0 outside of context manager

825662b

Give context manager a better name

daad8af

maresb mentioned this pull request Jun 10, 2022

Test suppress windows warnings conda-forge/aesara-feedstock#72

Closed

5 tasks

maresb added a commit to maresb/aesara-feedstock that referenced this pull request Jun 10, 2022

Test aesara-devs/aesara#980

68cdf40

maresb mentioned this pull request Jun 10, 2022

Test suppress without patch xfail conda-forge/aesara-feedstock#73

Closed

5 tasks

maresb added 4 commits June 11, 2022 00:52

Filter both stdout and stderr

5d96d9e

Move numpy verbosity = 0 statement closer to comment

538c8b0

Update tests

97fbbbf

Satisfy Numpy's assumptions for stdout

b88a502

Merge branch 'main' into catch-numpy-warnings

95b3475

brandonwillard requested changes Jun 15, 2022

View reviewed changes

brandonwillard added the testing label Jun 15, 2022

brandonwillard marked this pull request as draft June 15, 2022 11:05

maresb added 3 commits June 19, 2022 11:55

Make use of capsys fixture in test

6c6f615

Improve docstring of NumpyCompatibleStdoutStringIO

65d7e36

Eliminate filter_numpy_missing_executable_warnings context

d48f056

maresb mentioned this pull request Jun 19, 2022

Numpy warnings should be filtered more precisely #1005

Closed

maresb marked this pull request as ready for review June 19, 2022 10:47

brandonwillard requested changes Jun 20, 2022

View reviewed changes

Default matrix.os to "ubuntu-latest"

e3d2feb

brandonwillard linked an issue Jun 20, 2022 that may be closed by this pull request

Numpy warnings should be filtered more precisely #1005

Closed

maresb added 2 commits June 21, 2022 00:31

Revert "Default matrix.os to "ubuntu-latest""

8bfb1ca

This reverts commit e3d2feb.

Add matrix.os for each include

c42f6f1

This was referenced Jul 2, 2022

Windows should be added to CI tests #1028

Open

Add Windows to CI tests #1029

Draft

maresb added 2 commits July 2, 2022 12:14

Revert all changes to .github/workflows/test.yml

f9badf3

Merge branch 'main' into catch-numpy-warnings

7a5b404

brandonwillard mentioned this pull request Jul 12, 2022

Replace distutils with setuptools #1050

Merged

Merge branch 'main' into catch-numpy-warnings

6f3fadf

maresb mentioned this pull request Jul 12, 2022

Test warnings with setuptools-based get_info conda-forge/aesara-feedstock#92

Closed

5 tasks

maresb closed this Jul 13, 2022

maresb deleted the catch-numpy-warnings branch August 3, 2022 21:56

maresb mentioned this pull request Aug 3, 2022

Revert numpy warnings workaround pymc-devs/pymc#6025

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch Numpy "Could not locate executable" warnings #980

Catch Numpy "Could not locate executable" warnings #980

maresb commented Jun 6, 2022 •

edited

Loading

codecov bot commented Jun 6, 2022 •

edited

Loading

maresb commented Jun 7, 2022

lucianopaz left a comment •

edited

Loading

lucianopaz Jun 7, 2022

maresb Jun 7, 2022

maresb Jun 19, 2022

lucianopaz Jun 7, 2022

maresb Jun 7, 2022

maresb Jun 19, 2022

brandonwillard commented Jun 7, 2022 •

edited

Loading

maresb commented Jun 10, 2022

maresb commented Jun 10, 2022

maresb commented Jun 11, 2022

brandonwillard left a comment

brandonwillard Jun 15, 2022

maresb Jun 15, 2022

maresb Jun 19, 2022

brandonwillard Jun 15, 2022

maresb Jun 15, 2022

maresb Jun 19, 2022

brandonwillard Jun 15, 2022

maresb Jun 19, 2022

maresb commented Jun 15, 2022 •

edited

Loading

maresb commented Jun 19, 2022

maresb commented Jun 19, 2022

brandonwillard left a comment •

edited

Loading

maresb commented Jun 20, 2022

maresb commented Jun 20, 2022

brandonwillard commented Jun 21, 2022

brandonwillard commented Jun 21, 2022

maresb commented Jun 24, 2022

maresb commented Jul 2, 2022

brandonwillard commented Jul 12, 2022

maresb commented Jul 13, 2022

		@contextmanager
		def filter_numpy_missing_executable_warnings():



		@patch("numpy.distutils.system_info.get_info", new=mocked_blas_opt)
		def test_blas_opt_warnings():

Catch Numpy "Could not locate executable" warnings #980

Catch Numpy "Could not locate executable" warnings #980

Conversation

maresb commented Jun 6, 2022 • edited Loading

codecov bot commented Jun 6, 2022 • edited Loading

Codecov Report

maresb commented Jun 7, 2022

lucianopaz left a comment • 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

brandonwillard commented Jun 7, 2022 • edited Loading

maresb commented Jun 10, 2022

maresb commented Jun 10, 2022

maresb commented Jun 11, 2022

brandonwillard left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maresb commented Jun 15, 2022 • edited Loading

maresb commented Jun 19, 2022

maresb commented Jun 19, 2022

brandonwillard left a comment • edited Loading

Choose a reason for hiding this comment

maresb commented Jun 20, 2022

maresb commented Jun 20, 2022

brandonwillard commented Jun 21, 2022

brandonwillard commented Jun 21, 2022

maresb commented Jun 24, 2022

maresb commented Jul 2, 2022

Status update:

brandonwillard commented Jul 12, 2022

maresb commented Jul 13, 2022

maresb commented Jun 6, 2022 •

edited

Loading

codecov bot commented Jun 6, 2022 •

edited

Loading

lucianopaz left a comment •

edited

Loading

brandonwillard commented Jun 7, 2022 •

edited

Loading

maresb commented Jun 15, 2022 •

edited

Loading

brandonwillard left a comment •

edited

Loading