Pin mac runners to x64 #450
Conversation
| @@ -27,42 +27,42 @@ jobs: | |||
| 0 | |||
|
|
|||
| run-doc-link-checks: | |||
| uses: neurodatawithoutborders/nwbinspector/.github/workflows/doc-link-checks.yml@dev | |||
| uses: ./.github/workflows/doc-link-checks.yml | |||
There was a problem hiding this comment.
GitHub also started allowing relative paths to these sometime in the last year - making it a lot more convenient
| steps: | ||
| - uses: s-weigand/setup-conda@v1 | ||
| - uses: conda-incubator/setup-miniconda@v3 |
There was a problem hiding this comment.
setup-conda hasn't been updated in forever and is far less used than the official miniconda action
| @@ -26,12 +26,12 @@ jobs: | |||
|
|
|||
| - name: Install package | |||
| run: | | |||
| pip install -e . | |||
| pip install . | |||
There was a problem hiding this comment.
-e has been deprecated
There was a problem hiding this comment.
When was -e of pip deprecated?
There was a problem hiding this comment.
Not sure - originally heard this from @h-mayorquin when we updated NeuroConv similarly
It's thrown a deprecation warning in console for a long time - this was the first time when trying it lead to an actual error in CI though
But reading into https://packaging.python.org/en/latest/discussions/setup-py-deprecated/ it seems it's not the method itself that's deprecated but rather the route through the CLI - so maybe that could still work
However, I'd rather swap the more official pyproject.toml approach anyhow in a follow-up since that is the most recommended way in 2024
There was a problem hiding this comment.
Oh I see. pip install -e is not deprecated with pyproject.toml, so the changes around removing -e could be resolved by using pyproject.toml.
There was a problem hiding this comment.
Editable mode is a setuptools feature that might not be availabe in other installation backends (e.g. poetry). pip just relies to the instructions to the backend:
https://pip.pypa.io/en/stable/topics/local-project-installs/#editable-installs
But I think most backends implement this. Now, the real problematic part is that editable mode in setuptools changed dramatically on recent versions:
https://setuptools.pypa.io/en/latest/userguide/development_mode.html
My suggestion was that we should not use that API in the CI because it is unstable (within setuptools versions and across build backends implementations) and I suspected it was generating an error (I don't remember if I was right). This is in line what what setuptools suggest themselves (see the attention section in top of limitations here):
https://setuptools.pypa.io/en/latest/userguide/development_mode.html#legacy-behavior
There was a problem hiding this comment.
Interesting. Thanks for the additional information @h-mayorquin ! In the pynwb/hdmf stack, we use pip install -e . to help coverage find the source files, but we can do it differently as @CodyCBakerPhD has done here. We'll make the same changes on the pynwb/hdmf stack.
.github/workflows/testing.yml
Outdated
| @@ -43,7 +43,7 @@ jobs: | |||
|
|
|||
| - name: Run pytest with coverage | |||
| run: | | |||
| pytest -rsx --cov=./ --cov-report xml:./nwbinspector/nwbinspector/coverage.xml | |||
| pytest -rsx --cov=nwbinspector --cov-report xml:./nwbinspector/nwbinspector/coverage.xml | |||
There was a problem hiding this comment.
Need to adjust name here to track coverage (used to rely on -e)
.github/workflows/testing.yml
Outdated
| pip install dandi | ||
| - name: Download testing data and set config path | ||
| run: | | ||
| dandi download "https://gui-staging.dandiarchive.org/#/dandiset/204919" | ||
| python -c "from nwbinspector.testing import update_testing_config; update_testing_config(key='LOCAL_PATH', value='./204919/testing_files/')" | ||
| python -c "from nwbinspector.testing import update_testing_config; update_testing_config(key='LOCAL_PATH', value='{nwbinspector.__file__}/204919/testing_files/')" |
There was a problem hiding this comment.
Another consequence of not using -e
|
Various other things going wrong now as side effects putting into draft |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| @@ -8,8 +8,8 @@ jobs: | |||
| strategy: | |||
| fail-fast: false | |||
| matrix: | |||
| os: ["ubuntu-latest", "macos-latest", "windows-latest"] | |||
| python-version: ["3.8", "3.11"] | |||
| os: ["ubuntu-latest", "windows-latest"] # TODO: update mac and streaming methods | |||
There was a problem hiding this comment.
Passing the buck here to a follow-up; swapping the conda activate action messes up the h5py install somehow, too much to figure out here
| os: ["ubuntu-latest", "macos-latest", "windows-latest"] | ||
| python-version: ["3.8", "3.11"] | ||
| os: ["ubuntu-latest", "macos-13", "windows-latest"] | ||
| python-version: ["3.8", "3.9", "3.10", "3.11"] |
There was a problem hiding this comment.
Action runners are easy to come by nowadays, and our suite is so light that I think it's fine to test against all versions
| pytest tests/ -rsx tests/read_nwbfile_tests.py # TODO when read_nwbfile is ported | ||
| - if: ${{ matrix.python-version == '3.10' && matrix.os == 'ubuntu-latest' }} | ||
| name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v3 | ||
| uses: codecov/codecov-action@v4 |
There was a problem hiding this comment.
Bumpbing to v4 required some other changes, including a new token
|
@oruebel OK this is ready for review now A long cascade of testing changes triggered by the big |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #450 +/- ##
==========================================
- Coverage 90.99% 86.69% -4.30%
==========================================
Files 20 23 +3
Lines 1133 1233 +100
==========================================
+ Hits 1031 1069 +38
- Misses 102 164 +62
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Mac runners were just auto upgraded to x64; pinning to previous version here and will add arm64 tests in followup