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

OSX CI on Travis #2244

Merged
merged 6 commits into from Jul 12, 2018
Merged

OSX CI on Travis #2244

merged 6 commits into from Jul 12, 2018

Conversation

@richafrank
Copy link
Member

@richafrank richafrank commented Jul 11, 2018

OSX testing and packages

@richafrank richafrank requested review from ssanderson and freddiev4 Jul 11, 2018
Copy link
Contributor

@ssanderson ssanderson left a comment

@richafrank nothing blocking, but had a few small comments.

.travis.yml Outdated
- PANDAS_VERSION=0.18.1
- SCIPY_VERSION=0.17.1
matrix:
- PANDAS_18=1

This comment has been minimized.

@ssanderson

ssanderson Jul 11, 2018
Contributor

What do you think about calling this OLD_PANDAS and NEW_PANDAS instead of encoding versions? I think the long term model we'd like to have is to support a "stable" version of pandas for use on Quantopian, and a relatively new release for general consumption.

This comment has been minimized.

@richafrank

richafrank Jul 11, 2018
Author Member

I had this internal debate as well. I don't have a super strong opinion, but the two thoughts I had before going with the proposed vars were:

  1. When I go to the travis build matrix, I actually find it useful to know what versions are being tested, without having to click through to see that NEW=0.22.
  2. Will there be transition periods where we support three versions briefly, or do you think the cut-overs will cleanly update the NEW version? If we ever have three, then again, I think explicit versions will be clearer than OLD, NEW, NEWER.

This comment has been minimized.

@ssanderson

ssanderson Jul 11, 2018
Contributor

Will there be transition periods where we support three versions briefly, or do you think the cut-overs will cleanly update the NEW version?

I don't think it's going to be feasible for us to realistically support more than two build configurations given the resources we currently have. I think the most likely model for transitions will be that we'll either update OLD_PANDAS to match NEW_PANDAS, in which case they'll be the same for a bit (and maybe we comment out the NEW_PANDAS builds temporarily) or we update OLD_PANDAS to some newer version that's still not as new as the newest (e.g., if we want to bump the version on Quantopian but can't upgrade all the way to the latest for some reason).

When I go to the travis build matrix, I actually find it useful to know what versions are being tested, without having to click through to see that NEW=0.22.

I feel like we update pandas versions infrequently enough that it's not too hard to keep track of the OLD/NEW versions.

Overall, my instinct is that making it easier to just increase the NEW_PANDAS version will make it more likely that we actually keep the new version up to date, and that seems more valuable to me than the points you raised. I don't have a super strong opinion about this either though.

This comment has been minimized.

@richafrank

richafrank Jul 11, 2018
Author Member

Sounds good, and we can always change it later.

.travis.yml Outdated
cache:
directories:
- $HOME/.cache/.pip/

before_install:
- if [ ${CONDA_ROOT_PYTHON_VERSION:0:1} == "2" ]; then wget https://repo.continuum.io/miniconda/Miniconda2-4.3.30-Linux-x86_64.sh -O miniconda.sh; else wget https://repo.continuum.io/miniconda/Miniconda3-4.3.30-Linux-x86_64.sh -O miniconda.sh; fi
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then MINICONDA_OS=MacOSX; else MINICONDA_OS=Linux; fi

This comment has been minimized.

@ssanderson

ssanderson Jul 11, 2018
Contributor

Thoughts on moving this into a script like etc/travis_before_install.sh? Writing bash in a yaml list seems strictly worse to me than writing it in a regular file that we invoke from the yaml.

This comment has been minimized.

@freddiev4

freddiev4 Jul 11, 2018
Contributor

+1 on this

This comment has been minimized.

@richafrank

richafrank Jul 11, 2018
Author Member

What do you think about breaking out the different pieces of this section into ci/travis/install_miniconda.sh and ci/travis/overwrite_requirements.sh as opposed to one bigger script?

This comment has been minimized.

@ssanderson

ssanderson Jul 11, 2018
Contributor

That seems fine to me as well. Having a few lines in the yaml doesn't bother me. It just seems a bit silly to have a full blown bash script here.

.travis.yml Outdated
- cat etc/requirements.txt

This comment has been minimized.

@ssanderson

ssanderson Jul 11, 2018
Contributor

Same question here for moving this into its own file instead of writing a bunch of bash inline.

.travis.yml Outdated
- SCIPY_VERSION=0.17.1
matrix:
- PANDAS_18=1
- PANDAS_22=1

This comment has been minimized.

@freddiev4

freddiev4 Jul 11, 2018
Contributor

What does the syntax mean here? Does settingPANDAS_22 to 1 generate an environment variable?

This comment has been minimized.

@richafrank

richafrank Jul 11, 2018
Author Member

Yup, exactly. It sets that env var.

@richafrank richafrank force-pushed the and-osx branch 2 times, most recently from 642dc85 to 2e2665e Jul 11, 2018
@richafrank richafrank force-pushed the and-osx branch from 2e2665e to 907616b Jul 11, 2018
@richafrank
Copy link
Member Author

@richafrank richafrank commented Jul 11, 2018

Updated.

Copy link
Contributor

@ssanderson ssanderson left a comment

lgtm

@richafrank richafrank merged commit 4eff14d into master Jul 12, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@richafrank richafrank deleted the and-osx branch Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.