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

Update conda env files #4757

Merged
merged 5 commits into from
Jun 17, 2021
Merged

Update conda env files #4757

merged 5 commits into from
Jun 17, 2021

Conversation

AlexAndorra
Copy link
Contributor

Ready for review and merge

While installing the new v4 to play around, I noticed the conda env files didn't include the dependencies from requirements.txt.

In particular, this PR now ensures ArviZ and Aesara are installed through conda-forge, to make sure there are no C-compiler issues when creating a new dev environment.

@ricardoV94
Copy link
Member

ricardoV94 commented Jun 9, 2021

We will probably soon change the scipy dependency to be >1.4.1, for instance in this WIP #4736. Do you want to change the conda dependency already, or it should be done in the same PR?

Relevant WIP commit: a9dbea5

@AlexAndorra
Copy link
Contributor Author

Yeah, let's do that here. Just pushed the change

@AlexAndorra
Copy link
Contributor Author

This is ready to merge @twiecki and @ricardoV94. The one test failure seems completely unrelated:

def test_find_MAP_issue_4488():
        # Test for https://github.com/pymc-devs/pymc3/issues/4488
        with Model() as m:
            x = Gamma("x", alpha=3, beta=10, observed=np.array([1, np.nan]))
            y = Deterministic("y", x + 1)
            map_estimate = find_MAP()
    
        assert not set.difference({"x_missing", "x_missing_log__", "y"}, set(map_estimate.keys()))
>       np.testing.assert_allclose(map_estimate["x_missing"], 0.2, rtol=1e-5, atol=1e-5)
E       AssertionError: 
E       Not equal to tolerance rtol=1e-05, atol=1e-05

@twiecki twiecki requested a review from MarcoGorelli June 17, 2021 13:54
@twiecki
Copy link
Member

twiecki commented Jun 17, 2021

Assigning @MarcoGorelli who has created these.

Copy link
Contributor

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, I noticed the conda env files didn't include the dependencies from requirements.txt.

Yeah, that's because when I made these the intention was to do

conda env create -f <environment file>

and then

pip install -e .

so that the theano / aesara would only be pinned in one location (requirements.txt)


see issue #4320 and PR #4322 for context

Copy link
Contributor

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection to this changing now though, esp if installing aesara via conda is better, just bringing up the previous issues to make sure development doesn't go in circles 😆

@AlexAndorra
Copy link
Contributor Author

Yeah it's much better to install aesara with conda, so we should base the requirements.txt on environment.yml.
But that's actually already the case, isn't it? I saw a pre-commit check that modified the pip file based on the conda file 🤔

@MarcoGorelli
Copy link
Contributor

IIRC that's just the requirements-dev.txt, not requirements.txt, as some devs (@rpgoldman , I think?) aren't using conda

@AlexAndorra
Copy link
Contributor Author

I see. This doesn't block merging though IMO, as those files are precisely mainly there for developers

@MarcoGorelli
Copy link
Contributor

yeah, no objection to merging this

@AlexAndorra AlexAndorra merged commit 6eaf9a0 into main Jun 17, 2021
@AlexAndorra AlexAndorra deleted the update-conda-envs branch June 17, 2021 16:01
@rpgoldman
Copy link
Contributor

IIRC that's just the requirements-dev.txt, not requirements.txt, as some devs (@rpgoldman , I think?) aren't using conda

I used to be using pip only, but had to give up, because I couldn't get mkl set up properly.

That said, using conda is still a mess, AFAICT, because for development we need to somehow conda install enough to bootstrap, but then install pymc3 and aesara from pip. I've never figured out how to do that without having pip accidentally install something I should have been getting from conda.

Is there a recipe for bootstrapping a development environment?

@MarcoGorelli
Copy link
Contributor

Does

conda env create -f conda-envs/environment-dev-py38.yml
conda activate pymc3-dev-py38

and then

pip install -e .

not work for you?

@twiecki
Copy link
Member

twiecki commented Jun 17, 2021 via email

@michaelosthege
Copy link
Member

Related: I'm making a dedicated environment file for Windows that matches the install instructions: #4779
Hopefully that'll fix the CI for v3. If it does we should cherry-pick these changes for v4 and get the Windows job back online.

@rpgoldman
Copy link
Contributor

rpgoldman commented Jun 17, 2021

In response to @MarcoGorelli and @twiecki I was trying to see if I could find authoritative and clear installation instructions.

When I go here: https://github.com/pymc-devs/pymc3 I see a link to http://docs.pymc.io/notebooks/getting_started which suggests installing PyMC3 using pip. My understanding is that this is a bad way to install, because it doesn't reliably get you a C++ compiler that will appropriately work with Theano/Aesara.

That dated "getting started" link appears above the links to how to install, potentially decoying new users down a bad path.

I don't see any discussion on any of the links from that page to a description of how to get a development environment/bleeding edge version up and running.

If one follows the link to the documentation at https://docs.pymc.io/ one is directed to install either from conda-forge or pip, with no warnings about the pitfalls of installing from pip.

The developer guide here https://docs.pymc.io/developer_guide.html doesn't say anything about how to set up a development environment.

So I think it's hard for people to know that they should be using the conda environment + a pip install to get started working with the bleeding edge.

@MarcoGorelli Your instructions fun successfully for me on MacOS Catalina, but doesn't the following message indicate that my environment is not properly using mkl?

In [2]: import aesara
WARNING (aesara.tensor.blas): Using NumPy C-API based implementation for BLAS functions.

P.S. this is a terrible warning message -- a novice will have no idea what it means, as witness this discussion on theano-users

@rpgoldman
Copy link
Contributor

rpgoldman commented Jun 17, 2021

There's also conda develop

@twiecki Not clear to me how I would use conda develop (the docs are pretty weak). Seems like installing pymc3 from source using conda develop is too stupid to install aesara. So it means that it's left as an exercise to the reader to install any dependencies for the conda develop package.

So it looks like one needs to do conda develop for both pymc3 and for aesara.

@twiecki
Copy link
Member

twiecki commented Jun 18, 2021

So it looks like one needs to do conda develop for both pymc3 and for aesara.

Yeah, you're right that's a short-coming.

@twiecki
Copy link
Member

twiecki commented Jun 18, 2021

@rpgoldman Do you have time to improve these installation instructions? Seems like you know what would have helped you best.

@rpgoldman
Copy link
Contributor

@twiecki I will be happy to have a try, but if you see my comment above, at least on MacOS, my best effort is still failing to get mkl incorporated correctly into the development environment. @MarcoGorelli -- any idea what I am doing wrong? If you will let me know, I will try to write up installation instructions.

@MarcoGorelli
Copy link
Contributor

I don't know, I only did the two steps above and it was fine

Some time ago I'd also done conda install pygpu, which is mentioned here, dunno if that'll help you

@rpgoldman
Copy link
Contributor

@MarcoGorelli -- I just checked with conda list, and it shows that I already have that installed, so the conda environment must require it.

Maybe aesara is not correctly configured to link up with mkl and clang in the environment?

@rpgoldman
Copy link
Contributor

BTW, I made a PR to pymc-examples to warn people that the installation instructions in "Getting Started" are obsolete and should be ignored (they refer to python 3.5!). See pymc-devs/pymc-examples#180 if you are interested.

@rpgoldman
Copy link
Contributor

rpgoldman commented Jun 18, 2021

Just to make sure I hadn't messed anything up with conda develop, I wiped my conda environment and reinstalled with:

conda env create --file conda-envs/environment-dev-py38.yml
conda activate pymc3-dev-py38
pip install . # in ~/src/pymc3 directory

Invoking import aesara in python gives the same warning:

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

so it is at least possible for that configuration to run without correctly integrating mkl into aesara and PyMC3.

Interestingly, aesara is installed by conda in the above set up, which means that it is conda's fault that mkl is not integrated properly. Could this be related:

(pymc3-dev-py38) rpg@RPG-MacBook-Pro: ~/src/pymc3 $ which clang
/Users/rpg/opt/anaconda3/envs/pymc3-dev-py38/bin/clang # looks good!
/usr/bin/clang
(pymc3-dev-py38) rpg@RPG-MacBook-Pro: ~/src/pymc3 $ which gcc
/usr/bin/gcc # maybe this is not good...

@rpgoldman
Copy link
Contributor

rpgoldman commented Jun 18, 2021

Never mind the last: the issue is that the conda environment does not automatically set the AESARA_FLAGS to use the mkl libraries. It's not clear to me how one determines what are the right flags for a platform. I guessed -lmkl and that seems to work, but it was strictly a wild guess.

From conversations on slack, sounds like this might be an issue with how aesara is guessing whether MKL is available. I will look at that and see if I can figure out why it wasn't guessing correctly on my install.

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.

6 participants