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

Exclude test data from the Python package #5970

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 5 additions & 9 deletions onnx/backend/test/loader/__init__.py
Original file line number Diff line number Diff line change
@@ -1,22 +1,18 @@
# Copyright (c) ONNX Project Contributors

# SPDX-License-Identifier: Apache-2.0
from __future__ import annotations

import json
import os
from typing import List, Optional

from onnx.backend.test.case.test_case import TestCase

DATA_DIR = os.path.join(
os.path.dirname(os.path.realpath(os.path.dirname(__file__))), "data"
)


def load_model_tests(
data_dir: str = DATA_DIR,
kind: Optional[str] = None,
) -> List[TestCase]:
data_dir: str | os.PathLike,
kind: str | None = None,
) -> list[TestCase]:
"""Load model test cases from on-disk data files."""
supported_kinds = os.listdir(data_dir)
if kind not in supported_kinds:
Expand All @@ -35,7 +31,7 @@ def load_model_tests(
if os.path.exists(os.path.join(case_dir, "model.onnx")):
url = None
model_name = test_name[len("test_")]
model_dir: Optional[str] = case_dir
model_dir: str | None = case_dir
else:
with open(os.path.join(case_dir, "data.json")) as f:
data = json.load(f)
Expand Down
32 changes: 17 additions & 15 deletions onnx/backend/test/runner/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import time
import unittest
from collections import defaultdict
from pathlib import Path
cbourjau marked this conversation as resolved.
Show resolved Hide resolved
from typing import Any, Callable, Iterable, Pattern, Sequence
from urllib.request import urlretrieve

Expand Down Expand Up @@ -57,6 +58,7 @@ class Runner:
def __init__(
self,
backend: type[Backend],
test_data_dir: str | os.PathLike,
parent_module: str | None = None,
test_kwargs: dict | None = None,
) -> None:
Expand All @@ -66,26 +68,31 @@ def __init__(
self._exclude_patterns: set[Pattern[str]] = set()
self._xfail_patterns: set[Pattern[str]] = set()
self._test_kwargs: dict = test_kwargs or {}
self._test_data_dir = test_data_dir

# This is the source of the truth of all test functions.
# Properties `test_cases`, `test_suite` and `tests` will be
# derived from it.
# {category: {name: func}}
self._test_items: dict[str, dict[str, TestItem]] = defaultdict(dict)

for rt in load_model_tests(kind="node"):
for rt in load_model_tests(data_dir=test_data_dir, kind="node"):
self._add_model_test(rt, "Node")

for rt in load_model_tests(kind="real"):
for rt in load_model_tests(data_dir=test_data_dir, kind="real"):
self._add_model_test(rt, "Real")

for rt in load_model_tests(kind="simple"):
for rt in load_model_tests(data_dir=test_data_dir, kind="simple"):
self._add_model_test(rt, "Simple")

for ct in load_model_tests(kind="pytorch-converted"):
for ct in load_model_tests(
data_dir=test_data_dir, kind="pytorch-converted"
):
self._add_model_test(ct, "PyTorchConverted")

for ot in load_model_tests(kind="pytorch-operator"):
for ot in load_model_tests(
data_dir=test_data_dir, kind="pytorch-operator"
):
self._add_model_test(ot, "PyTorchOperator")

def _get_test_case(self, name: str) -> type[unittest.TestCase]:
Expand Down Expand Up @@ -345,17 +352,12 @@ def run(test_self: Any, device: str, **kwargs) -> None:
if model_test.url is not None and model_test.url.startswith(
"onnx/backend/test/data/light/"
):
# Get the root directory from the test data dir in a
# hacky way. This will break when moving the files
# elsewhere.
root = Path(self._test_data_dir).parent.parent.parent.parent
# testing local files
model_pb_path = os.path.normpath(
os.path.join(
os.path.dirname(__file__),
"..",
"..",
"..",
"..",
model_test.url,
)
)
model_pb_path = root / model_test.url
if not os.path.exists(model_pb_path):
raise FileNotFoundError(f"Unable to find model {model_pb_path!r}.")
onnx_home = os.path.expanduser(
Expand Down
14 changes: 6 additions & 8 deletions onnx/backend/test/stat_coverage.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import os
from typing import IO, Any, Dict, List, Sequence
import pathlib

Check warning on line 9 in onnx/backend/test/stat_coverage.py

View check run for this annotation

Codecov / codecov/patch

onnx/backend/test/stat_coverage.py#L9

Added line #L9 was not covered by tests

from onnx import AttributeProto, defs, load
from onnx.backend.test.case import collect_snippets
Expand Down Expand Up @@ -98,13 +99,8 @@
f.write("Node tests have covered 0/0 (N/A) common operators. \n\n")
if num_experimental:
f.write(
"Node tests have covered {}/{} ({:.2f}%, {} generators excluded) " # noqa: UP032
"experimental operators.\n\n".format(
len(experimental_covered),
num_experimental,
(len(experimental_covered) / float(num_experimental) * 100),
len(experimental_generator),
)
f"Node tests have covered {len(experimental_covered)}/{num_experimental} ({len(experimental_covered) / float(num_experimental) * 100:.2f}%, {len(experimental_generator)} generators excluded) "
"experimental operators.\n\n"
)
else:
f.write("Node tests have covered 0/0 (N/A) experimental operators.\n\n")
Expand Down Expand Up @@ -157,7 +153,9 @@
# Need to grab associated nodes
attrs: Dict[str, Dict[str, List[Any]]] = {}
model_paths: List[Any] = []
for rt in load_model_tests(kind="real"):
for rt in load_model_tests(
data_dir=pathlib.Path(__file__).parent / "data", kind="real"
):
if rt.url.startswith("onnx/backend/test/data/light/"):
# testing local files
model_name = os.path.normpath(
Expand Down
7 changes: 6 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,21 @@ onnx = [
"**/*.h",
"**/*.proto",
"**/*.pyi",
"backend/test/data/**/*",
"py.typed",
]

[tool.setuptools.exclude-package-data]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the change goes into the right direction. My only concern is runtimes such as onnxruntime expects to find this data. I'd me more confortable to display an error message saying what instruction is needed to get the data and have the backend tests passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onnxruntime already fetches the entire onnx source during the build process anyway. Are they really using the PyPI package for testing, too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is used. onnx should be shipped without data. That's not a debate. Since this data can be automatically generated, we could also generate this data when the package is being installed. Something like pip install onnx <options>... So developers using the data could just install onnx with this option and nothing else would change.

onnx = [
"backend/test/data/**/*",
]

[tool.pytest.ini_options]

addopts = "--nbval --nbval-current-env --tb=short --color=yes"
testpaths = [
"onnx/test",
"onnx/examples",
"tests",
]

[tool.mypy]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import platform
import unittest
import pathlib
from typing import Any

import numpy
Expand All @@ -26,6 +27,7 @@
ort: Any = None # type: ignore[no-redef]
ort_version: Any = None # type: ignore[no-redef]

TEST_DATA_DIR = pathlib.Path(__file__).parent.parent.parent / "onnx/backend/test/data"

# The following just executes a backend based on InferenceSession through the backend test

Expand Down Expand Up @@ -108,7 +110,9 @@ def run_node(cls, node, inputs, device=None, outputs_info=None, **kwargs):


if ort is not None:
backend_test = onnx.backend.test.BackendTest(InferenceSessionBackend, __name__)
backend_test = onnx.backend.test.BackendTest(
InferenceSessionBackend, test_data_dir=TEST_DATA_DIR, parent_module=__name__
)

if platform.architecture()[0] == "32bit":
backend_test.exclude("(test_vgg19|test_zfnet|test_bvlc_alexnet)")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
# SPDX-License-Identifier: Apache-2.0

import os
import pathlib
import platform
import sys
import unittest
from typing import Any

import numpy
import version_utils
import packaging

import onnx.backend.base
import onnx.backend.test
Expand All @@ -19,6 +20,8 @@
from onnx.backend.base import Device, DeviceType
from onnx.reference import ReferenceEvaluator

_TEST_DATA_DIR = pathlib.Path(__file__).parent.parent.parent / "onnx/backend/test/data"

# The following just executes a backend based on ReferenceEvaluator through the backend test


Expand Down Expand Up @@ -89,7 +92,9 @@ def run_node(cls, node, inputs, device=None, outputs_info=None, **kwargs):
raise NotImplementedError("Unable to run the model node by node.")


backend_test = onnx.backend.test.BackendTest(ReferenceEvaluatorBackend, __name__)
backend_test = onnx.backend.test.BackendTest(
ReferenceEvaluatorBackend, test_data_dir=_TEST_DATA_DIR, parent_module=__name__
)

if os.getenv("APPVEYOR"):
backend_test.exclude("(test_vgg19|test_zfnet)")
Expand Down Expand Up @@ -193,7 +198,7 @@ def run_node(cls, node, inputs, device=None, outputs_info=None, **kwargs):
backend_test.exclude("test_qlinearmatmul_3D_int8_float32_cpu")

# op_dft and op_stft requires numpy >= 1.21.5
if version_utils.numpy_older_than("1.21.5"):
if packaging.version.parse(numpy.__version__) < packaging.version.parse("1.21.5"):
backend_test.exclude("test_stft")
backend_test.exclude("test_stft_with_window")
backend_test.exclude("test_stft_cpu")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import itertools
import os
import pathlib
import platform
import unittest
from typing import Any, Optional, Sequence, Tuple
Expand All @@ -29,6 +30,8 @@
# We don't enable report in this test because the report collection logic itself
# fails when models are mal-formed.

_TEST_DATA_DIR = pathlib.Path(__file__).parent.parent.parent / "onnx/backend/test/data"


class DummyBackend(onnx.backend.base.Backend):
@classmethod
Expand Down Expand Up @@ -111,7 +114,10 @@ def do_enforce_test_coverage_safelist(model: ModelProto) -> bool:
}

backend_test = onnx.backend.test.BackendTest(
DummyBackend, __name__, test_kwargs=test_kwargs
DummyBackend,
test_data_dir=_TEST_DATA_DIR,
parent_module=__name__,
test_kwargs=test_kwargs,
)
if os.getenv("APPVEYOR"):
backend_test.exclude(r"(test_vgg19|test_zfnet)")
Expand Down