Remove pip from scripts and use only conda

[aseyboldt]

We should not mix conda packages from defaults with conda-forge during testing, and should not use pip with conda.

[codecov]

Codecov Report

Merging #3993 into master will decrease coverage by 0.23%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3993      +/-   ##
==========================================
- Coverage   86.79%   86.56%   -0.24%     
==========================================
  Files          88       88              
  Lines       14143    14020     -123     
==========================================
- Hits        12276    12136     -140     
- Misses       1867     1884      +17     

Impacted Files	Coverage Δ
pymc3/distributions/simulator.py	57.89% <0.00%> (-28.78%)	⬇️
pymc3/__init__.py	93.33% <0.00%> (-6.67%)	⬇️
pymc3/smc/smc.py	93.75% <0.00%> (-5.75%)	⬇️
pymc3/parallel_sampling.py	85.76% <0.00%> (-2.60%)	⬇️
pymc3/distributions/timeseries.py	71.20% <0.00%> (-0.74%)	⬇️
pymc3/distributions/posterior_predictive.py	88.79% <0.00%> (-0.51%)	⬇️
pymc3/sampling.py	86.02% <0.00%> (-0.34%)	⬇️
pymc3/distributions/multivariate.py	78.13% <0.00%> (-0.05%)	⬇️
pymc3/gp/cov.py	97.96% <0.00%> (ø)
pymc3/smc/sample_smc.py	88.15% <0.00%> (+0.20%)	⬆️
... and 1 more

[aseyboldt]

This is ready for review.
The tests don't pass because it seems I broke travis caching by disabling and enabling it in the PR, but the tests did pass before the last commit that re-enabled caching. This shouldn't be a problem if we merge, although we will have to remote the cache on master through the travis settings so that the miniconda environment is recreated.

[AlexAndorra]

Looks good, except for the removal of black_nbconvert in dev requirements. I suppose you did that because it can't be installed through any other channel than pip?
It seems to me that we should be able to require packages that are only installable via pip though. Thoughts?

[AlexAndorra]

Seems like you removed black_nbconvert? It's needed for NB styling

[AlexAndorra]

Now that Travis cache issue is resolved, couldn't you add back black_nbconvert to the file?

[aseyboldt]

Ah that's where the black_nbconvert is coming from. Guess I messed up the grep :-)
Would it be ok if we add this in a second PR again, so that we can avoid making the caching issue even more confusing?

[AlexAndorra]

Ha ha, that's fine by me (I don't completely understand this caching issue TBH). We will probably need to install black_nbconvert with pip though, as I don't think there is a conda port. Le ven. 3 juil. 2020 à 19:18, Adrian Seyboldt <[email protected]> a écrit :

…

Ah that's where the black_nbconvert is coming from. Guess I messed up the grep :-) Would it be ok if we add this in a second PR again, so that we can avoid making the caching issue even more confusing? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3993 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHIJMTGVJIVHJG52MWUZEJ3RZYHFBANCNFSM4OPSLWLQ> .

[aseyboldt]

Yes, if there is no conda package then I guess we'll have to use pip :-)

[AlexAndorra]

@aseyboldt there is a conflict in requirements.txt now: contextvars is in your branch whereas master doesn't have it. Can I remove contextvars to resolve the conflict?
And IIUC you're gonna solve the caching issue on Travis after this PR is merged, so I can merge even if tests are red?

[aseyboldt]

The conflict is fixed. I removed the cache on master (also to get the second py3.6 fix merged). The tests should pass now I hope.

[AlexAndorra]

Test are failing because of these three packages:

PackagesNotFoundError: The following packages are not available from current channels:
  - nbsphinx[version='>=0.4.2']
  - sphinx-autobuild==0.7.1
  - watermark

I think it's expected, as watermark (used for NB styling) is only installable by pip and create_testenv.sh doesn't look there anymore -- it should probably do that in last resort?

[aseyboldt]

Turns out keras doesn't support py37 yet: keras-team/keras#13964
It is used in one example notebook: https://github.com/pymc-devs/pymc3/blob/master/docs/source/notebooks/convolutional_vae_keras_advi.ipynb
I guess if we removed it from the requirements-dev file, notebook rerendering would break for that notebook?

[AlexAndorra]

Yes it would, but we don't run NBs automatically -- just on demand before releases or (hopefully) more frequently and gradually now, given that NB is not a format that lends itself easily to batch runs. So, I don't think removing Keras is very problematic TBH, all the more so since this NB had already problems running IIRC (cc @michaelosthege).

[AlexAndorra]

Looks good now, thanks @aseyboldt ! Waiting for your answers to my last two comments and I'll merge then 😉

[aseyboldt]

I think we have to wait with this one for #4003, it will need a rebase after that.

[AlexAndorra]

Where are we on this @aseyboldt ? I think you can rebase now, and I'll merge when tests pass

[twiecki]

Ping @aseyboldt.