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

Fix 54 #68

Closed
wants to merge 9 commits into from
Closed

Fix 54 #68

wants to merge 9 commits into from

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Jun 6, 2022

Fix #54

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@maresb
Copy link
Contributor Author

maresb commented Jun 6, 2022

@conda-forge-admin, please rerender

@brandonwillard
Copy link
Member

brandonwillard commented Jun 6, 2022

What are these changes supposed to do? Nevermind, I just noticed the other, related PR.

@maresb
Copy link
Contributor Author

maresb commented Jun 6, 2022

@brandonwillard this is unfortunately a stab in the dark. I don't really understand the problem or have any real ideas on how to fix them. Do you have any advice?

@brandonwillard
Copy link
Member

brandonwillard commented Jun 6, 2022

@brandonwillard this is unfortunately a stab in the dark. I don't really understand the problem or have any real ideas on how to fix them. Do you have any advice?

#54 has more to do with the Aesara initialization/config process, which uses the noisy numpy.distutils.system_info.get_info function to get some build environment information.

The warnings like

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

emitted by that NumPy function shouldn't actually be signs of any errors, because they're referencing toolchain components that aren't relevant to Aesara.

The warning

WARNING (aesara.tensor.blas): Using NumPy C-API based implementation for BLAS functions.

is the only relevant one, but it depends on a confluence of external packages in one's Conda environment that has been known to change and/or be somewhat flaky across package versions.

Even when a combination of packages is known to work, I recall there being some challenges with constructing a single, general recipe that covers everything (e.g. the Conda recipe spec doesn't make it easy to condition on non-trivial OS, requirement, and version combinations).

@maresb
Copy link
Contributor Author

maresb commented Jun 6, 2022

Ah, that's great to know! So you're saying that these warnings can be simply ignored?

I was concerned by the slowdown reported by @michaelosthege, but if I understand correctly then the slowdown was caused exclusively by the NumPy C-API error.

@brandonwillard
Copy link
Member

brandonwillard commented Jun 6, 2022

Ah, that's great to know! So you're saying that these warnings can be simply ignored?

Yes, the first set of warnings can be ignored. It would be great if we could remove them altogether by capturing/filtering them in the Aesara initiation process, though. Those steps are here.

I was concerned by the slowdown reported by @michaelosthege, but if I understand correctly then the slowdown was caused exclusively by the NumPy C-API error.

Yes, that's the most likely cause, and that's where some updates/changes to this recipe could help. Last I tried, I was able to create a Conda environment with correctly linked BLAS libraries using these instructions, but, as #54 implies, that might've changed (e.g. due to external package changes, or the detection code in Aesara linked above, etc.).

Otherwise, potential errors aside, we could consider adding the m2w64-toolchain requirement to Windows installs, as alluded to in #54.

@maresb
Copy link
Contributor Author

maresb commented Jun 6, 2022

Yes, the first set of warnings can be ignored. It would be great if we could remove them altogether by capturing/filtering them in the Aesara initiation process, though. Those steps are here.

Ah, ok, this seems like it should be straightforward to implement. I'll try to submit a quick patch.

Yes, that's the most likely cause,

That's great to know! That should let me quickly resolve this upstream in pymc-feedstock! 😄

In the meantime I'll try running a few more tests to see if any of those Conda packages helps...

consider adding the m2w64-toolchain...as alluded to in #54.

But @michaelosthege says:

Including m2w64-toolchain doesn't help with these warnings.

I'm trying it out anyways...

@maresb
Copy link
Contributor Author

maresb commented Jun 6, 2022

PR which (hopefully) suppresses warnings submitted here: aesara-devs/aesara#980

@maresb
Copy link
Contributor Author

maresb commented Jun 6, 2022

@conda-forge-admin, please rerender

Since we're only changing the tests, we don't want to build a new
package.
@maresb
Copy link
Contributor Author

maresb commented Jun 6, 2022

@conda-forge-admin, please rerender

@maresb
Copy link
Contributor Author

maresb commented Jun 6, 2022

@brandonwillard I accomplished my goal of cleaning up the downstream pymc feedstock here: conda-forge/pymc-feedstock#42

The tests I added here are not strictly necessary, but could be quite useful for catching undesired warnings.

In case you're not interested, go ahead and close out this PR for me. Thanks!

@maresb maresb marked this pull request as ready for review June 6, 2022 21:41
@maresb
Copy link
Contributor Author

maresb commented Jul 3, 2022

Supserseded by #77

@maresb maresb closed this Jul 3, 2022
@maresb maresb deleted the fix-54 branch July 3, 2022 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NumPy compiler warnings on Windows
3 participants