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

Only pin theano-pymc in requirements.txt #4322

Merged
merged 16 commits into from
Dec 11, 2020

Conversation

MarcoGorelli
Copy link
Contributor

closes #4320

@MarcoGorelli MarcoGorelli changed the title Pin theano Check theano-pymc pins match Dec 10, 2020
@@ -0,0 +1,131 @@
# BSD 3-Clause License
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is mainly taken from pandas, so my understanding is that I need to include the bsd license

@MarcoGorelli MarcoGorelli requested a review from twiecki December 10, 2020 14:23
@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

Merging #4322 (1e970ea) into master (4fd56fd) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4322      +/-   ##
==========================================
- Coverage   87.56%   87.54%   -0.02%     
==========================================
  Files          88       88              
  Lines       14270    14272       +2     
==========================================
  Hits        12495    12495              
- Misses       1775     1777       +2     
Impacted Files Coverage Δ
pymc3/distributions/multivariate.py 82.75% <0.00%> (-0.58%) ⬇️
pymc3/distributions/posterior_predictive.py 89.01% <0.00%> (-0.29%) ⬇️
pymc3/distributions/simulator.py 82.43% <0.00%> (+0.24%) ⬆️
pymc3/distributions/distribution.py 95.03% <0.00%> (+0.75%) ⬆️

- scipy>=0.18
- sphinx-autobuild>=0.7
- sphinx>=1.5
- theano-pymc=1.0.12
Copy link
Member

Choose a reason for hiding this comment

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

I thought the point is that we can drop this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how to drop it, I don't think it can be substituted from somewhere else. Suggestions?

So, in lieu of that, we can have a little script which checks they're all pinned to the same version

Copy link
Member

Choose a reason for hiding this comment

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

Why not just defer to what's in requirements.txt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in, leave it out of here and then do pip install -e .?

Sure, could do, but my understanding was that the recommended way of installing theano was via conda

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked this again and the docs read

We only support the installation of the requirements through conda.

So, as long as we install mlk-service via conda, then we're OK to install theano-pymc via pip? If so I'll do that, it's easier than adding extra scripts/checks

@MarcoGorelli MarcoGorelli changed the title Check theano-pymc pins match Only pin theano-pymc in requirements.txt Dec 10, 2020
CONTRIBUTING.md Outdated
@@ -15,6 +15,8 @@ We appreciate being notified of problems with the existing PyMC code. We prefer

Please verify that your issue is not being currently addressed by other issues or pull requests by using the GitHub search tool to look for key words in the project issue tracker.

Filter on the "beginner friendly" for issues which are good for new contributors.
Copy link
Member

Choose a reason for hiding this comment

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

add a link

- id: pip-to-conda
additional_dependencies: [pyyaml]
entry: python scripts/generate_pip_deps_from_conda.py
files: ^(conda-envs/environment-dev-py38\.yml|requirements-dev\.txt)$
Copy link
Member

Choose a reason for hiding this comment

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

why not do this for 36 and 37 files too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was originally thinking that there could be a single conda file which generates the requirements-dev file, but there's no reason why they shouldn't all generate it

Sure, done 👍

@@ -1,4 +1,4 @@
# This file is auto-generated from environment-dev-py38.yml, do not modify.
# This file is auto-generated from by scripts/generate_pip_deps_from_conda.py, do not modify.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# This file is auto-generated from by scripts/generate_pip_deps_from_conda.py, do not modify.
# This file is auto-generated by scripts/generate_pip_deps_from_conda.py, do not modify.

@twiecki twiecki merged commit eb4be99 into pymc-devs:master Dec 11, 2020
@twiecki
Copy link
Member

twiecki commented Dec 11, 2020

Thanks!

@MarcoGorelli MarcoGorelli deleted the pin-theano branch December 11, 2020 13:57
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.

Theano dependency is pinned in 4 places
2 participants