Windows CI #269
Windows CI #269
Conversation
.github/workflows/build.yml
Outdated
| run: | | ||
| sudo apt install libgsl-dev # Needed for msprime < 1.0. Binary wheels include GSL for >= 1.0 | ||
| python -m pip install --upgrade pip | ||
| conda install msprime | ||
| pip install -r requirements.txt -r requirements-dev.txt |
There was a problem hiding this comment.
Should we install these also via conda?
There was a problem hiding this comment.
I'm not sure. What's the downside of doing it like this?
There was a problem hiding this comment.
@tomwhite not sure, but if we already use conda, why not use it all the way?
There was a problem hiding this comment.
I'd be in favour of using conda by default - it's good to test out CI on the conda infrastructure. I'd only use pip when conda packages aren't available.
.github/workflows/build.yml
Outdated
| python-version: ${{ matrix.python-version }} | ||
| - name: Install dependencies | ||
| shell: bash -l {0} |
There was a problem hiding this comment.
Why is this necessary?
There was a problem hiding this comment.
It's to ensure the conda env is activated. See https://github.com/marketplace/actions/setup-miniconda#important
I can add a comment to document this.
There was a problem hiding this comment.
Oh, this is why internally we use:
- uses: s-weigand/setup-conda@v1
with:
python-version: 3.8
conda-channels: conda-forge
that makes it easier to use the base env.
.github/workflows/build.yml
Outdated
| if: matrix.os == 'ubuntu-latest' && matrix.python-version == '3.7' | ||
| shell: bash -l {0} | ||
| run: | | ||
| pre-commit install |
There was a problem hiding this comment.
So this is not using cache for pre-commit env anymore? FYI pre-commit/action comes with cache built in
Edit: maybe we should try to get pre-commit/action to work again? what's the problem?
There was a problem hiding this comment.
Here's an example of a run where it failed: https://github.com/tomwhite/sgkit/runs/1121992680?check_suite_focus=true
##[error]Unable to locate executable file: pre-commit. Please verify either the file path exists or the file can be found within a directory specified by the PATH environment variable. Also check the file mode to verify the file is executable.
.github/workflows/build.yml
Outdated
| pip install -r requirements.txt -r requirements-dev.txt | ||
| - name: Run pre-commit | ||
| uses: pre-commit/[email protected] | ||
| if: matrix.os == 'ubuntu-latest' && matrix.python-version == '3.7' |
There was a problem hiding this comment.
Is there no better way to do this than having it repeated in each step?
|
My take would be that it'd be better to write a specific workflow for windows/conda and leave the current Ubuntu workflows as they are. In practise, it never works having complicated stuff work across all three platforms. Having a specific workflow where we just do the bare minimum to make the unit tests work on Windows will make things much easier. |
|
Thanks for all the feedback. Hows does this look now? |
.github/workflows/windows.yml
Outdated
| @@ -0,0 +1,30 @@ | |||
| name: Windows build | |||
There was a problem hiding this comment.
Do you want to add this as a badge? I assume so, and if we do, this name will appear on the badge, is this the name we want (maybe just Windows)?
There was a problem hiding this comment.
Be nice to have the badge.
There was a problem hiding this comment.
Fixed in pystatgen/sgkit@5314a83
.github/workflows/windows.yml
Outdated
| on: [push, pull_request] | ||
|
|
||
| jobs: | ||
| build: |
There was a problem hiding this comment.
Do we need to update mergify config (we might want to rename build to say win_build to avoid collision)?
There was a problem hiding this comment.
Fixed in pystatgen/sgkit@5314a83
|
LGTM - ready to merge? |
This is a first attempt at #247.
(array([0, 2], dtype=int64)not(array([0, 2]). I'm not sure why this is happening exactly, but I couldn't see a way in numpy to control whether the dtype was displayed or not.docs.ymlto use the same approach for consistency.