diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d999e12..cdfada6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -118,47 +118,3 @@ jobs: with: name: python-package-distributions path: dist/ - - publish-test: - needs: build - runs-on: ubuntu-latest - if: github.event_name == 'workflow_dispatch' - - steps: - - uses: actions/checkout@v6 - - - name: Set up Python - uses: actions/setup-python@v6 - with: - python-version: '3.11' - - - name: Install dependencies - run: | - python -m pip install --upgrade pip - pip install poetry - - - name: Download all the dists - uses: actions/download-artifact@v4 - with: - name: python-package-distributions - path: dist/ - - - name: Check if version already published to TestPyPI - id: check-version - run: | - current_version=$(poetry version -s) - status=$(curl -s -o /dev/null -w "%{http_code}" "https://test.pypi.org/pypi/openmodels/${current_version}/json") - if [ "$status" == "200" ]; then - echo "Version $current_version already exists on TestPyPI, skipping publish." - echo "skip=true" >> "$GITHUB_OUTPUT" - else - echo "skip=false" >> "$GITHUB_OUTPUT" - fi - - - name: Publish package to TestPyPI - if: steps.check-version.outputs.skip == 'false' - env: - TEST_PYPI_API_TOKEN: ${{ secrets.TEST_PYPI_API_TOKEN }} - run: | - poetry config repositories.testpypi https://test.pypi.org/legacy/ - poetry publish --repository testpypi --username __token__ --password $TEST_PYPI_API_TOKEN diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e8f17e..eb435cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,33 @@ All notable changes to the OpenModels project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.1.0-alpha.22] - 2026-08-04 + +### Removed + +- TestPyPI publishing: dropped the `publish-test` job from `.github/workflows/ci.yml` (built a package on every `workflow_dispatch` run and published it to `test.pypi.org` if that version wasn't already there) and the now-unused `testpypi-badge.json`/README badge that displayed the last-published TestPyPI version + +### Added + +- `roundtrip_fit()` test helper (`openmodels/test_helpers.py`): monkeypatches `fit()`/`fit_transform()`/`fit_predict()` on given estimator classes so their fitted state is replaced with the result of an openmodels serialize→deserialize round-trip, letting any existing test suite double as a round-trip fidelity check +- `test/test_estimator_conformance.py`: runs scikit-learn's own generic `parametrize_with_checks()` battery against round-tripped instances of every estimator openmodels supports, with a strict, per-(estimator, check) xfail list distinguishing known openmodels gaps from pre-existing sklearn/check fragility +- `test/upstream/`: reuses scikit-learn's own `cross_decomposition` test suite (`test_pls.py`) unmodified against `PLSRegression`/`PLSCanonical`/`CCA`/`PLSSVD` via a `conftest.py` that registers sklearn's test fixtures as a pytest plugin +- `test/_estimator_construction.py`: shared registry of minimal constructor arguments for meta-estimators that can't be built with bare defaults (e.g. `estimator=` for `ClassifierChain`, `RFE`, `StackingRegressor`), replacing duplicated per-file special-casing across the smoke test modules +- scikit-learn 1.9.0 added to the README/docs compatibility matrix +- `test/test_serializer_base.py`, `test/test_birch_cftree.py`, `test/test_sparse_containers.py`, `test/upstream/cluster/`: regression coverage for the round-trip fixes below +- 29 stale entries removed from `test/test_estimator_conformance.py`'s `KNOWN_ROUNDTRIP_XFAILS` now that the underlying gaps are fixed + +### Fixed + +- `PLSRegression`, `CCA`, `PLSCanonical`, and `PLSSVD` were missing `_x_std`/`_y_mean`/`_y_std` from `ATTRIBUTE_EXCEPTIONS`, so `predict()` on a round-tripped model silently used unfitted/default scaling statistics instead of the values learned during `fit()` +- `test_others.py` estimator discovery now filters through `ALL_ESTIMATORS` so experimental-only estimators that become discoverable as a side effect of importing `sklearn.utils.estimator_checks` (e.g. `HalvingGridSearchCV`) aren't constructed without openmodels actually knowing how to serialize them +- README: corrected the scikit-learn compatibility workflow description (it runs on-demand via `workflow_dispatch`, not on every push to `main` and weekly, since push/schedule triggers were removed) and fixed a stale placeholder clone URL +- Numpy `dtype` object attributes (e.g. `SimpleImputer._fit_dtype`) failed to round-trip for any dtype other than `float64`: the type tag used to look up a deserializer was numpy's internal per-dtype subclass name (`Int64DType`, `BoolDType`, ...), which had no matching handler, so the value silently came back as a raw string instead of an `np.dtype`, and `transform()` later crashed with `'str' object has no attribute 'kind'` +- Dict-valued attributes with non-string keys (e.g. `OrdinalEncoder._missing_indices: dict[int, int]`) lost their key types on deserialize, since JSON forces string keys and there was no handler to coerce them back; `transform()` then crashed indexing with a `str` instead of an `int`. Fixed generically in `SerializerMixin` for any `int`/`float`/`bool`/`str`-keyed dict, not just this one estimator +- `BisectingKMeans`'s internal `_BisectingTree` centroids (and `KDTree` data) were silently widened from `float32` to `float64` on round-trip, since the top-level attribute-dtype tracking doesn't reach values nested inside these bespoke serializers; `predict()` then crashed with a Cython buffer dtype mismatch +- `Birch`'s fitted `root_`/`dummy_leaf_` CF-tree was reduced to an empty, structure-less stub on deserialize (only 4 scalar config values were captured, no subclusters/centroids/leaf links); `partial_fit()` on a round-tripped model then crashed trying to concatenate zero leaf centroid arrays. Now serialized and deserialized recursively, including the cross-cutting doubly-linked list of leaf nodes +- The sparse (de)serializer only recognized `scipy.sparse.csr_matrix`; `csc_matrix` and the newer array-API `csr_array`/`csc_array` containers raised `TypeError: ... is not JSON serializable` instead of round-tripping. Affected any estimator that stores its sparse training/fitted data verbatim (e.g. `KNeighborsClassifier`, `NearestNeighbors`, `DBSCAN`, `KernelRidge`) + ## [0.1.0-alpha.21] - 2026-03-14 ### Added diff --git a/README.md b/README.md index a4c1b38..b8f3fe1 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,6 @@ [![PyPI version](https://badge.fury.io/py/openmodels.svg?cacheBust=1)](https://badge.fury.io/py/openmodels) [![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://opensource.org/licenses/MIT) [![Python Versions](https://img.shields.io/pypi/pyversions/openmodels.svg?cacheBust=1)](https://pypi.org/project/openmodels/) -[![TestPyPI](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/Gnpd/openmodels/refs/heads/main/testpypi-badge.json)](https://test.pypi.org/project/openmodels/) OpenModels is a flexible and extensible library for serializing and deserializing machine learning models. It's designed to support any serialization format through a plugin-based architecture, providing a safe and transparent solution for exporting and sharing predictive models. @@ -171,13 +170,14 @@ You can pass any compatible `all_estimators` function, list, or dictionary to `S ## scikit-learn Version Compatibility -OpenModels is automatically tested against the following scikit-learn versions on every push to `main` and weekly via CI: +OpenModels is tested against the following scikit-learn versions via the on-demand [`scikit-learn compatibility`](https://github.com/Gnpd/openmodels/actions/workflows/sklearn-compat.yml) CI workflow: | scikit-learn | Status | |---|---| | 1.6.1 | ✅ Tested | | 1.7.2 | ✅ Tested | | 1.8.0 | ✅ Tested | +| 1.9.0 | ✅ Tested | If you encounter any incompatibility or a use case where the library does not work correctly with your version of scikit-learn, please [open an issue](https://github.com/Gnpd/openmodels/issues/new) — we would greatly appreciate your feedback! @@ -196,7 +196,7 @@ To run the tests: 1. Clone the repository: ```bash - git clone https://github.com/your-repo/openmodels.git + git clone https://github.com/Gnpd/openmodels.git cd openmodels ``` diff --git a/conftest.py b/conftest.py new file mode 100644 index 0000000..7962a0c --- /dev/null +++ b/conftest.py @@ -0,0 +1,25 @@ +# Registers sklearn's own conftest as a plugin - required because some of the sklearn test +# modules reused wholesale under test/upstream/ (see test/upstream/cross_decomposition/ for the +# worked example) depend on fixtures sklearn defines there, e.g. `global_random_seed`, which is +# wired up via a pytest_generate_tests hook. pytest only allows pytest_plugins to be declared in +# a conftest.py at the true rootdir, which is why this lives here rather than under test/. +# +# Recipe for reusing another sklearn test module (e.g. sklearn/svm/tests/test_svm.py): +# 1. mkdir test/upstream// with an empty __init__.py. +# 2. test/upstream//conftest.py: a module-scoped autouse fixture wrapping +# openmodels.test_helpers.roundtrip_fit around the estimator classes that module's tests +# exercise, e.g.: +# @pytest.fixture(scope="module", autouse=True) +# def _roundtrip_something(): +# with roundtrip_fit(EstimatorA, EstimatorB): +# yield +# 3. test/upstream//test__upstream.py, a one-liner: +# from sklearn..tests.test_ import * # noqa: F401,F403 +# 4. Run it and skim the output. Not everything in an upstream test file is fair game as a +# round-trip check (some test private helper functions unrelated to any estimator - that's +# harmless bonus coverage) and not everything that fails is openmodels's fault (some fail +# identically against a bare, unpatched estimator - verify with roundtrip_fit removed +# before assuming a failure is a real serialization gap; see +# test/test_estimator_conformance.py's KNOWN_ROUNDTRIP_XFAILS for the established pattern +# of documenting which is which, with reasons). +pytest_plugins = ["sklearn.conftest"] diff --git a/docs/supported_models.md b/docs/supported_models.md index 8eb216f..7253f2c 100644 --- a/docs/supported_models.md +++ b/docs/supported_models.md @@ -1,7 +1,7 @@ # Supported Models OpenModels currently supports **scikit-learn** estimators. The library has been tested against -scikit-learn versions **1.6.1**, **1.7.2**, and **1.8.0**. +scikit-learn versions **1.6.1**, **1.7.2**, **1.8.0**, and **1.9.0**. ## scikit-learn diff --git a/openmodels/serializers/base.py b/openmodels/serializers/base.py index 8e3115c..71d915a 100644 --- a/openmodels/serializers/base.py +++ b/openmodels/serializers/base.py @@ -8,7 +8,7 @@ """ import numpy as np -from scipy.sparse import csr_matrix # type: ignore +from scipy.sparse import csr_matrix, csc_matrix, csr_array, csc_array # type: ignore from scipy.interpolate import interp1d, BSpline # type: ignore from scipy.stats._distn_infrastructure import rv_continuous_frozen # type: ignore import scipy.stats # type: ignore @@ -32,7 +32,17 @@ def convert_to_serializable(self, value): # Recursive case: dict, list, tuple if isinstance(value, dict): - return {str(k): self.convert_to_serializable(v) for k, v in value.items()} + if all(isinstance(k, str) for k in value): + return {k: self.convert_to_serializable(v) for k, v in value.items()} + # Non-string keys (e.g. int) can't be represented as JSON object keys without + # losing their type, so fall back to a self-describing keys/values envelope that + # _deserialize_dict can reconstruct exactly. + return { + "__openmodels_dict__": True, + "keys": [self.convert_to_serializable(k) for k in value.keys()], + "key_types": [type(k).__name__ for k in value.keys()], + "values": [self.convert_to_serializable(v) for v in value.values()], + } if isinstance(value, (list, tuple)): return [self.convert_to_serializable(v) for v in value] @@ -98,6 +108,21 @@ def _deserialize_function(self, data: Dict[str, str]) -> Callable: module = __import__(data["module"], fromlist=[data["name"]]) return getattr(module, data["name"]) + def _deserialize_dict(self, value: Any) -> Any: + """Deserialize a dict, restoring non-string key types for the envelope produced by + convert_to_serializable's dict branch. Plain string-keyed dicts (the common case, + including dicts produced by older openmodels versions) pass through unchanged. + """ + if isinstance(value, dict) and value.get("__openmodels_dict__"): + allowed_key_types = {"int": int, "float": float, "bool": bool, "str": str} + return { + allowed_key_types.get(kt, lambda x: x)( + k + ): self.convert_from_serializable(v) + for k, kt, v in zip(value["keys"], value["key_types"], value["values"]) + } + return value + # --- Handlers --- def _get_serializer_handlers(self): """Each mixin extends this list.""" @@ -116,6 +141,7 @@ def _get_deserializer_handlers(self): ("str", str), ("type", self._deserialize_type), ("tuple", tuple), + ("dict", self._deserialize_dict), ("function", self._deserialize_function), ] @@ -282,7 +308,14 @@ def _deserialize_bspline(self, data: Dict[str, Any]) -> BSpline: def _get_serializer_handlers(self): return [ (BSpline, self._serialize_bspline), - (csr_matrix, self._serialize_csr_matrix), + # csr_matrix is the only sparse container openmodels round-trips on the wire, but + # any scipy sparse container (the older *_matrix family or the newer array-API + # *_array family) is accepted here - _serialize_csr_matrix normalizes it to csr via + # the csr_matrix(value) constructor, which accepts any sparse-like input. + ( + (csr_matrix, csc_matrix, csr_array, csc_array), + self._serialize_csr_matrix, + ), (interp1d, self._serialize_interp1d), (rv_continuous_frozen, self._serialize_scipy_dist), ] + super()._get_serializer_handlers() diff --git a/openmodels/serializers/sklearn/sklearn_serializer.py b/openmodels/serializers/sklearn/sklearn_serializer.py index 8a7e712..000bdff 100644 --- a/openmodels/serializers/sklearn/sklearn_serializer.py +++ b/openmodels/serializers/sklearn/sklearn_serializer.py @@ -8,12 +8,13 @@ from typing import Any, Callable, Dict, List, Tuple, Type, Optional, Union import numpy as np import inspect +from scipy.sparse import issparse # type: ignore from ._custom_estimator import load_custom_estimators import sklearn from sklearn.calibration import _CalibratedClassifier, _SigmoidCalibration -from sklearn.cluster._birch import _CFNode +from sklearn.cluster._birch import _CFNode, _CFSubcluster from sklearn.cluster._bisect_k_means import _BisectingTree from sklearn.ensemble._hist_gradient_boosting.predictor import TreePredictor from sklearn.ensemble._hist_gradient_boosting.binning import _BinMapper @@ -99,7 +100,7 @@ # Dictionary of attribute exceptions ATTRIBUTE_EXCEPTIONS: Dict[str, List] = { # Regressors: - "PLSRegression": ["_x_mean", "_predict_1d"], + "PLSRegression": ["_x_mean", "_x_std", "_y_mean", "_y_std", "_predict_1d"], "SVR": [ "_sparse", "_n_support", @@ -132,10 +133,10 @@ "_bin_mapper", ], "RadiusNeighborsRegressor": ["_fit_method", "_fit_X", "_y"], - "CCA": ["_x_mean", "_predict_1d"], + "CCA": ["_x_mean", "_x_std", "_y_mean", "_y_std", "_predict_1d"], "GammaRegressor": ["_base_loss"], "PoissonRegressor": ["_base_loss"], - "PLSCanonical": ["_x_mean", "_predict_1d"], + "PLSCanonical": ["_x_mean", "_x_std", "_y_mean", "_y_std", "_predict_1d"], "IsotonicRegression": ["f_"], "TransformedTargetRegressor": ["_training_dim"], # Clusters: @@ -199,7 +200,7 @@ "MissingIndicator": ["_n_features", "_precomputed"], "MultiLabelBinarizer": ["_cached_dict"], "PolynomialFeatures": ["_max_degree", "_n_out_full", "_min_degree"], - "PLSSVD": ["_x_mean", "_x_std"], + "PLSSVD": ["_x_mean", "_x_std", "_y_mean", "_y_std"], "TargetEncoder": ["_infrequent_enabled"], # Others: "IsolationForest": [ @@ -292,6 +293,11 @@ def __init__( else {} ) self._all_estimators: Dict[str, Type] = {**ALL_ESTIMATORS, **extra} + # Scratch state for one deserialize() call: (node, "prev_leaf_"|"next_leaf_") pairs + # a Birch _CFNode's leaf-chain pointer couldn't resolve within its own subtree (root_ + # and dummy_leaf_ are independently-deserialized top-level attributes; the pointer + # crossing between them is fixed up once both are set, see _resolve_birch_leaf_links). + self._birch_pending_leaf_links: List[Tuple[Any, str]] = [] # --- Helpers --- def _check_version(self, stored_version: Optional[str]) -> None: @@ -394,6 +400,18 @@ def _get_nested_types(self, item: Any) -> Any: elif isinstance(item, BaseEstimator): # For estimators, return their class name instead of just 'BaseEstimator' return item.__class__.__name__ + elif isinstance(item, np.dtype): + # Normalize to a single stable tag: concrete np.dtype instances are actually + # instances of numpy-internal per-dtype subclasses (Float64DType, Int64DType, + # BoolDType, ...), so type(item).__name__ is not a stable/registrable tag. + return "dtype" + elif issparse(item): + # Normalize every scipy sparse container (csr_matrix, csc_matrix, csr_array, + # csc_array, ...) to the one tag ScipySerializerMixin actually registers a + # deserializer for - _serialize_csr_matrix already converts any of them to csr_matrix + # via the csr_matrix(value) constructor, so type(item).__name__ (e.g. "csr_array") + # would tag a value the deserializer dispatch table has no matching entry for. + return "csr_matrix" else: # Return the type name if it's not a list or it's an empty list return type(item).__name__ @@ -498,9 +516,17 @@ def _get_deserializer_handlers(self): # --- Sklearn specific serializers/deserializers --- def _serialize_bisecting_tree(self, tree: _BisectingTree) -> dict: + # center/indices are serialized with an explicit dtype (rather than relying on the + # generic convert_to_serializable, which loses dtype via a plain .tolist()) so that + # e.g. a float32-fitted tree doesn't get silently widened to JSON's float64 on the + # way back - predict()'s Cython inner loop requires the exact original buffer dtype. + center = np.asarray(tree.center) + indices = np.asarray(tree.indices) return { - "center": self.convert_to_serializable(tree.center), - "indices": self.convert_to_serializable(tree.indices), + "center": self.convert_to_serializable(center), + "center_dtype": str(center.dtype), + "indices": self.convert_to_serializable(indices), + "indices_dtype": str(indices.dtype), "score": tree.score, "label": getattr(tree, "label", None), "left": self._serialize_bisecting_tree(tree.left) if tree.left else None, @@ -511,8 +537,8 @@ def _deserialize_bisecting_tree(self, data: dict) -> _BisectingTree: if data is None: return None node = _BisectingTree( - center=self.convert_from_serializable(data["center"]), - indices=self.convert_from_serializable(data["indices"]), + center=np.array(data["center"], dtype=data.get("center_dtype")), + indices=np.array(data["indices"], dtype=data.get("indices_dtype")), score=data["score"], ) if data.get("label") is not None: @@ -544,26 +570,175 @@ def _deserialize_calibrated_classifier( ) def _serialize_cfnode(self, node: _CFNode) -> Dict[str, Any]: - """Recursively serialize a _CFNode.""" - return { - "threshold": node.threshold, - "branching_factor": node.branching_factor, - "is_leaf": node.is_leaf, - "n_features": node.n_features, - # dtype=X.dtype, - } - - def _deserialize_cfnode(self, data: dict) -> _CFNode: + """ + Recursively serialize a Birch _CFNode and its full subtree of _CFSubcluster/_CFNode + descendants (a _CFSubcluster's `child_` is where the tree actually recurses one level + down). Captures the real fitted state - not just the node's scalar config - so + predict()/partial_fit() keep working after a round-trip. + + Also captures the doubly-linked prev_leaf_/next_leaf_ chain Birch threads across leaf + nodes only (used by Birch._get_leaves() for fast traversal). A leaf's neighbor can live + outside this node's own subtree - e.g. root_'s leftmost leaf's prev_leaf_ is + Birch.dummy_leaf_, a sibling top-level attribute serialized independently - such refs + are tagged "external" and cross-linked after deserialization by + _resolve_birch_leaf_links, since node_id namespaces are local to each top-level + _serialize_cfnode call. + """ + node_ids: Dict[int, int] = {} + + def get_id(n: _CFNode) -> int: + key = id(n) + if key not in node_ids: + node_ids[key] = len(node_ids) + return node_ids[key] + + # Assign every reachable node an id up front so leaf_ref can resolve a ref to a node + # not yet visited by the (depth-first) serialize_node walk below. + def collect(n: _CFNode) -> None: + get_id(n) + for sub in n.subclusters_: + if sub.child_ is not None: + collect(sub.child_) + + collect(node) + + def leaf_ref(neighbor: Optional[_CFNode]) -> Union[int, str, None]: + if neighbor is None: + return None + return node_ids.get(id(neighbor), "external") + + def serialize_subcluster(sub: _CFSubcluster) -> Dict[str, Any]: + centroid = np.asarray(sub.centroid_) + linear_sum = np.asarray(sub.linear_sum_) + return { + "n_samples_": sub.n_samples_, + # squared_sum_/sq_norm_ are np.dot(...) results - numpy scalars (e.g. + # np.float32), not plain Python floats - so they need convert_to_serializable's + # np.generic handling (.item()) to be JSON-safe. + "squared_sum_": self.convert_to_serializable(sub.squared_sum_), + "sq_norm_": self.convert_to_serializable(sub.sq_norm_), + "linear_sum_": self.convert_to_serializable(linear_sum), + "linear_sum_dtype": str(linear_sum.dtype), + "centroid_": self.convert_to_serializable(centroid), + "centroid_dtype": str(centroid.dtype), + "child": serialize_node(sub.child_) if sub.child_ is not None else None, + } + + def serialize_node(n: _CFNode) -> Dict[str, Any]: + return { + "node_id": get_id(n), + "threshold": n.threshold, + "branching_factor": n.branching_factor, + "is_leaf": n.is_leaf, + "n_features": n.n_features, + "dtype": str(n.init_centroids_.dtype), + "subclusters": [serialize_subcluster(sub) for sub in n.subclusters_], + "prev_leaf_ref": leaf_ref(n.prev_leaf_) if n.is_leaf else None, + "next_leaf_ref": leaf_ref(n.next_leaf_) if n.is_leaf else None, + } + + return serialize_node(node) + + def _deserialize_cfnode(self, data: dict) -> Optional[_CFNode]: + """ + Deserialize a Birch _CFNode subtree (inverse of _serialize_cfnode). Rebuilds each node + by replaying the real _CFNode.append_subcluster()/_CFSubcluster construction path + rather than hand-assembling the init_centroids_/centroids_ view relationship, for + structural parity with what Birch.fit() itself produces. + + Old (pre-fix) serialized files only ever captured a node's scalar config (no + "subclusters"/"node_id"/leaf-ref keys) - those fields are read via .get() with the + same defaults the old stub used, so old files keep deserializing to the same + structure-less (broken-but-non-crashing) node as before, not retroactively fixed. + """ if data is None: return None - node = _CFNode( - threshold=data["threshold"], - branching_factor=data["branching_factor"], - is_leaf=data["is_leaf"], - n_features=data["n_features"], - dtype=np.float64, # or use dtype from centroids if needed - ) - return node + + nodes: Dict[int, _CFNode] = {} + + def deserialize_subcluster(sub_data: dict) -> _CFSubcluster: + linear_sum = np.array( + self.convert_from_serializable(sub_data["linear_sum_"]), + dtype=sub_data.get("linear_sum_dtype"), + ) + subcluster = _CFSubcluster(linear_sum=linear_sum) + subcluster.n_samples_ = sub_data["n_samples_"] + subcluster.squared_sum_ = sub_data["squared_sum_"] + subcluster.sq_norm_ = sub_data["sq_norm_"] + subcluster.centroid_ = np.array( + self.convert_from_serializable(sub_data["centroid_"]), + dtype=sub_data.get("centroid_dtype"), + ) + if sub_data.get("child") is not None: + subcluster.child_ = deserialize_node(sub_data["child"]) + return subcluster + + def deserialize_node(node_data: dict) -> _CFNode: + node = _CFNode( + threshold=node_data["threshold"], + branching_factor=node_data["branching_factor"], + is_leaf=node_data["is_leaf"], + n_features=node_data["n_features"], + dtype=np.dtype(node_data.get("dtype") or np.float64), + ) + node_id = node_data.get("node_id") + if node_id is not None: + nodes[node_id] = node + for sub_data in node_data.get("subclusters", []): + node.append_subcluster(deserialize_subcluster(sub_data)) + return node + + root = deserialize_node(data) + + # Second pass: wire up leaf refs now that every node in this subtree has been built + # (and thus has an id in `nodes`, regardless of forward/backward reference order). + def link_leaves(node_data: dict) -> None: + if node_data["is_leaf"]: + node = nodes[node_data["node_id"]] + for ref_key, attr in ( + ("prev_leaf_ref", "prev_leaf_"), + ("next_leaf_ref", "next_leaf_"), + ): + ref = node_data.get(ref_key) + if ref is None: + setattr(node, attr, None) + elif ref == "external": + self._birch_pending_leaf_links.append((node, attr)) + else: + setattr(node, attr, nodes[ref]) + for sub_data in node_data.get("subclusters", []): + if sub_data.get("child") is not None: + link_leaves(sub_data["child"]) + + if "node_id" in data: + link_leaves(data) + + return root + + def _resolve_birch_leaf_links(self, model: BaseEstimator) -> None: + """ + Cross-link the two independently-deserialized Birch._CFNode graphs (root_ and + dummy_leaf_): dummy_leaf_.next_leaf_ always points at the current globally-leftmost + leaf inside root_'s tree, and that leaf's prev_leaf_ points back at dummy_leaf_. Since + root_ and dummy_leaf_ are deserialized as independent top-level attributes (in + whichever order they appear in the serialized data), _deserialize_cfnode can't resolve + this pointer itself - it queues each side on self._birch_pending_leaf_links, and this + is called once both attributes are guaranteed to be set. + """ + pending = self._birch_pending_leaf_links + self._birch_pending_leaf_links = [] + prev_pending = [node for node, attr in pending if attr == "prev_leaf_"] + next_pending = [node for node, attr in pending if attr == "next_leaf_"] + if len(prev_pending) == 1 and len(next_pending) == 1: + prev_pending[0].prev_leaf_ = next_pending[0] + next_pending[0].next_leaf_ = prev_pending[0] + elif pending: + warnings.warn( + "Could not resolve Birch's dummy_leaf_/root_ leaf-chain link after " + "deserialization (unexpected pending link shape); partial_fit()/predict() may " + "not walk the full leaf chain.", + UserWarning, + ) def _serialize_tree(self, tree: Tree) -> Dict[str, Any]: """ @@ -700,17 +875,20 @@ def _serialize_kdtree(self, value: KDTree) -> Dict[str, Any]: Serializes a KDTree object to a dictionary. """ # For KDTree, we'll use a simpler approach - just serialize the essential data - # and let the tree be reconstructed from the data + # and let the tree be reconstructed from the data. dtype is captured explicitly + # (same reasoning as _serialize_bisecting_tree) so non-float64 data isn't silently + # widened by the generic JSON round-trip. data = np.array(value.data) return { "data": self.convert_to_serializable(data), + "data_dtype": str(data.dtype), } def _deserialize_kdtree(self, kdtree_data: Dict[str, Any]) -> KDTree: """ Deserializes a dictionary representation of a KDTree back to a KDTree object. """ - data = np.array(kdtree_data["data"]) + data = np.array(kdtree_data["data"], dtype=kdtree_data.get("data_dtype")) # Create KDTree with data - the tree will be rebuilt automatically return KDTree(data) @@ -945,6 +1123,9 @@ def deserialize(self, data: Dict[str, Any]) -> BaseEstimator: # Version control check self._check_version(data.get("producer_version")) + # Reset per-call scratch state used by _deserialize_cfnode/_resolve_birch_leaf_links. + self._birch_pending_leaf_links = [] + estimator_class = data["estimator_class"] if estimator_class in NOT_SUPPORTED_ESTIMATORS: raise UnsupportedEstimatorError( @@ -1008,4 +1189,7 @@ def deserialize(self, data: Dict[str, Any]) -> BaseEstimator: self.convert_from_serializable(value, attr_type, attr_dtype), ) + if estimator_class == "Birch" and self._birch_pending_leaf_links: + self._resolve_birch_leaf_links(model) + return model diff --git a/openmodels/test_helpers.py b/openmodels/test_helpers.py index 254ce2d..336514c 100644 --- a/openmodels/test_helpers.py +++ b/openmodels/test_helpers.py @@ -3,8 +3,19 @@ models using the `openmodels` library. """ +import functools import os -from typing import Optional, Union, Protocol, runtime_checkable, TypeVar, cast +from contextlib import contextmanager +from typing import ( + Iterator, + Optional, + Type, + Union, + Protocol, + runtime_checkable, + TypeVar, + cast, +) import numpy as np from numpy import testing from sklearn.base import BaseEstimator @@ -45,6 +56,90 @@ def fit( ModelType = Union[PredictorModel, TransformerModel, FittableModel] T = TypeVar("T", bound=Union[BaseEstimator, ModelType]) +_ROUNDTRIPPABLE_METHODS = ("fit", "fit_transform", "fit_predict") + + +@contextmanager +def roundtrip_fit( + *classes: Type[BaseEstimator], format_name: str = "json" +) -> Iterator[None]: + """ + Monkeypatch fit()/fit_transform()/fit_predict() on the given estimator classes so that, + immediately after fitting, the estimator's *fitted* attributes are replaced with the result + of an openmodels serialize -> deserialize round-trip. Restores the original methods on exit. + + Only methods defined directly on the class are patched (not inherited ones), since e.g. + TransformerMixin.fit_transform already calls self.fit(...) and would be covered by patching + fit alone; only classes that override fit_transform/fit_predict directly need those patched. + + Only attributes openmodels actually serializes as fitted state (per + SklearnSerializer._extract_estimator_attributes - sklearn's trailing-underscore convention, + plus the small set of private attributes some estimators need at predict time) are copied + back onto the original instance. Constructor-set parameters are deliberately left untouched: + JSON has no tuple type, so e.g. a tuple-valued param would come back as a list after a + round-trip even though openmodels never claims to preserve exact param typing - copying it + over would make the *test infrastructure* look like a fidelity bug in checks like + check_dont_overwrite_parameters. + + This lets sklearn's own estimator-specific test suites be reused unmodified against + round-tripped models: any assertion they make about a fitted estimator becomes, implicitly, + an openmodels fidelity check. + """ + serializer = SklearnSerializer() + manager = SerializationManager(serializer) + originals: dict = {} + + def _roundtrip(instance) -> None: + fitted_keys = serializer._extract_estimator_attributes(instance).keys() + serialized = manager.serialize(instance, format_name=format_name) + deserialized = manager.deserialize(serialized, format_name=format_name) + for key in fitted_keys: + if hasattr(deserialized, key): + setattr(instance, key, getattr(deserialized, key)) + + class _RoundtripDescriptor: + """ + Wraps `orig` (a plain function, or a descriptor like sklearn's @available_if or a + property) so attribute access still goes through __get__ first. This matters because + many sklearn meta-estimators (Pipeline, FeatureAgglomeration, ...) use + hasattr(estimator, "fit_transform")-style availability checks that rely on the + descriptor's __get__ raising AttributeError when the method isn't actually available + (e.g. a Pipeline whose last step has no transform()). Replacing the class attribute + with a plain function would make it unconditionally "available", breaking that check + for everyone who does hasattr() introspection - not just this test suite. + """ + + def __init__(self, orig): + self._orig = orig + + def __get__(self, obj, objtype=None): + if obj is None: + return self + # Raises AttributeError here (not inside the call below) if unavailable, exactly + # like normal attribute access on the original descriptor would. + bound_orig = self._orig.__get__(obj, objtype) + + @functools.wraps(bound_orig) + def wrapper(*args, **kwargs): + result = bound_orig(*args, **kwargs) + _roundtrip(obj) + return result + + return wrapper + + for cls in classes: + for method_name in _ROUNDTRIPPABLE_METHODS: + if method_name in vars(cls): + orig = vars(cls)[method_name] + originals[(cls, method_name)] = orig + setattr(cls, method_name, _RoundtripDescriptor(orig)) + + try: + yield + finally: + for (cls, method_name), orig in originals.items(): + setattr(cls, method_name, orig) + def ensure_correct_sparse_format( x: Union[np.ndarray, csr_matrix], diff --git a/pyproject.toml b/pyproject.toml index 7153574..a68b421 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "openmodels" -version = "0.1.0-alpha.21" +version = "0.1.0-alpha.22" description = "Export scikit-learn model files to JSON for sharing or deploying predictive models with peace of mind." authors = [ "Alejandro Gutierrez , Pau Cabaneros , Raúl Marín ", diff --git a/test/_estimator_construction.py b/test/_estimator_construction.py new file mode 100644 index 0000000..6ee89d2 --- /dev/null +++ b/test/_estimator_construction.py @@ -0,0 +1,122 @@ +""" +Shared source of truth for "what does it take to construct this estimator at all" - mostly +meta-estimators that wrap another estimator or composite transformers that require +sub-components, which raise TypeError if built with bare defaults. + +This is used two ways: +- test_estimator_conformance.py imports CONSTRUCTOR_ARGS/construct() wholesale: it only needs + *unfitted* instances (sklearn's own check_estimator machinery fits them with its own + synthetic data), so the minimal entries here are sufficient on their own. +- test_classification.py/test_regression.py/test_transformation.py import individual + CONSTRUCTOR_ARGS[name] entries (via BASE_CLASSIFIER/BASE_REGRESSOR or the dict directly) for + the subset of estimators where their own special-casing needs nothing more than "a valid + estimator to wrap" - avoiding maintaining the same "this class needs an estimator= kwarg" + fact independently in multiple files (see the sklearn-1.9-compat branch's audit notes: this + already caused HalvingGridSearchCV support to be added in only one of two files that needed + it). Where a file's fitting data legitimately calls for a richer choice than the minimal + default here (e.g. StackingRegressor/VotingRegressor mixing in a RandomForestRegressor for + more realistic coverage, or ColumnTransformer/FeatureUnion needing column selectors matched + to real data), that file keeps its own local, richer construction instead of using this + registry - this file only holds the bare minimum needed to construct, not every reasonable + choice. + +This is deliberately separate from the fit-data special-casing in test_regression.py, +test_classification.py, etc. - those shape input data for this repo's own smoke tests, which +is a different concern from what's required to construct the estimator at all. +""" + +from typing import Any, Dict + +from sklearn.linear_model import LinearRegression, LogisticRegression +from sklearn.preprocessing import OneHotEncoder, StandardScaler + +BASE_CLASSIFIER = LogisticRegression(solver="lbfgs") +BASE_REGRESSOR = LinearRegression() + +CONSTRUCTOR_ARGS: Dict[str, Dict[str, Any]] = { + "ClassifierChain": {"estimator": BASE_CLASSIFIER}, + "FixedThresholdClassifier": {"estimator": BASE_CLASSIFIER}, + "TunedThresholdClassifierCV": {"estimator": BASE_CLASSIFIER}, + "OneVsOneClassifier": {"estimator": BASE_CLASSIFIER}, + "OneVsRestClassifier": {"estimator": BASE_CLASSIFIER}, + "OutputCodeClassifier": {"estimator": BASE_CLASSIFIER}, + "SelfTrainingClassifier": {"estimator": BASE_CLASSIFIER}, + "MultiOutputClassifier": {"estimator": BASE_CLASSIFIER}, + "StackingClassifier": { + "estimators": [ + ("lr1", BASE_CLASSIFIER), + ("lr2", LogisticRegression(solver="lbfgs", random_state=1)), + ] + }, + "VotingClassifier": { + "estimators": [ + ("lr1", BASE_CLASSIFIER), + ("lr2", LogisticRegression(solver="lbfgs", random_state=1)), + ] + }, + "MultiOutputRegressor": {"estimator": BASE_REGRESSOR}, + "RegressorChain": {"estimator": BASE_REGRESSOR}, + "StackingRegressor": { + "estimators": [("lr1", BASE_REGRESSOR), ("lr2", LinearRegression())] + }, + "VotingRegressor": { + "estimators": [("lr1", BASE_REGRESSOR), ("lr2", LinearRegression())] + }, + "SelectFromModel": {"estimator": BASE_CLASSIFIER}, + "SequentialFeatureSelector": {"estimator": BASE_CLASSIFIER}, + "RFE": {"estimator": BASE_CLASSIFIER}, + "RFECV": {"estimator": BASE_CLASSIFIER}, + "ColumnTransformer": { + "transformers": [ + ("num", StandardScaler(), [0, 1]), + ("cat", OneHotEncoder(), [2]), + ] + }, + "FeatureUnion": {"transformer_list": [("scaler", StandardScaler())]}, + "GridSearchCV": { + "estimator": LogisticRegression(solver="lbfgs"), + "param_grid": {"C": [1.0]}, + }, + "RandomizedSearchCV": { + "estimator": LogisticRegression(solver="lbfgs"), + "param_distributions": {"C": [1.0]}, + "n_iter": 1, + }, + "Pipeline": {"steps": [("clf", LogisticRegression(solver="lbfgs"))]}, + # Default hyperparameters that are incompatible with the tiny synthetic datasets + # sklearn's own check_estimator machinery generates internally (unrelated to openmodels + # round-trip fidelity - these would fail identically with no serialization involved). + "SparseRandomProjection": {"n_components": 2}, + "GaussianRandomProjection": {"n_components": 2}, + "TSNE": {"perplexity": 5}, + "CCA": {"n_components": 1}, + "PLSCanonical": {"n_components": 1}, + "PLSSVD": {"n_components": 1}, +} + +# Composite/meta transformers whose valid construction and/or expected input shape is too +# specific to the estimator (column selectors, dictionaries sized to a particular n_features, +# text-only input, y-only fit/transform) to be exercised meaningfully by sklearn's generic, +# single-2D-X check battery. They're still covered by this repo's own smoke tests +# (test_transformation.py), which know how to build fitting data for them. +NOT_CHECKED: Dict[str, str] = { + "ColumnTransformer": "column selectors are tied to a specific input shape/dtype", + "FeatureUnion": "sub-transformers are tied to a specific input shape", + "SparseCoder": "dictionary must be sized to match n_features", + "DictVectorizer": "expects list-of-dicts input, not a 2D array", + "HashingVectorizer": "expects text input, not a 2D array", + "FeatureHasher": "expects dicts/strings input, not a 2D array", + "LabelBinarizer": "fits/transforms on y only, not X", + "LabelEncoder": "fits/transforms on y only, not X", + "MultiLabelBinarizer": "fits/transforms on label collections, not a 2D array", + "FrozenEstimator": "wraps an already-fitted estimator; fit() is a no-op by design", + "SpectralBiclustering": "n_best/n_components/n_clusters interact and need per-check tuning to fit tiny synthetic data - not an openmodels round-trip issue", + "SpectralCoclustering": "n_best/n_components/n_clusters interact and need per-check tuning to fit tiny synthetic data - not an openmodels round-trip issue", + "HalvingGridSearchCV": "successive-halving resource scheduling needs more samples than check_estimator's tiny synthetic datasets provide - not an openmodels round-trip issue", + "HalvingRandomSearchCV": "successive-halving resource scheduling needs more samples than check_estimator's tiny synthetic datasets provide - not an openmodels round-trip issue", +} + + +def construct(cls: type) -> Any: + """Instantiate `cls` with its default constructor, or the required kwargs above.""" + return cls(**CONSTRUCTOR_ARGS.get(cls.__name__, {})) diff --git a/test/test_birch_cftree.py b/test/test_birch_cftree.py new file mode 100644 index 0000000..4c4f9b4 --- /dev/null +++ b/test/test_birch_cftree.py @@ -0,0 +1,74 @@ +""" +Structural invariant checks for Birch's root_/dummy_leaf_ CF-tree round-trip. + +Generic predict()/transform() checks don't exercise root_/dummy_leaf_ at all (those methods +only use subcluster_centers_/subcluster_labels_/_subcluster_norms), so a broken leaf chain +would pass predict()-only testing silently. These checks walk the tree directly. +""" + +import numpy as np +from sklearn.cluster import Birch + +from openmodels import SerializationManager, SklearnSerializer + + +def _leaves(model): + leaves = [] + leaf = model.dummy_leaf_.next_leaf_ + while leaf is not None: + leaves.append(leaf) + leaf = leaf.next_leaf_ + return leaves + + +def test_birch_leaf_chain_round_trips(): + X = np.random.RandomState(0).rand(200, 4) + model = Birch(n_clusters=3, threshold=0.3, branching_factor=10) + model.fit(X) + + leaves_before = _leaves(model) + n_subclusters_before = sum(len(leaf.subclusters_) for leaf in leaves_before) + + manager = SerializationManager(SklearnSerializer()) + restored = manager.deserialize(manager.serialize(model)) + + leaves_after = _leaves(restored) + n_subclusters_after = sum(len(leaf.subclusters_) for leaf in leaves_after) + + assert len(leaves_after) == len(leaves_before) + assert n_subclusters_after == n_subclusters_before + assert restored.dummy_leaf_.next_leaf_ is leaves_after[0] + assert leaves_after[0].prev_leaf_ is restored.dummy_leaf_ + assert leaves_after[-1].next_leaf_ is None + + np.testing.assert_allclose(model.subcluster_centers_, restored.subcluster_centers_) + + +def test_birch_partial_fit_after_round_trip(): + # This is the scenario the original bug affected: predict()/transform() never touch + # root_/dummy_leaf_, but partial_fit() walks the leaf chain via Birch._get_leaves() to + # recompute subcluster_centers_, which crashed with "need at least one array to + # concatenate" when the tree structure was lost on deserialize. + X = np.random.RandomState(1).rand(100, 3) + model = Birch(n_clusters=2, threshold=0.5, branching_factor=10) + model.fit(X) + + manager = SerializationManager(SklearnSerializer()) + restored = manager.deserialize(manager.serialize(model)) + + restored.partial_fit(X[:10]) + assert restored.subcluster_centers_.shape[1] == X.shape[1] + + +def test_birch_float32_center_dtype_preserved(): + X = np.random.RandomState(2).rand(60, 4).astype(np.float32) + model = Birch(n_clusters=2, threshold=0.3, branching_factor=10) + model.fit(X) + + manager = SerializationManager(SklearnSerializer()) + restored = manager.deserialize(manager.serialize(model)) + + for leaf in _leaves(restored): + assert leaf.centroids_.dtype == np.float32 + for sub in leaf.subclusters_: + assert sub.centroid_.dtype == np.float32 diff --git a/test/test_classification.py b/test/test_classification.py index 47acda2..3794857 100644 --- a/test/test_classification.py +++ b/test/test_classification.py @@ -1,10 +1,10 @@ import pytest import numpy as np -from sklearn.linear_model import LogisticRegression from sklearn.utils.discovery import all_estimators from sklearn.datasets import make_classification from openmodels.test_helpers import run_test_model from openmodels.serializers.sklearn.sklearn_serializer import NOT_SUPPORTED_ESTIMATORS +from test._estimator_construction import BASE_CLASSIFIER, CONSTRUCTOR_ARGS # Get all classifier estimators, filtering out not supported classifiers CLASSIFIERS = [cls for name, cls in all_estimators(type_filter="classifier") @@ -48,31 +48,29 @@ def test_classifier(Classifier, data): args = {} abs = False - base_lr = LogisticRegression(solver='lbfgs', random_state=0) + base_lr = BASE_CLASSIFIER if Classifier.__name__ in ["CategoricalNB", "ComplementNB", "MultinomialNB"]: abs = True elif Classifier.__name__ == "ClassifierChain": - args["estimator"] = base_lr + args.update(CONSTRUCTOR_ARGS["ClassifierChain"]) y_multi = np.column_stack([(y == i).astype(int) for i in np.unique(y)]) y = y_multi elif Classifier.__name__ in ["FixedThresholdClassifier", "TunedThresholdClassifierCV"]: - args["estimator"] = base_lr + args.update(CONSTRUCTOR_ARGS[Classifier.__name__]) y_binary = (y == 0).astype(int) y = y_binary elif Classifier.__name__ in ["OneVsOneClassifier", "OutputCodeClassifier", "SelfTrainingClassifier"]: - args["estimator"] = base_lr + args.update(CONSTRUCTOR_ARGS[Classifier.__name__]) elif Classifier.__name__ in ["MultiOutputClassifier", "OneVsRestClassifier"]: - args["estimator"] = base_lr + args.update(CONSTRUCTOR_ARGS[Classifier.__name__]) y_multi = np.column_stack([(y == i).astype(int) for i in np.unique(y)]) y = y_multi elif Classifier.__name__ == "StackingClassifier": + # Data-dependent (one sub-estimator per class label) - can't be a static shared entry. args["estimators"] = [(str(name), base_lr) for name in np.unique(y)] elif Classifier.__name__ == "VotingClassifier": - args["estimators"] = [ - ("lr", LogisticRegression(solver='lbfgs', random_state=0)), - ("lr2", LogisticRegression(solver='lbfgs', random_state=1)) - ] + args.update(CONSTRUCTOR_ARGS["VotingClassifier"]) classifier = Classifier(**args) diff --git a/test/test_estimator_conformance.py b/test/test_estimator_conformance.py new file mode 100644 index 0000000..07f087f --- /dev/null +++ b/test/test_estimator_conformance.py @@ -0,0 +1,217 @@ +""" +Runs scikit-learn's own generic estimator conformance battery +(sklearn.utils.estimator_checks.parametrize_with_checks) against openmodels-round-tripped +estimators. + +This is the same battery scikit-learn runs against itself for every estimator it ships +(check_fit_idempotent, check_methods_subset_invariance, transformer/classifier/regressor +contract checks, ...). Discovery reuses the same all_estimators() + NOT_SUPPORTED_ESTIMATORS +pattern as test_regression.py/test_classification.py/test_clustering.py/test_transformation.py, +so it automatically covers every estimator openmodels claims to support, with no per-module +mapping to maintain. + +Deliberately uses the public, version-stable parametrize_with_checks(estimators) form rather +than the private sklearn.utils._test_common.instance_generator helpers (expected_failed_checks, +_tested_estimators, ...), whose shape has already changed across the sklearn versions this repo +tests against in .github/workflows/sklearn-compat.yml. +""" + +import pytest +from sklearn.utils.discovery import all_estimators +from sklearn.utils.estimator_checks import parametrize_with_checks + +from openmodels.serializers.sklearn.sklearn_serializer import NOT_SUPPORTED_ESTIMATORS +from openmodels.test_helpers import roundtrip_fit +from test._estimator_construction import NOT_CHECKED, construct + +ESTIMATOR_CLASSES = [ + cls + for name, cls in all_estimators() + if name not in NOT_SUPPORTED_ESTIMATORS and name not in NOT_CHECKED +] + +ESTIMATORS = [construct(cls) for cls in ESTIMATOR_CLASSES] + + +def _entries(reason: str, estimator_checks: dict) -> dict: + """Expand {estimator_name: [check_name, ...]} sharing one `reason` into individual + (estimator_name, check_name) -> reason entries for KNOWN_ROUNDTRIP_XFAILS.""" + return { + (estimator_name, check_name): reason + for estimator_name, check_names in estimator_checks.items() + for check_name in check_names + } + + +# Known, understood round-trip gaps - populated from real CI failures, each with a reason, and +# always keyed per (estimator class name, check function name). Earlier revisions of this file +# grouped some entries by check name alone (e.g. "check_sample_weight_equivalence_on_dense_data +# always fails, for every estimator") on the assumption that a check failing for a handful of +# estimators meant it was fundamentally broken for everyone. Once the xfail mechanism below was +# made strict (see test_estimator_conformance_roundtrip), that assumption was caught immediately +# as inaccurate: most estimators carrying e.g. check_estimator_sparse_tag never exercise sparse +# data at all in this check and pass it fine, so a check-level skip silently hid that from ever +# running. Keying strictly per estimator is what makes strict=True meaningful - it only ever +# flags real staleness, not "this check-level skip happened to also cover an unrelated pass". +KNOWN_ROUNDTRIP_XFAILS: dict = { + ("TunedThresholdClassifierCV", "check_classifiers_train"): ( + "JSON round-trips the tuned decision threshold as a float that isn't bit-identical to " + "the original numpy float64; on this check's synthetic data one sample's decision value " + "sits close enough to the threshold that the tiny precision difference flips its " + "predicted class (1/200 samples). Not a correctness bug - a boundary-case artifact of " + "float round-tripping through JSON." + ), + ("DictionaryLearning", "check_transformer_general"): ( + "Pre-existing sklearn/check fragility, unrelated to serialization: fails identically " + "against a bare, unpatched DictionaryLearning() with check_estimator's default " + "synthetic data." + ), + ("DictionaryLearning", "check_transformer_data_not_an_array"): ( + "Same pre-existing, serialization-unrelated fragility as this estimator's " + "check_transformer_general failure." + ), + ("MiniBatchNMF", "check_transformer_general"): ( + "Pre-existing sklearn/check fragility, unrelated to serialization: fails identically " + "against a bare, unpatched MiniBatchNMF() with check_estimator's default synthetic " + "data." + ), + ("MiniBatchNMF", "check_transformer_data_not_an_array"): ( + "Same pre-existing, serialization-unrelated fragility as this estimator's " + "check_transformer_general failure." + ), + ("NMF", "check_transformer_general"): ( + "Pre-existing sklearn/check fragility, unrelated to serialization: fails identically " + "against a bare, unpatched NMF() with check_estimator's default synthetic data." + ), + ("NMF", "check_transformer_data_not_an_array"): ( + "Same pre-existing, serialization-unrelated fragility as this estimator's " + "check_transformer_general failure." + ), + ("LassoLarsIC", "check_fit2d_1sample"): ( + "Pre-existing sklearn/check fragility, unrelated to serialization: fails identically " + "against a bare, unpatched LassoLarsIC() - the expected error message pattern for a " + "1-sample fit doesn't match what this estimator actually raises." + ), + ("BernoulliRBM", "check_methods_sample_order_invariance"): ( + "Pre-existing sklearn fragility, unrelated to serialization: fails identically against " + "a bare, unpatched BernoulliRBM(). score_samples() is documented as stochastic " + "(it perturbs one feature per call for its pseudo-likelihood estimate), so exact " + "order/subset invariance isn't guaranteed even without any round-trip involved." + ), + ("BernoulliRBM", "check_methods_subset_invariance"): ( + "Same pre-existing, serialization-unrelated fragility as this estimator's " + "check_methods_sample_order_invariance failure." + ), + ("LogisticRegressionCV", "check_sparsify_coefficients"): ( + "Pre-existing sklearn/check fragility, unrelated to serialization: fails identically " + "against a bare, unpatched LogisticRegressionCV() - its internal CV splitting can't " + "satisfy n_splits=5 on this check's small per-class sample counts." + ), + ("NuSVC", "check_classifiers_one_label_sample_weights"): ( + "Pre-existing sklearn/check fragility, unrelated to serialization: fails identically " + "against a bare, unpatched NuSVC()." + ), + ("NuSVC", "check_class_weight_classifiers"): ( + "Pre-existing sklearn/check fragility, unrelated to serialization: fails identically " + "against a bare, unpatched NuSVC()." + ), + ("Pipeline", "check_estimators_overwrite_params"): ( + "Pre-existing sklearn/check fragility, unrelated to serialization: fails identically " + "against a bare, unpatched Pipeline(steps=[...])." + ), + ("Pipeline", "check_dont_overwrite_parameters"): ( + "Same pre-existing, serialization-unrelated fragility as this estimator's " + "check_estimators_overwrite_params failure." + ), +} + +# Pre-existing sklearn/check fragility, unrelated to serialization: fails identically against a +# bare, unpatched estimator for every one of these (verified directly, roundtrip_fit disabled). +# Fitting with sample_weight is not always numerically equivalent to fitting on repeated/removed +# rows, due to floating-point non-associativity - a well-known limitation of this specific check +# for ensemble/boosting/margin-based estimators. Confirmed accurate estimator-by-estimator (not +# assumed check-wide) after the strict xfail mechanism below flagged the original, too-broad +# check-level version of this entry as producing hundreds of unexpected passes. +_SAMPLE_WEIGHT_EQUIVALENCE_REASON = ( + "Pre-existing sklearn fragility, unrelated to serialization: fails identically against a " + "bare, unpatched estimator. Fitting with sample_weight is not always numerically " + "equivalent to fitting on repeated/removed rows due to floating-point non-associativity." +) +_BOTH_SAMPLE_WEIGHT_CHECKS = [ + "check_sample_weight_equivalence_on_dense_data", + "check_sample_weight_equivalence_on_sparse_data", +] +KNOWN_ROUNDTRIP_XFAILS.update( + _entries( + _SAMPLE_WEIGHT_EQUIVALENCE_REASON, + { + "AdaBoostClassifier": _BOTH_SAMPLE_WEIGHT_CHECKS, + "BaggingClassifier": _BOTH_SAMPLE_WEIGHT_CHECKS, + "BaggingRegressor": _BOTH_SAMPLE_WEIGHT_CHECKS, + "BisectingKMeans": _BOTH_SAMPLE_WEIGHT_CHECKS, + "GradientBoostingClassifier": _BOTH_SAMPLE_WEIGHT_CHECKS, + "GradientBoostingRegressor": _BOTH_SAMPLE_WEIGHT_CHECKS, + "HuberRegressor": _BOTH_SAMPLE_WEIGHT_CHECKS, + "IsolationForest": _BOTH_SAMPLE_WEIGHT_CHECKS, + "KMeans": _BOTH_SAMPLE_WEIGHT_CHECKS, + "LinearSVC": _BOTH_SAMPLE_WEIGHT_CHECKS, + "LinearSVR": _BOTH_SAMPLE_WEIGHT_CHECKS, + "MiniBatchKMeans": _BOTH_SAMPLE_WEIGHT_CHECKS, + "NuSVC": _BOTH_SAMPLE_WEIGHT_CHECKS, + "NuSVR": _BOTH_SAMPLE_WEIGHT_CHECKS, + "OneClassSVM": _BOTH_SAMPLE_WEIGHT_CHECKS, + "Perceptron": _BOTH_SAMPLE_WEIGHT_CHECKS, + "RANSACRegressor": _BOTH_SAMPLE_WEIGHT_CHECKS, + "RandomForestClassifier": _BOTH_SAMPLE_WEIGHT_CHECKS, + "RandomForestRegressor": _BOTH_SAMPLE_WEIGHT_CHECKS, + "RandomTreesEmbedding": _BOTH_SAMPLE_WEIGHT_CHECKS, + "SGDClassifier": _BOTH_SAMPLE_WEIGHT_CHECKS, + "SGDOneClassSVM": _BOTH_SAMPLE_WEIGHT_CHECKS, + "SGDRegressor": _BOTH_SAMPLE_WEIGHT_CHECKS, + "SVC": _BOTH_SAMPLE_WEIGHT_CHECKS, + "SVR": _BOTH_SAMPLE_WEIGHT_CHECKS, + }, + ) +) + +# Pre-existing sklearn fragility, unrelated to serialization: fails identically against a bare, +# unpatched estimator (verified directly, roundtrip_fit disabled). On check_estimator's tiny +# synthetic data, some solvers report n_iter_=None, or the estimator does zero iterations (e.g. +# SelfTrainingClassifier when the data happens to have no unlabeled samples) - which this check +# doesn't tolerate. +_N_ITER_REASON = ( + "Pre-existing sklearn fragility, unrelated to serialization: fails identically against a " + "bare, unpatched estimator. On check_estimator's tiny synthetic data, some solvers report " + "n_iter_=None or the estimator does zero iterations, which this check doesn't tolerate." +) +KNOWN_ROUNDTRIP_XFAILS.update( + _entries( + _N_ITER_REASON, + { + "LogisticRegressionCV": ["check_non_transformer_estimators_n_iter"], + "Ridge": ["check_non_transformer_estimators_n_iter"], + "RidgeClassifier": ["check_non_transformer_estimators_n_iter"], + "SelfTrainingClassifier": ["check_non_transformer_estimators_n_iter"], + }, + ) +) + + +def _check_name(check) -> str: + return getattr(check, "func", check).__name__ + + +@parametrize_with_checks(ESTIMATORS) +def test_estimator_conformance_roundtrip(estimator, check, request): + name = _check_name(check) + reason = KNOWN_ROUNDTRIP_XFAILS.get((type(estimator).__name__, name)) + + # Applied dynamically (rather than pytest.xfail(), which would abort here without ever + # running the check) so that strict=True can catch the check actually starting to pass - + # e.g. once one of the real gaps documented above gets fixed. Without strict mode, a fixed + # bug would keep reporting XFAIL forever and nobody would be prompted to clean up the entry. + if reason is not None: + request.applymarker(pytest.mark.xfail(reason=reason, strict=True)) + + with roundtrip_fit(type(estimator)): + check(estimator) diff --git a/test/test_others.py b/test/test_others.py index 5e4618e..f7e5f8a 100644 --- a/test/test_others.py +++ b/test/test_others.py @@ -2,16 +2,22 @@ from sklearn.utils.discovery import all_estimators from sklearn.datasets import make_classification from openmodels.test_helpers import run_test_model -from openmodels.serializers.sklearn.sklearn_serializer import NOT_SUPPORTED_ESTIMATORS +from openmodels.serializers.sklearn.sklearn_serializer import ALL_ESTIMATORS, NOT_SUPPORTED_ESTIMATORS from test.test_classification import CLASSIFIERS from test.test_clustering import CLUSTERS from test.test_regression import REGRESSORS from test.test_transformation import TRANSFORMERS -# Get all other estimators, filtering out not supported +# Get all other estimators, filtering out not supported. Also excludes anything all_estimators() +# discovers but ALL_ESTIMATORS doesn't know about: ALL_ESTIMATORS is frozen at whatever +# all_estimators() returned when sklearn_serializer.py was first imported, so an +# experimental-only estimator (e.g. HalvingGridSearchCV) that becomes discoverable later in the +# process - such as importing sklearn.utils.estimator_checks, which enables it as a side effect - +# would be constructible here but not actually deserializable by openmodels, raising KeyError. OTHERS = [cls for name, cls in all_estimators() if cls not in CLASSIFIERS + CLUSTERS + REGRESSORS + TRANSFORMERS and name not in NOT_SUPPORTED_ESTIMATORS + and name in ALL_ESTIMATORS ] # Define constants @@ -91,6 +97,15 @@ def test_others(Others, data): base_estimator = LogisticRegression() args["estimator"] = base_estimator args["param_distributions"] = param_distributions + if Others.__name__ == "HalvingGridSearchCV": + from sklearn.linear_model import LogisticRegression + args["estimator"] = LogisticRegression() + args["param_grid"] = {"C": [0.1, 1.0]} + if Others.__name__ == "HalvingRandomSearchCV": + from sklearn.linear_model import LogisticRegression + from scipy.stats import uniform + args["estimator"] = LogisticRegression() + args["param_distributions"] = {"C": uniform(0.1, 1.0)} others = Others(**args) diff --git a/test/test_regression.py b/test/test_regression.py index 656a22f..1dbf182 100644 --- a/test/test_regression.py +++ b/test/test_regression.py @@ -8,6 +8,7 @@ from sklearn.ensemble import RandomForestRegressor from openmodels.test_helpers import run_test_model from openmodels.serializers.sklearn.sklearn_serializer import NOT_SUPPORTED_ESTIMATORS +from test._estimator_construction import CONSTRUCTOR_ARGS # Get all regressor estimators, filtering out not supported regressors REGRESSORS = [cls for name, cls in all_estimators(type_filter="regressor") @@ -66,14 +67,12 @@ def test_regressor(Regressor, data): x, y = make_regression(n_samples=50, n_features=3, n_targets=2, random_state=42) x_sparse = None y_sparse = None - base_estimator = LinearRegression() - args = {"estimator": base_estimator} + args.update(CONSTRUCTOR_ARGS["MultiOutputRegressor"]) elif Regressor.__name__ in ["RegressorChain"]: x, y = make_regression(n_samples=50, n_features=3, n_targets=2, random_state=42) x_sparse = None y_sparse = None - base_estimator = LinearRegression() - args = {"estimator": base_estimator} + args.update(CONSTRUCTOR_ARGS["RegressorChain"]) elif Regressor.__name__ == "StackingRegressor": # Create simpler base estimators that are easier to serialize diff --git a/test/test_serializer_base.py b/test/test_serializer_base.py new file mode 100644 index 0000000..1550be1 --- /dev/null +++ b/test/test_serializer_base.py @@ -0,0 +1,26 @@ +from openmodels.serializers.base import SerializerMixin + + +def test_str_keyed_dict_wire_shape_unchanged(): + mixin = SerializerMixin() + value = {"a": 1, "b": 2} + serialized = mixin.convert_to_serializable(value) + assert serialized == value + + +def test_non_string_keyed_dict_round_trips(): + mixin = SerializerMixin() + value = {1: "one", 2: "two", 3: "three"} + serialized = mixin.convert_to_serializable(value) + restored = mixin.convert_from_serializable(serialized, "dict") + assert restored == value + assert {type(k) for k in restored} == {int} + + +def test_mixed_key_type_dict_round_trips(): + mixin = SerializerMixin() + value = {1: "a", "x": "b", 2.5: "c"} + serialized = mixin.convert_to_serializable(value) + restored = mixin.convert_from_serializable(serialized, "dict") + assert restored == value + assert {type(k) for k in restored} == {int, str, float} diff --git a/test/test_sparse_containers.py b/test/test_sparse_containers.py new file mode 100644 index 0000000..751e338 --- /dev/null +++ b/test/test_sparse_containers.py @@ -0,0 +1,29 @@ +import numpy as np +import pytest +import scipy.sparse as sp + +from openmodels import SerializationManager, SklearnSerializer +from sklearn.neighbors import KNeighborsClassifier + + +@pytest.mark.parametrize( + "sparse_cls", [sp.csr_matrix, sp.csc_matrix, sp.csr_array, sp.csc_array] +) +def test_estimator_with_sparse_fitted_attribute_round_trips(sparse_cls): + # KNeighborsClassifier stores its training data (whatever sparse container it was fit + # with) as the fitted attribute _fit_X - this exercises openmodels's sparse serializer + # directly, independent of which format the caller happened to pass in. + X = np.random.RandomState(0).rand(30, 4) + X[X < 0.5] = 0 + X_sparse = sparse_cls(X) + y = np.arange(30) % 3 + + model = KNeighborsClassifier(n_neighbors=3) + model.fit(X_sparse, y) + + manager = SerializationManager(SklearnSerializer()) + restored = manager.deserialize(manager.serialize(model)) + + np.testing.assert_array_equal( + model.predict(X_sparse[:5]), restored.predict(X_sparse[:5]) + ) diff --git a/test/test_transformation.py b/test/test_transformation.py index 17d9b8c..243673f 100644 --- a/test/test_transformation.py +++ b/test/test_transformation.py @@ -14,6 +14,7 @@ from openmodels.serializers.sklearn.sklearn_serializer import NOT_SUPPORTED_ESTIMATORS from test.test_regression import REGRESSORS from test.test_classification import CLASSIFIERS +from test._estimator_construction import CONSTRUCTOR_ARGS # Get all transformer estimators, filtering out not supported ones and those that are also regressors/classifiers TRANSFORMERS = [ @@ -108,8 +109,7 @@ def test_transformer(Transformer, data): ("scaler2", MinMaxScaler()) ] if Transformer.__name__ in ["SelectFromModel", "SequentialFeatureSelector", "RFE", "RFECV"]: - from sklearn.linear_model import LogisticRegression - args["estimator"] = LogisticRegression() + args.update(CONSTRUCTOR_ARGS[Transformer.__name__]) if Transformer.__name__ == "SparseCoder": # SparseCoder requires a dictionary (components) for initialization # Let's create a random dictionary with shape (n_components, n_features) diff --git a/test/upstream/__init__.py b/test/upstream/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/test/upstream/cluster/__init__.py b/test/upstream/cluster/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/test/upstream/cluster/conftest.py b/test/upstream/cluster/conftest.py new file mode 100644 index 0000000..7901917 --- /dev/null +++ b/test/upstream/cluster/conftest.py @@ -0,0 +1,10 @@ +import pytest +from sklearn.cluster import Birch + +from openmodels.test_helpers import roundtrip_fit + + +@pytest.fixture(scope="module", autouse=True) +def _roundtrip_birch(): + with roundtrip_fit(Birch): + yield diff --git a/test/upstream/cluster/test_birch_upstream.py b/test/upstream/cluster/test_birch_upstream.py new file mode 100644 index 0000000..3c559b5 --- /dev/null +++ b/test/upstream/cluster/test_birch_upstream.py @@ -0,0 +1,9 @@ +""" +Reuses scikit-learn's own Birch test suite unmodified. The autouse fixture in conftest.py +round-trips Birch through openmodels immediately after fit()/fit_transform()/fit_predict(), so +every assertion sklearn's own maintainers wrote here - including scenarios that call +partial_fit() after fit(), which is what actually exercises Birch's root_/dummy_leaf_ CF-tree +structure - doubles as an openmodels fidelity check for this estimator. +""" + +from sklearn.cluster.tests.test_birch import * # noqa: F401,F403 diff --git a/test/upstream/cross_decomposition/__init__.py b/test/upstream/cross_decomposition/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/test/upstream/cross_decomposition/conftest.py b/test/upstream/cross_decomposition/conftest.py new file mode 100644 index 0000000..5e4dc41 --- /dev/null +++ b/test/upstream/cross_decomposition/conftest.py @@ -0,0 +1,10 @@ +import pytest +from sklearn.cross_decomposition import CCA, PLSCanonical, PLSRegression, PLSSVD + +from openmodels.test_helpers import roundtrip_fit + + +@pytest.fixture(scope="module", autouse=True) +def _roundtrip_pls_family(): + with roundtrip_fit(PLSRegression, PLSCanonical, CCA, PLSSVD): + yield diff --git a/test/upstream/cross_decomposition/test_pls_upstream.py b/test/upstream/cross_decomposition/test_pls_upstream.py new file mode 100644 index 0000000..d6610b4 --- /dev/null +++ b/test/upstream/cross_decomposition/test_pls_upstream.py @@ -0,0 +1,8 @@ +""" +Reuses scikit-learn's own cross_decomposition test suite unmodified. The autouse fixture in +conftest.py round-trips PLSRegression/PLSCanonical/CCA/PLSSVD through openmodels immediately +after fit(), so every assertion sklearn's own maintainers wrote here doubles as an openmodels +fidelity check for this estimator family. +""" + +from sklearn.cross_decomposition.tests.test_pls import * # noqa: F401,F403 diff --git a/testpypi-badge.json b/testpypi-badge.json deleted file mode 100644 index 404d540..0000000 --- a/testpypi-badge.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "schemaVersion": 1, - "label": "TestPyPI", - "message": "0.1.0a21", - "color": "orange" -} \ No newline at end of file