Remove libblas, mkl and m2w64-toolchain dependencies #4932
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4932 +/- ##
=======================================
Coverage 78.41% 78.41%
=======================================
Files 130 130
Lines 24461 24461
=======================================
Hits 19182 19182
Misses 5279 5279
|
|
Should the aesara change be done first, else some tests will fail here? The test failures here, at least, look relevant |
|
@MarcoGorelli Yes, definitely. |
|
@MarcoGorelli What is |
That, and setting up a local dev environment |
|
Presumably we should leave |
why, would that not become a dependency of aesara? |
|
The recipe has I wonder if that takes care of it. |
Ideally yes, but right now the Aesara recipe doesn't even work. Conda create and activate output |
|
Thanks for testing it @michaelosthege. From what you post I can't tell what about it is crap? |
All these prints after the |
|
Hmmm, odd, the packages that it installs all look correct. What do you think is causing this? Can you actually run aesara successfully? |
No compilers, so more or less useless for any real work.. Conclusion: this part of the recipe you mentioned is not sufficient. |
|
Does that go away when you install |
|
It is surprising because it seems like a C++ compiler gets installed (as it should according the conda env). |
|
After installing |
|
Yeah, for mkl we would need to change the installation instructions to include |
No change in printouts/warnings after But it could be a different story if these dependencies are included in the recipe already. After all the environment was created kinda broken already. |
|
It still complains about missing |
|
Trying to add it here: conda-forge/aesara-feedstock#29 |
That brings it down to one warning: |
|
@michaelosthege I merged conda-forge/aesara-feedstock#29. Can you try installing aesara (once it's built) in a fresh env using |
|
Not perfect, but a a lot better than before: Full output |
|
I have tested the latest Create pm3v4 environment with specific packages (base) C:\Users\sreedatta>conda create -n pm3v4 -c conda-forge "python=3.9" libpython mkl mkl-service m2w64-toolchain numba python-graphviz scipy numpy blas libblas=*=*mkl aesara -y Installation DetailsCollecting package metadata (current_repodata.json): done Solving environment: donePackage Planenvironment location: C:\ProgramData\Anaconda3\envs\pm3v4 added / updated specs: The following packages will be downloaded: The following NEW packages will be INSTALLED: aesara conda-forge/win-64::aesara-2.1.3-py39h7fcc1c2_1 Downloading and Extracting Packages To activate this environment, use$ conda activate pm3v4To deactivate an active environment, use$ conda deactivate** Testing the Aesara Install** (pm3v4) C:\Users\sreedatta>python
Running the |
|
@twiecki @michaelosthege I continued my testing where I installed I could successfully run first example in the Getting Started page: http://docs.pymc.io/notebooks/getting_started I got the following error next when I tried to plot with Error: I could extract the last 5 values of the For the current iteration of |
|
Can you check what the difference in environment between 1 & 2 are? Ideally all 3 but 1 & 2 seem like the most surprising currently to have a difference. And can you check whether all of them have |
|
@twiecki I will post a comparison of the packages across 1 & 2, later this evening. I will check on |
|
@twiecki I have completed the comparison. Please note the following:
|
| @@ -10,10 +10,8 @@ dependencies: | |||
| - cloudpickle | |||
| - fastprogress>=0.2.0 | |||
| - h5py>=2.7 | |||
| - libblas=*=*mkl | |||
There was a problem hiding this comment.
What does this Syntax mean?
And why is the line only for Windows Python 3.8?
(Maybe add a comment inline)
There was a problem hiding this comment.
Oh wait, is there just one yml for Windows? (on my phone, sorry)
There was a problem hiding this comment.
That means that you want to use the mkl implementation of libblas, rather than e.g. openblas.
There was a problem hiding this comment.
I couldn't find any explanation of the =*=* operator, but it looks like this is the relevant docs section: https://conda-forge.org/docs/maintainer/knowledge_base.html#switching-blas-implementation
(Just leaving this here for reference.)
|
@MarcoGorelli I could use your input here. I want the conda env to not specify which blas implementation should be used, and not list |
|
Hey @twiecki
would it work to add it here ? |
|
@MarcoGorelli I don't think so, needs to be specified during installation of the conda packages. |
|
Maybe make separate conda env files for CI then, and specify them here ? I don't think |
|
Just reporting that the 3.9 env installs and runs well on my M1 Macbook under these changes. |
|
@MarcoGorelli I think custom files for the purest env is the way to go. Thanks! |
|
@MarcoGorelli OK I added additional test envs but I couldn't find where I would select that only those would be used on GH actions. |
|
I think you just need to use them here (and likewise for the others) |
|
OK, this should be done. |
| - myst-nb | ||
| - numpy=1.15 | ||
| - numpydoc>=0.9 | ||
| - pandas=0.24 |
There was a problem hiding this comment.
Do we really need to pin pandas to such an old version?
Below it doesn't even pin a version. Is this because of some Python 3.7 oddity?
There was a problem hiding this comment.
IIRC there was one env which would have the deps pinned to the earliest allowed versions
There was a problem hiding this comment.
and I believe that pinning minimum versions helps and speeds up the conda dependency resolver, right?
conda-envs/environment-test-py37.yml
Outdated
| - libblas=*=*mkl | ||
| - mkl-service | ||
| - myst-nb | ||
| - numpy=1.15 |
There was a problem hiding this comment.
Do we have a ticket about numpy version compatibility? Pinning to such an old version doesn't feel right.
Below it's a >= pin.
conda-envs/environment-test-py39.yml
Outdated
| - nbsphinx>=0.4 | ||
| - numpy>=1.15.0 | ||
| - numpydoc>=0.9 | ||
| - pandas>=0.24.0 |
There was a problem hiding this comment.
This is a third different way to pin pandas.
If these differences have a good reason we should probably comment that right in the file.
| - libpython | ||
| - mkl-service | ||
| - m2w64-toolchain | ||
| - numba |
There was a problem hiding this comment.
This is the hack to get compiler dependencies installed correctly. Can we remove it already, or is the Aesara recipe still broken?
|
@michaelosthege Tried to relax. |
|
Anything holding this one up? |
twiecki
force-pushed
the
remove_mkl
branch
2 times, most recently
from
71d01d5 to
5951ea5
Compare
|
Rebased and squashed. Can merge once CI passes. |
|
Not sure why is occurring. |
… does not compile anything, it should not have these dependencies. Instead, it is aesaras job to specify its compile-chain dependencies. Closes #4715. Add custom envs for testing. Remove patsy dependency.
As PyMC3 itself does not compile anything, it should not have these dependencies. Instead, it is aesaras job to specify its compile-chain dependencies. Closes #4715.
In addition to this change, we should make sure that
aesarainstalls all required dependencies for compilation (aesara-devs/aesara#564) as well as update our installation instructions to tell people to usemklif their processor supports it.