Conversation
….remove MLflow schema validation warning for Pandera-typed inputs
fmind
left a comment
There was a problem hiding this comment.
Thanks for the contribution and for adding regression coverage for these failures.
I reviewed head 29e365b7e0df29f1b47e302b02ef8bfd6ece6dbe and found three changes needed before merging:
- P2: Apply notification limits only on Windows. The current implementation also truncates Linux and macOS messages, introducing information loss on platforms that do not use these Win32 buffers.
- P2: Count UTF-16 units. The current helper treats 32 emoji as 32 characters and leaves them unchanged for a title limit of 63, although they occupy 64 UTF-16 units. The Win32 buffers use WCHAR fields. See NOTIFYICONDATAW. Truncation must also preserve complete surrogate pairs.
- P2: Remove the training config change. Setting
max_depth: 7changes the existing default of 20, independently of either reported failure, without model-quality evidence.
The pd.DataFrame annotation is justified specifically at the MLflow adapter boundary. I checked the installed MLflow 3.15.1 implementation: it recognizes the pandas annotation but cannot infer a signature from the Pandera generic alias. Keeping InputsSchema.check() before calling the domain model retains both the typed schemas.Inputs contract and runtime constraints. The domain model should continue using schemas.Inputs.
I prepared the follow-up patch below. It removes the training override, restricts truncation to Windows, handles UTF-16 limits, preserves full console fallback text, and extends coverage to short/exact-limit/long/emoji fields, Linux/macOS behavior, and semantic validation after an actual MLflow save/load round trip.
Validation of this local patched candidate on Python 3.14.8: 57 passed, 3 existing expected failures, 100% coverage. Ruff lint/format, ty, config/lock checks, and wheel/sdist builds pass. Windows behavior is tested with the platform and notification call mocked; a native Windows notification was not exercised. Local tooling used installed uv 0.12.22 because the repository lock selects an asdf backend disabled on this workstation.
The full mise run all gate is blocked separately by the unchanged dependency baseline: pip-audit reports 20 vulnerabilities in five packages, including MLflow PYSEC-2026-3865, for which the audit lists no fixed version. The source patch leaves pyproject.toml and uv.lock unchanged. I have not committed/pushed it or bypassed the required checks. Hosted CI for the submitted head also awaits fork-workflow approval; these local results are not hosted CI results.
Tested follow-up patch against the submitted PR head
diff --git c/confs/training.yaml w/confs/training.yaml
index b110b2d..e037286 100644
--- c/confs/training.yaml
+++ w/confs/training.yaml
@@ -6,6 +6,3 @@ job:
targets:
KIND: ParquetReader
path: data/targets_train.parquet
- model:
- KIND: BaselineSklearnModel
- max_depth: 7
diff --git c/src/bikes/io/registries.py w/src/bikes/io/registries.py
index 86f1646..08e6857 100644
--- c/src/bikes/io/registries.py
+++ w/src/bikes/io/registries.py
@@ -138,8 +138,8 @@ class CustomSaver(Saver):
Returns:
schemas.Outputs: validated outputs of the project model.
"""
- # MLflow recognizes pandas annotations, but cannot infer a Pandera schema.
- # Keep the typed, validated project inputs behind this boundary.
+ # MLflow accepts pandas here and validates the logged signature;
+ # Pandera enforces the project's constraints before the typed model call.
inputs = schemas.InputsSchema.check(data=model_input)
return self.model.predict(inputs=inputs)
diff --git c/src/bikes/io/services.py w/src/bikes/io/services.py
index 0a1845a..550f79a 100644
--- c/src/bikes/io/services.py
+++ w/src/bikes/io/services.py
@@ -101,6 +101,7 @@ class AlertsService(Service):
app_name: str = "Bikes"
timeout: int | None = None
+ # Plyer's NOTIFYICONDATAW buffers reserve one UTF-16 unit for the terminator.
_MAX_APP_NAME_LENGTH: T.ClassVar[int] = 127
_MAX_TITLE_LENGTH: T.ClassVar[int] = 63
_MAX_MESSAGE_LENGTH: T.ClassVar[int] = 255
@@ -117,11 +118,16 @@ class AlertsService(Service):
message (str): message of the notification.
"""
if self.enable:
+ notify_title, notify_message, app_name = title, message, self.app_name
+ if sys.platform == "win32":
+ notify_title = self._truncate(title, self._MAX_TITLE_LENGTH)
+ notify_message = self._truncate(message, self._MAX_MESSAGE_LENGTH)
+ app_name = self._truncate(app_name, self._MAX_APP_NAME_LENGTH)
try:
notification.notify(
- title=self._truncate(title, self._MAX_TITLE_LENGTH),
- message=self._truncate(message, self._MAX_MESSAGE_LENGTH),
- app_name=self._truncate(self.app_name, self._MAX_APP_NAME_LENGTH),
+ title=notify_title,
+ message=notify_message,
+ app_name=app_name,
timeout=self.timeout,
)
except NotImplementedError:
@@ -132,10 +138,12 @@ class AlertsService(Service):
@staticmethod
def _truncate(value: str, max_length: int) -> str:
- """Truncate a notification field to a platform-safe length."""
- if len(value) <= max_length:
+ """Fit a Windows field in UTF-16 units without splitting a surrogate pair."""
+ encoded = value.encode("utf-16-le")
+ if len(encoded) <= max_length * 2:
return value
- return f"{value[: max_length - 1]}\N{HORIZONTAL ELLIPSIS}"
+ prefix = encoded[: (max_length - 1) * 2].decode("utf-16-le", errors="ignore")
+ return f"{prefix}\N{HORIZONTAL ELLIPSIS}"
def _print(self, title: str, message: str) -> None:
"""Print a notification to the system.
diff --git c/tests/io/test_registries.py w/tests/io/test_registries.py
index cb00537..7b53b01 100644
--- c/tests/io/test_registries.py
+++ w/tests/io/test_registries.py
@@ -116,6 +116,11 @@ def test_custom_pipeline(
)
# - output
assert schemas.OutputsSchema.check(outputs) is not None, "Outputs should be valid!"
+ # The serialized PyFunc must retain Pandera constraints beyond MLflow's column types.
+ invalid_inputs = inputs.copy()
+ invalid_inputs.loc[:, "hr"] = 24
+ with pytest.raises(pe.SchemaError, match="less_than_or_equal_to"):
+ adapter.predict(inputs=invalid_inputs)
def test_builtin_pipeline(
diff --git c/tests/io/test_services.py w/tests/io/test_services.py
index 3fc543d..beaa8fa 100644
--- c/tests/io/test_services.py
+++ w/tests/io/test_services.py
@@ -59,19 +59,61 @@ def test_alerts_service__not_supported(mocker: pm.MockerFixture, capsys: pc.Capt
assert "Notifications are not supported on this system." in capsys.readouterr().out
-def test_alerts_service__truncates_long_fields(mocker: pm.MockerFixture) -> None:
+@pytest.mark.parametrize(
+ ("title", "message", "app_name", "expected_title", "expected_message", "expected_app_name"),
+ [
+ ("test", "hello", "Bikes", "test", "hello", "Bikes"),
+ ("t" * 63, "m" * 255, "a" * 127, "t" * 63, "m" * 255, "a" * 127),
+ ("t" * 64, "m" * 376, "a" * 128, "t" * 62 + "…", "m" * 254 + "…", "a" * 126 + "…"),
+ ("😀" * 32, "😀" * 128, "😀" * 64, "😀" * 31 + "…", "😀" * 127 + "…", "😀" * 63 + "…"),
+ ("t" + "😀" * 32, "m" * 376, "a" * 128, "t" + "😀" * 30 + "…", "m" * 254 + "…", "a" * 126 + "…"),
+ ],
+)
+def test_alerts_service__windows_limits(
+ mocker: pm.MockerFixture,
+ title: str,
+ message: str,
+ app_name: str,
+ expected_title: str,
+ expected_message: str,
+ expected_app_name: str,
+) -> None:
# given
- service = services.AlertsService(enable=True, app_name="a" * 128)
+ mocker.patch("bikes.io.services.sys.platform", "win32")
+ service = services.AlertsService(enable=True, app_name=app_name)
notify = mocker.patch(target="plyer.notification.notify")
# when
- service.notify(title="t" * 64, message="m" * 376)
+ service.notify(title=title, message=message)
# then
notify.assert_called_once_with(
- title=f"{'t' * 62}\N{HORIZONTAL ELLIPSIS}",
- message=f"{'m' * 254}\N{HORIZONTAL ELLIPSIS}",
- app_name=f"{'a' * 126}\N{HORIZONTAL ELLIPSIS}",
+ title=expected_title,
+ message=expected_message,
+ app_name=expected_app_name,
timeout=None,
)
+ for field, limit in [(expected_title, 63), (expected_message, 255), (expected_app_name, 127)]:
+ assert len(field.encode("utf-16-le")) // 2 <= limit
+
+
+@pytest.mark.parametrize("platform", ["linux", "darwin"])
+def test_alerts_service__long_fields_on_other_platforms(platform: str, mocker: pm.MockerFixture) -> None:
+ mocker.patch("bikes.io.services.sys.platform", platform)
+ service = services.AlertsService(app_name="a" * 128)
+ notify = mocker.patch("plyer.notification.notify")
+ service.notify(title="t" * 64, message="m" * 376)
+ notify.assert_called_once_with(title="t" * 64, message="m" * 376, app_name="a" * 128, timeout=None)
+
+
+@pytest.mark.parametrize("enable", [True, False])
+def test_alerts_service__windows_fallback_keeps_long_fields(
+ enable: bool, mocker: pm.MockerFixture, capsys: pc.CaptureFixture[str]
+) -> None:
+ mocker.patch("bikes.io.services.sys.platform", "win32")
+ notify = mocker.patch("plyer.notification.notify", side_effect=NotImplementedError)
+ service = services.AlertsService(enable=enable, app_name="a" * 128)
+ service.notify(title="t" * 64, message="m" * 376)
+ assert f"[{'a' * 128}] {'t' * 64}: {'m' * 376}\n" in capsys.readouterr().out
+ assert notify.call_count == int(enable)
def test_mlflow_service(mlflow_service: services.MlflowService) -> None:| message=message, | ||
| app_name=self.app_name, | ||
| title=self._truncate(title, self._MAX_TITLE_LENGTH), | ||
| message=self._truncate(message, self._MAX_MESSAGE_LENGTH), |
There was a problem hiding this comment.
[P2] Restrict these limits to Windows. An enabled Linux/macOS notification longer than 255 characters now loses its content too, even though those backends do not use the Win32 buffers. Add platform-specific regression coverage and retain the original text for console fallback.
| @staticmethod | ||
| def _truncate(value: str, max_length: int) -> str: | ||
| """Truncate a notification field to a platform-safe length.""" | ||
| if len(value) <= max_length: |
There was a problem hiding this comment.
[P2] Measure UTF-16 code units rather than Python characters. For example, _truncate("😀" * 32, 63) returns all 32 emoji, which occupy 64 UTF-16 units and exceed the Windows title budget. Truncate the UTF-16 representation without leaving a partial surrogate pair, and test both emoji overflow and a cut through a surrogate pair.
| path: data/targets_train.parquet | ||
| model: | ||
| KIND: BaselineSklearnModel | ||
| max_depth: 7 |
There was a problem hiding this comment.
[P2] Remove this training-model override from these fixes. BaselineSklearnModel already defaults to max_depth=20; setting 7 changes training behavior and potentially prediction quality without evaluation evidence. Neither notification limits nor the MLflow annotation fix requires it.
fix(services): Windows notification failures and removes MLflow schema validation warnings for Pandera-typed inputs.
Changes
Reasons
ValueErrorwhen notification messages exceeded Win32 field limits.Testing
Impacts
Notes