-
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 1 commit
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.
|
||
| 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.
|
||
|
|
||
| - 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' | ||
|
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. Unused file 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. Agreed, vestigial now that the repo uses cibuildwheel |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,10 @@ | |
| requires = [ | ||
| "setuptools", | ||
| "cython>=3.0,<4.0", | ||
| "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", | ||
| "murmurhash>=1.0.2,<2", | ||
| "cymem>=2.0.2,<3", | ||
| "preshed>=3.0.2,<4", | ||
| "blis>=1.3.0,<2", | ||
|
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. Can you keep the upper pins to be on a feature version? See the explanation below. |
||
| "numpy>=2.0.0,<3.0.0", | ||
| ] | ||
| build-backend = "setuptools.build_meta" | ||
|
|
@@ -63,3 +63,18 @@ repair-wheel-command = "delocate-wheel --require-archs {delocate_archs} -w {dest | |
|
|
||
| [tool.isort] | ||
| profile = "black" | ||
|
|
||
| [tool.pytest.ini_options] | ||
| addopts = "--strict-markers --strict-config -v -r sxfE --color=yes --durations=10" | ||
| markers = [ | ||
| "slow: test with long runtime", | ||
| ] | ||
| filterwarnings = [ | ||
| "error", | ||
| # This is triggered by pytest -p thinc.tests.enable_tensorflow | ||
| # Not an issue as the coverage upon import is recorded during the | ||
| # pytest run "without extras". | ||
| "ignore:Module thinc was previously imported, but not measured:coverage.exceptions.CoverageWarning", | ||
| # https://github.com/coveragepy/coveragepy/issues/1790 | ||
| "ignore:Plugin file tracers:coverage.exceptions.CoverageWarning", | ||
| ] | ||
|
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 |
|---|---|---|
| @@ -1,39 +1,21 @@ | ||
| # Explosion-provided dependencies | ||
| 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 | ||
| 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" | ||
| # 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" | ||
| # Explosion-provided test dependencies | ||
| ml-datasets>=0.2.0,<1 | ||
|
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 pin |
||
| # Development dependencies | ||
| cython>=0.29.0 | ||
| hypothesis>=3.27.0,<6.72.2 | ||
| hypothesis>=3.27.0,<7 | ||
| pytest>=5.2.0,!=7.1.0 | ||
| pytest-cov>=2.7.0,<5.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" | ||
| pytest-cov>=2.7.0 | ||
| coverage>=5.0.0 | ||
| cython>=3.0,<4.0 # Cython is a test dependency | ||
| # 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 | ||
| 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 | ||
| nbformat>=5.0.4 | ||
| # Test to_disk/from_disk against pathlib.Path subclasses | ||
| pathy>=0.3.5 | ||
| # Linters | ||
| 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. Moved pin from tests.yml |
||
| 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. This is a bizarre pin which I did not investigate. This mypy version is extremely old and would need to be upgraded anyway, which is out of scope for this exercise. |
||
| black>=22.0,<23.0 | ||
| isort>=5.0,<6.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 | ||
|
|
@@ -30,35 +26,31 @@ classifiers = | |
| [options] | ||
| zip_safe = false | ||
| include_package_data = true | ||
| python_requires = >=3.10,<3.15 | ||
|
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. Please leave this as well. |
||
| python_requires = >=3.10 | ||
| setup_requires = | ||
| cython>=3.0,<4.0 | ||
| numpy>=2.0.0,<3.0.0 | ||
| # We also need our Cython packages here to compile against | ||
| cymem>=2.0.2,<2.1.0 | ||
| preshed>=3.0.2,<3.1.0 | ||
| murmurhash>=1.0.2,<1.1.0 | ||
| blis>=1.3.0,<1.4.0 | ||
| cymem>=2.0.2,<3 | ||
| preshed>=3.0.2,<4 | ||
| murmurhash>=1.0.2,<2 | ||
| blis>=1.3.0,<2 | ||
| install_requires = | ||
| # Explosion-provided dependencies | ||
| blis>=1.3.0,<1.4.0 | ||
| murmurhash>=1.0.2,<1.1.0 | ||
| 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 | ||
| catalogue>=2.0.4,<2.1.0 | ||
| confection>=0.0.1,<1.0.0 | ||
| blis>=1.3.0,<2 | ||
| murmurhash>=1.0.2,<2 | ||
| cymem>=2.0.2,<3 | ||
| preshed>=3.0.2,<4 | ||
| wasabi>=0.8.1,<2 | ||
| srsly>=2.4.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. This enables explosion/srsly#120 |
||
| catalogue>=2.0.4,<3 | ||
| confection>=0.0.1,<2 | ||
|
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 enables confection git tip |
||
| # 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.