-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Github Actions CI Setup #2634
Github Actions CI Setup #2634
Conversation
Removed cyordereddict recipe that we no longer depend on Removed strict bounds rewriting in setup.py. Now setup.py reads from requirements.in, which is not strict in its bounds Need to update case for pip-compile's output Filter out comments from lockfile, now that we have "via"
c33f7aa
to
8c69e05
Compare
8c69e05
to
ade2044
Compare
- name: Pip cache | ||
uses: actions/cache@v1 | ||
with: | ||
path: ~/.cache/.pip/pip_np$NUMPY_VERSION |
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.
Given that we're using a key
, I don't think we should need to encode the numpy_version
into the pip cache directory anymore.
TALIB_VERSION=$(cat ./etc/requirements.txt | grep "ta-lib" | sed "s/ta-lib==\([^ ]*\) *.*/\1/") | ||
CERTIFI_VERSION=$(cat ./etc/requirements.txt | grep "certifi" | sed "s/certifi==\([^ ]*\) *.*/\1/") | ||
|
||
if [[ "$RUNNER_OS" == "Linux" ]]; then | ||
sudo apt-get install python3-venv | ||
fi | ||
python3 -m venv testenv | ||
source ./testenv/bin/activate | ||
conda install -c anaconda libgfortran=3.0 | ||
pip install pip cython numpy pandas scipy ta-lib certifi -c etc/requirements.txt --cache-dir="$HOME/.cache/.pip/pip_np$NUMPY_VERSION" | ||
etc/dev-install --cache-dir="$HOME/.cache/.pip/pip_np$NUMPY_VERSION" | ||
pip freeze | sort | ||
echo "::set-env name=PATH::$PATH" |
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 seems like more complexity than I would have expected/hoped would be necessary. A couple quick suggestions:
- Given that we're using the
setup-python
action to make a fresh python for each of these builds, I don't think it should be necessary to create a separate virtualenv for the build. - I don't think we should need to use conda for anything here anymore. In particular, installing
anaconda
brings in a lot of stuff we don't want (anaconda is python + all the packages that anaconda the company curates, c.f. https://stackoverflow.com/questions/30034840/what-are-the-differences-between-conda-and-anaconda). - I don't think we want to be pre-installing all the packages you currently have listed in the top-level
pip install
. In particular, we probably don't want to be installingscipy
andpandas
at the same time as we installnumpy
, since the reason we pre-install numpy and Cython is so that those packages are available at build time for scipy and pandas (and other packages that depend on them at build time). I'd expect that the right sequence of installs here should look something like:pip install pip==<pinned pip version> setuptools=<pinned setuptools version>
,pip install -r etc/requirements_build.in -c etc/requirements.txt
,pip install -e .[all] -c etc/requirements.txt
.
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.
@ssanderson Sorry you caught this PR in a wip state but appreciate the feedback!
I set up a virtualenv to get around this error that I was running into when I tried to pip install directly to the actions host.
ERROR: Could not install packages due to an EnvironmentError: [Errno 13] Permission denied: '/usr/local/lib/python2.7/dist-packages/cython.py'
Consider using the `--user` option or check the permissions.
I tried to strip away the conda installs and as you warned me last week, pip doesn't handle the dependencies as well and I was running into a bunch of dependency constraint errors.
Setup for GH actions to replace Travis/Appveyor config.
Note: