-
Notifications
You must be signed in to change notification settings - Fork 0
Tests and dependencies refresh #1
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,19 +2,26 @@ name: tests | |
|
|
||
| on: | ||
| push: | ||
| branches: [main, v8.3.x] | ||
| paths-ignore: | ||
| - "website/**" | ||
| - "*.md" | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, edited] | ||
| branches: ["*"] | ||
| paths-ignore: | ||
| - "website/**" | ||
| - "*.md" | ||
| workflow_dispatch: # allows you to trigger manually | ||
|
|
||
| # When this workflow is queued, automatically cancel any previous running | ||
| # or pending jobs from the same branch | ||
| concurrency: | ||
| group: tests-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| validate: | ||
| name: Validate | ||
| if: github.repository_owner == 'explosion' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Check out repo | ||
|
|
@@ -35,26 +42,31 @@ jobs: | |
| python -m isort thinc --check | ||
| - name: flake8 | ||
| run: | | ||
| python -m pip install flake8==5.0.4 | ||
| python -m pip install flake8 -c requirements.txt | ||
crusaderky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| python -m flake8 thinc --count --select=E901,E999,F821,F822,F823,W605 --show-source --statistics | ||
| - name: mypy | ||
| run: | | ||
| python -m pip install mypy -c requirements.txt | ||
| python -m pip install catalogue confection numpy packaging pydantic | ||
| python -m mypy thinc --no-implicit-reexport | ||
|
|
||
| tests: | ||
| name: Test | ||
| needs: Validate | ||
| if: github.repository_owner == 'explosion' | ||
| name: ${{ matrix.os }} - Python ${{ matrix.python_version }} | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-15-intel] | ||
| python_version: ["3.12"] | ||
| include: | ||
| - os: windows-latest | ||
| python_version: "3.10" | ||
| - os: macos-15-intel | ||
| os: | ||
| - ubuntu-latest | ||
| - ubuntu-24.04-arm | ||
| - macos-15-intel | ||
| - macos-latest | ||
| - windows-latest | ||
| - windows-11-arm | ||
| python_version: ["3.10", "3.11", "3.12", "3.13"] | ||
| exclude: | ||
| - os: windows-11-arm | ||
| python_version: "3.10" | ||
| - os: ubuntu-latest | ||
| python_version: "3.11" | ||
| - os: windows-latest | ||
| python_version: "3.11" | ||
|
|
||
| runs-on: ${{ matrix.os }} | ||
| env: | ||
| NOTEBOOK_KERNEL: "thinc-notebook-tests" | ||
|
|
@@ -68,26 +80,11 @@ jobs: | |
| with: | ||
| python-version: ${{ matrix.python_version }} | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip setuptools wheel | ||
| pip install -r requirements.txt | ||
|
|
||
| - name: Build sdist | ||
| run: | | ||
| # Remove the '.eggs' directory in case it's not empty | ||
| # due to setuptools quirks | ||
| rm -rf .eggs | ||
| python setup.py build_ext --inplace | ||
| rm -rf .eggs | ||
| python setup.py sdist --formats=gztar | ||
| shell: bash | ||
| - name: Install build dependencies | ||
| run: python -m pip install --upgrade build pip wheel | ||
|
|
||
| - name: Run mypy | ||
| run: python -m mypy thinc --no-implicit-reexport | ||
| if: | | ||
| matrix.python_version != '3.6' && | ||
| matrix.python_version != '3.7' | ||
| - name: Build sdist and wheel | ||
| run: python -m build | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This redesign of the build workflow could look unnecessary on first sight. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes sense to me to change, |
||
|
|
||
| - name: Delete source directory | ||
| run: rm -rf thinc | ||
|
|
@@ -99,28 +96,24 @@ jobs: | |
| pip freeze --exclude pywin32 > installed.txt | ||
| pip uninstall -y -r installed.txt | ||
|
|
||
| - name: Install from sdist | ||
| run: | | ||
| SDIST=$(python -c "import os;print(os.listdir('./dist')[-1])" 2>&1) | ||
| PIP_CONSTRAINT="build-constraints.txt" pip install dist/$SDIST | ||
| - name: Install from wheel | ||
| run: pip install dist/*.whl | ||
| shell: bash | ||
|
|
||
| - name: Test import | ||
| run: python -c "import thinc" | ||
|
|
||
| - name: Install test requirements | ||
| run: | | ||
| pip install -r requirements.txt | ||
| run: pip install -r requirements.txt | ||
|
|
||
| - name: Install notebook test requirements | ||
| run: | | ||
| pip install ipykernel pydot graphviz | ||
| python -m ipykernel install --name thinc-notebook-tests --user | ||
| if: matrix.python_version != '3.12' | ||
| run: python -m ipykernel install --name thinc-notebook-tests --user | ||
|
|
||
| - name: List installed packages | ||
| run: python -m pip list | ||
|
|
||
| - name: Run tests without extras | ||
| run: | | ||
| python -m pytest --pyargs thinc -Werror --cov=thinc --cov-report=term | ||
| run: python -m pytest --pyargs thinc --cov=thinc --cov-report=term | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| # Notes on numpy requirements hacks: | ||
| # 1. torch does not have a direct numpy requirement but is compiled | ||
|
|
@@ -156,8 +149,8 @@ jobs: | |
| pip uninstall -y tensorflow | ||
| pip install "thinc-apple-ops>=1.0.0,<2.0.0" | ||
| python -m pytest --pyargs thinc_apple_ops | ||
| if: matrix.os == 'macos-15-intel' && matrix.python_version == '3.10' | ||
| if: runner.os == 'macOS' && matrix.python_version == '3.10' | ||
crusaderky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| - name: Run tests with thinc-apple-ops | ||
| run: python -m pytest --pyargs thinc | ||
| if: matrix.os == 'macos-15-intel' && matrix.python_version == '3.10' | ||
| if: runner.os == 'macOS' && matrix.python_version == '3.10' | ||
This file was deleted.
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed a bunch of redundant dependencies. Now only test dependencies are left. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd leave the dependencies needed for building too, in case someone wants to do a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also similar to below, even if it does reduce maintenance burden, the requirements files here are relatively strict for a reason and we shouldn't change that without explicit buy-in from upstream. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,36 +3,29 @@ murmurhash>=1.0.2,<1.1.0 | |
| cymem>=2.0.2,<2.1.0 | ||
| preshed>=3.0.2,<3.1.0 | ||
| blis>=1.3.0,<1.4.0 | ||
| srsly>=2.4.0,<3.0.0 | ||
| srsly>=2.4.0,<3.1.0 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enables explosion/srsly#120 |
||
| wasabi>=0.8.1,<1.2.0 | ||
| catalogue>=2.0.4,<2.1.0 | ||
| confection>=0.0.1,<1.0.0 | ||
| ml_datasets>=0.2.0,<0.3.0; python_version < "3.11" | ||
| confection>=0.0.1,<1.1.0 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enables confection git tip |
||
| ml_datasets>=0.2.0,<0.3.0 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was no strong reason to pin it only to Python 3.10. |
||
| # Third-party dependencies | ||
| pydantic>=2.0.0,<3.0.0 | ||
| numpy>=2.0.0,<3.0.0 | ||
| packaging>=20.0 | ||
| # Backports of modern Python features | ||
| dataclasses>=0.6,<1.0; python_version < "3.7" | ||
| typing_extensions>=3.7.4.1,<4.5.0; python_version < "3.8" | ||
| contextvars>=2.4,<3; python_version < "3.7" | ||
| # Development dependencies | ||
| cython>=0.29.0 | ||
| hypothesis>=3.27.0,<6.72.2 | ||
| cython>=3.0,<4.0 | ||
| hypothesis>=3.27.0,<6.149 | ||
| pytest>=5.2.0,!=7.1.0 | ||
| pytest-cov>=2.7.0,<5.0.0 | ||
| pytest-cov>=2.7.0,<8.0.0 | ||
| coverage>=5.0.0,<8.0.0 | ||
| mock>=2.0.0,<3.0.0 | ||
| flake8>=3.5.0,<3.6.0 | ||
| mypy>=1.5.0,<1.6.0; platform_machine != "aarch64" and python_version >= "3.8" | ||
| types-mock>=0.1.1 | ||
| types-contextvars>=0.1.2; python_version < "3.7" | ||
| types-dataclasses>=0.1.3; python_version < "3.7" | ||
| importlib_resources; python_version < "3.7" | ||
| flake8==5.0.4 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pin moved from |
||
| mypy>=1.5.0,<1.6.0; platform_machine != "aarch64" | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not spend time to investigate this bizarre arch pin. This is a very old mypy version. The whole linters stack could use a proper refresh, which is out of scope for this PR. |
||
| # Executing notebook tests | ||
| ipykernel>=5.1.4,<5.2.0 | ||
| nbconvert>=5.6.1,<6.5.0 | ||
| nbformat>=5.0.4,<5.2.0 | ||
| ipykernel>=5.1.4,<7.2 | ||
| pydot | ||
| graphviz | ||
|
Comment on lines
+25
to
+26
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved from |
||
| nbconvert>=5.6.1,<7.17 | ||
| nbformat>=5.0.4,<5.11 | ||
| # Test to_disk/from_disk against pathlib.Path subclasses | ||
| pathy>=0.3.5 | ||
| black>=22.0,<23.0 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,6 @@ classifiers = | |
| Operating System :: Microsoft :: Windows | ||
| Programming Language :: Cython | ||
| Programming Language :: Python :: 3 | ||
| Programming Language :: Python :: 3.6 | ||
| Programming Language :: Python :: 3.7 | ||
| Programming Language :: Python :: 3.8 | ||
| Programming Language :: Python :: 3.9 | ||
| Programming Language :: Python :: 3.10 | ||
| Programming Language :: Python :: 3.11 | ||
| Programming Language :: Python :: 3.12 | ||
|
|
@@ -46,19 +42,15 @@ install_requires = | |
| cymem>=2.0.2,<2.1.0 | ||
| preshed>=3.0.2,<3.1.0 | ||
| wasabi>=0.8.1,<1.2.0 | ||
| srsly>=2.4.0,<3.0.0 | ||
| srsly>=2.4.0,<3.1.0 | ||
| catalogue>=2.0.4,<2.1.0 | ||
| confection>=0.0.1,<1.0.0 | ||
| confection>=0.0.1,<1.1.0 | ||
| # Third-party dependencies | ||
| setuptools | ||
| numpy>=1.19.0,<3.0.0 | ||
| numpy>=1.21.0,<3.0.0 | ||
| pydantic>=2.0.0,<3.0.0 | ||
| packaging>=20.0 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer not relaxing the upper version pins unless you need to and only then to relax them to the needed feature release. The maintainers added these pins for a reason, IMO we need to leave them as-is unless we get explicit buy-in to remove them. |
||
| # Backports of modern Python features | ||
| dataclasses>=0.6,<1.0; python_version < "3.7" | ||
| typing_extensions>=3.7.4.1,<5.0.0; python_version < "3.8" | ||
| contextvars>=2.4,<3; python_version < "3.7" | ||
|
|
||
|
|
||
| [options.entry_points] | ||
| pytest_randomly.random_seeder = | ||
| thinc = thinc.api:fix_random_seed | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,7 @@ def get_shuffled_batches(Xs, Ys, batch_size): | |
| @pytest.mark.parametrize( | ||
| ("depth", "width", "vector_width", "nb_epoch"), [(2, 32, 16, 5)] | ||
| ) | ||
| @pytest.mark.xfail(reason="Flaky live download from the internet", strict=False) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. won't
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct, that's why I wrote There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we want the test to fail if the download succeeds? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It won't fail, it does say
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test will XPASS if the download succeeds. This is a textbook use case for @pytest.mark.xfail(
exception=URLError, reason="Flaky live download from the internet", strict=False
)which would neatly single out the flakiness of the download phase and leave a hard fail if anything else raises. |
||
| def test_small_end_to_end(depth, width, vector_width, nb_epoch, create_model, ancora): | ||
| (train_X, train_Y), (dev_X, dev_Y) = ancora | ||
| batch_size = 8 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| import platform | ||
|
|
||
| import pytest | ||
|
|
||
| from thinc.api import ( | ||
|
|
@@ -79,6 +81,11 @@ def create_model(request): | |
|
|
||
|
|
||
| @pytest.mark.slow | ||
| @pytest.mark.xfail( | ||
| platform.system() == "Darwin", | ||
| reason="SSL: CERTIFICATE_VERIFY_FAILED", | ||
| strict=False, # Works on macos-15-intel Python 3.10, for some reason | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is saying "works" here a typo? If I'm reading this correctly the xfail says this always fails on macs.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MacOS Intel Python 3.10 - always works I did not invest time to investigate why. So I just relaxed the check and moved on. The test must pass on Linux and Windows, which is plenty for me. Everything else is a ml-datasets issue that shouldn't be dealt with here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I'd delete the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This looks fine as is, the code says There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I'm sorry! Thank you. I'd just delete the
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's healthier to explicitly declare that this is the intended behaviour if someone later sets strict=True by default in the pytest config? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then |
||
| ) | ||
| @pytest.mark.parametrize(("width", "nb_epoch", "min_score"), [(32, 20, 0.8)]) | ||
| def test_small_end_to_end(width, nb_epoch, min_score, create_model, mnist): | ||
| batch_size = 128 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| from mock import MagicMock | ||
| from unittest.mock import MagicMock | ||
|
|
||
| from thinc.api import Linear, with_debug | ||
|
|
||
|
|
||
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.
I'd say
[main, v8.3.x]here as well.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.
I routinely do pull requests against a staging branch of some sort on my local forks to verify that everything works AND produce a nice PR diff.
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.
I understand that, but you can always change that locally in your fork. When I work with CI I prefer to leave behind settings that reduce unnecessary electricity usage.
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.
The only case where this comes in to play is when someone deliberately opens a PR against a staging branch, at which point why wouldn't they want to run CI?
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.
Let's agree to disagree, it's not a big deal. I like to err on the side of reducing unnecessary CI use, that's all.