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

Pin mac runners to x64 #450

Merged
merged 38 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
dcb880e
pin mac to x64
CodyCBakerPhD Apr 25, 2024
eb741a3
make relative
CodyCBakerPhD Apr 25, 2024
5bcbfdb
swap to miniconda
CodyCBakerPhD Apr 25, 2024
e4d3f48
restore options
CodyCBakerPhD Apr 25, 2024
1d5bc2a
restore conda forge
CodyCBakerPhD Apr 25, 2024
076da13
try --user
CodyCBakerPhD Apr 25, 2024
9e3374f
remove easy
CodyCBakerPhD Apr 25, 2024
041761d
try to fix testing directory
CodyCBakerPhD Apr 25, 2024
b890ffe
missing f string
CodyCBakerPhD Apr 25, 2024
15c16d5
follow neuroconv style: https://github.com/catalystneuro/neuroconv/bl…
CodyCBakerPhD Apr 25, 2024
05eff25
restore local
CodyCBakerPhD Apr 25, 2024
411c056
make global; try changing parent by one level
CodyCBakerPhD Apr 28, 2024
7c1b9c1
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 28, 2024
918808e
annotate global and try making relative to setup file
CodyCBakerPhD Apr 28, 2024
0442778
try str cast
CodyCBakerPhD Apr 28, 2024
85cc954
move str cast
CodyCBakerPhD Apr 28, 2024
d5a041e
add parents
CodyCBakerPhD Apr 28, 2024
5471a2e
Update CHANGELOG.md
CodyCBakerPhD Apr 28, 2024
0bb9d0e
try another level
CodyCBakerPhD Apr 28, 2024
15f09b5
try cwd
CodyCBakerPhD Apr 29, 2024
cd8a673
update cwd
CodyCBakerPhD Apr 29, 2024
7f70019
update testing path
CodyCBakerPhD Apr 29, 2024
3383a9e
remove prints
CodyCBakerPhD Apr 29, 2024
837f380
remove cd
CodyCBakerPhD Apr 29, 2024
fea135d
fix name
CodyCBakerPhD Apr 29, 2024
a408ee7
adjust streaming workflow too
CodyCBakerPhD Apr 29, 2024
aaa2001
update streaming too
CodyCBakerPhD Apr 29, 2024
7fd881f
fix coverage path
CodyCBakerPhD Apr 29, 2024
e5eaadd
update coverage version
CodyCBakerPhD Apr 29, 2024
29fc00a
update badge
CodyCBakerPhD Apr 29, 2024
e0a06c8
propagate secret
CodyCBakerPhD Apr 29, 2024
5b08f4d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 29, 2024
cc05b06
update deployment to pass secret
CodyCBakerPhD Apr 29, 2024
e8969c9
try activating streaming environment
CodyCBakerPhD Apr 29, 2024
79b2cd4
release pin on codecov
CodyCBakerPhD Apr 29, 2024
b0e39bf
fix yaml
CodyCBakerPhD Apr 29, 2024
252f57c
revert conda setup for streaming and remove mac
CodyCBakerPhD Apr 29, 2024
abbb2a7
Merge branch 'dev' into pin_mac_x64
CodyCBakerPhD Apr 29, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions .github/workflows/deploy-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub also started allowing relative paths to these sometime in the last year - making it a lot more convenient


run-tests:
needs: assess-file-changes
if: ${{ needs.assess-file-changes.outputs.SOURCE_CHANGED == 'true' }}
uses: neurodatawithoutborders/nwbinspector/.github/workflows/testing.yml@dev
uses: ./.github/workflows/testing.yml

run-streaming-tests:
needs: assess-file-changes
if: ${{ needs.assess-file-changes.outputs.SOURCE_CHANGED == 'true' }}
uses: neurodatawithoutborders/nwbinspector/.github/workflows/streaming-tests.yml@dev
uses: ./.github/workflows/streaming-tests.yml

run-past-gallery:
needs: assess-file-changes
if: ${{ needs.assess-file-changes.outputs.SOURCE_CHANGED == 'true' }}
uses: neurodatawithoutborders/nwbinspector/.github/workflows/version-gallery.yml@dev
uses: ./.github/workflows/version-gallery.yml

run-dev-gallery:
needs: assess-file-changes
if: ${{ needs.assess-file-changes.outputs.SOURCE_CHANGED == 'true' }}
uses: neurodatawithoutborders/nwbinspector/.github/workflows/dev-gallery.yml@dev
uses: ./.github/workflows/dev-gallery.yml

test-dandi-latest:
needs: assess-file-changes
if: ${{ needs.assess-file-changes.outputs.SOURCE_CHANGED == 'true' }}
uses: neurodatawithoutborders/nwbinspector/.github/workflows/dandi_release.yml@dev
uses: ./.github/workflows/dandi_release.yml

test-dandi-dev:
needs: assess-file-changes
if: ${{ needs.assess-file-changes.outputs.SOURCE_CHANGED == 'true' }}
uses: neurodatawithoutborders/nwbinspector/.github/workflows/dandi_dev.yml@dev
uses: ./.github/workflows/dandi_dev.yml

test-dandi-dev-live:
needs: assess-file-changes
if: ${{ needs.assess-file-changes.outputs.SOURCE_CHANGED == 'true' }}
uses: neurodatawithoutborders/nwbinspector/.github/workflows/dandi_dev_live_service.yml@dev
uses: ./.github/workflows/dandi_dev_live_service.yml

check-final-status:
name: All tests passing
Expand All @@ -74,7 +74,7 @@ jobs:
runs-on: ubuntu-latest

steps:
- name: Decide whether the all jobs succeeded or at least one failed
- name: Decide whether all jobs succeeded or at least one failed
uses: re-actors/alls-green@release/v1
with:
allowed-skips: run-tests # Sometimes only docs are adjusted
Expand Down
16 changes: 8 additions & 8 deletions .github/workflows/testing.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ jobs:
strategy:
fail-fast: false
matrix:
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"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

steps:
- uses: s-weigand/setup-conda@v1
- uses: conda-incubator/setup-miniconda@v3
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setup-conda hasn't been updated in forever and is far less used than the official miniconda action

with:
update-conda: true
auto-update-conda: true
python-version: ${{ matrix.python-version }}
conda-channels: conda-forge
channels: conda-forge
- uses: actions/checkout@v3
- run: git fetch --prune --unshallow --tags

Expand All @@ -26,12 +26,12 @@ jobs:

- name: Install package
run: |
pip install -e .
pip install .
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-e has been deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When was -e of pip deprecated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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/')"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another consequence of not using -e

- name: Uninstall h5py
run: pip uninstall -y h5py
- name: Install ROS3
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to adjust name here to track coverage (used to rely on -e)

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
Expand Down
2 changes: 1 addition & 1 deletion src/nwbinspector/testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def load_testing_config() -> dict:
# directly from GitHub and then updated the branch/commit in-place
if not test_config_file_path.exists(): # pragma: no cover
raise FileNotFoundError(
"The testing configuration file not found at the location '{test_config_file_path}'! "
f"The testing configuration file not found at the location '{test_config_file_path}'! "
"Please try reinstalling the package."
)

Expand Down
Loading