conda-forge / aesara-feedstock

A conda-smithy repository for aesara.
BSD 3-Clause "New" or "Revised" License
4 stars 9 forks source link

Fix 54 #68

Closed maresb closed 2 years ago

maresb commented 2 years ago

Fix #54

Checklist

conda-forge-linter commented 2 years ago

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 commented 2 years ago

@conda-forge-admin, please rerender

brandonwillard commented 2 years ago

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

maresb commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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! :smile:

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 https://github.com/conda-forge/aesara-feedstock/issues/54.

But @michaelosthege says:

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

I'm trying it out anyways...

maresb commented 2 years ago

PR which (hopefully) suppresses warnings submitted here: https://github.com/aesara-devs/aesara/pull/980

maresb commented 2 years ago

@conda-forge-admin, please rerender

maresb commented 2 years ago

@conda-forge-admin, please rerender

maresb commented 2 years ago

@brandonwillard I accomplished my goal of cleaning up the downstream pymc feedstock here: https://github.com/conda-forge/pymc-feedstock/pull/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 commented 2 years ago

Supserseded by #77