-
Notifications
You must be signed in to change notification settings - Fork 34
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
DEP minor version updates #64
Conversation
TensorFlow v1.12 doesn't work with Python 3.7 (see tensorflow/tensorflow#20517). Their imminent v1.13 release should work. I suggest this PR wait on that release, since it should 🤞 be in the next week or two. |
Just a quick note that this PR is going to resolve #63. |
Ok, this is now in a state where it should pass once the tensorflow release is out. Huge thanks to @shelbrudy for figuring out what was going on with conda! |
I removed the check on the number of packages coming from conda-forge. Is there any reason I should leave it in? It's a hardcoded number that has changed every time I've done a release, and I'm not sure what we're learning from it, since the conda-forge packages are listed in the previous test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're running into issues with pip/conda disagreements. Pip thinks it's uninstalling a conda package and installing a new package, and that doesn't work. I suggest we add the following test to make sure this gets caught automatically in the future:
-run:
name: Check for conda/pip conflicts
command: docker run ds-python /bin/bash -c "conda list|python -c \"import sys; from collections import Counter; duplicates = {k: v for k, v in Counter([l.split()[0] for l in sys.stdin.readlines() if not l.startswith('#')]).items() if v>1}; assert not duplicates, f'These packages are installed by both pip and conda: {duplicates}'\""
I've put suggestions for what to do with each individual package inline.
I'm okay with removing the check on number of conda-forge installed packages. There's historical reasons for that which I don't think apply any longer.
On versions -- I suggest that this be a new major version. We're upgrading from Python 3.6 to 3.7, and we know that at least one package (tensorflow) has compatibility issues there. I think a signifier for the Python version upgrade is also nice. It's not clear what semantic versioning means for a Docker image like this.
- psycopg2 2.6.2 -> 2.7.7 | ||
- pyarrow 0.8.0 -> 0.12.0 | ||
- pytest 3.1.3 -> 3.10.1 | ||
- python 3.6.4 -> 3.7.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python version update should be pulled out to a separate bullet under ### Changed
, so that it's obvious to people.
@@ -9,6 +9,52 @@ Version number changes (major.minor.micro) in this package denote the following: | |||
|
|||
## Unreleased | |||
|
|||
## [4.3.0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
## [4.3.0] | |
## [5.0.0] |
- boto=2.49.0 | ||
- boto3=1.9.86 | ||
- bqplot=0.11.5 | ||
- cython=0.29.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something in the list of conda packages has a click
dependency; conda is installing click=7.0.0
, which conflicts with civis-python's requirement of click~6
(civisanalytics/civis-python#286). Let's explicitly install click=6.7
here to avoid the conflict.
- cython=0.29.4 | |
- click=6.7 | |
- cython=0.29.4 |
- dropbox==7.1.1 | ||
- civis==1.9.3 | ||
- civisml-extensions==0.1.10 | ||
- cloudpickle==0.7.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a cloudpickle
v0.8.0 available on conda-forge, and conda is already installing it (I don't know where the dependency is). I suggest we move this from the pip to the conda section, and use v0.8.0.
- s3fs=0.2.0 | ||
- seaborn=0.9.0 | ||
- scipy=1.2.0 | ||
- scikit-learn=0.20.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incompatible with the latest civisml-extensions
(civisanalytics/civisml-extensions#45). I suggest holding this PR on an update of civisml-extensions, or else downgrading scikit-learn for now.
Closing in favor of #66. |
Lots of minor/micro version updates, including updating python to 3.7.
Closes #63.