diff --git a/packages/core/src/mostlyright/__init__.py b/packages/core/src/mostlyright/__init__.py index e84cb41b..66688556 100644 --- a/packages/core/src/mostlyright/__init__.py +++ b/packages/core/src/mostlyright/__init__.py @@ -34,6 +34,8 @@ # Split-distribution namespace: extend __path__ to discover sibling packages' contributions. __path__ = __import__("pkgutil").extend_path(__path__, __name__) +import sys as _sys +import types as _types import warnings from importlib.metadata import PackageNotFoundError from importlib.metadata import version as _dist_version @@ -181,16 +183,72 @@ def __getattr__(name: str): if name in _LEGACY_SHIM_HOMES: _warn_legacy_shim(name) obj = _resolve_legacy_shim(name) - # Resolving a shim imports ``mostlyright.research`` / ``mostlyright.live``, - # whose import machinery auto-binds them as package attributes here — which - # would shadow the ``research``/``live`` shims on the next access (skipping - # the deprecation warning + returning a MODULE for ``research``). Scrub them - # so ``__getattr__`` stays authoritative (once-per-session is governed by - # ``_LEGACY_SHIM_WARNED``, not attribute caching). Internal code reaches the - # submodules via ``sys.modules`` / ``from mostlyright.research import ...``, - # never these package attributes. - globals().pop("research", None) + # Resolving a shim imports ``mostlyright.live``, whose import machinery + # auto-binds it as a package attribute here — which would skip the + # deprecation warning on the next access. Scrub it so ``__getattr__`` + # stays authoritative (once-per-session is governed by + # ``_LEGACY_SHIM_WARNED``, not attribute caching). ``research`` is + # deliberately NOT scrubbed here anymore: its data-descriptor property + # (below) already neutralizes the auto-bound submodule, so a pop here + # could only ever delete an INTENTIONAL user override — silently + # un-patching ``mock.patch("mostlyright.research")`` the moment any + # OTHER legacy symbol was accessed (Python-Architect review-iter-2). globals().pop("live", None) return obj raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + +# --------------------------------------------------------------------------- +# Import-system-proof shim for the ``research`` name collision (post-1.17.0). +# +# The ``__getattr__`` scrub above only runs when a SHIM ACCESS performed the +# first import of the submodule. When something else imports it first — e.g. +# the eager ``obs()`` path's ``from mostlyright.research import +# _resolve_station`` — the import system auto-binds the SUBMODULE as this +# package's ``research`` attribute with no scrub, shadowing the shim: the next +# ``mostlyright.research(...)`` raised ``TypeError: 'module' object is not +# callable`` for the rest of the process. A data descriptor on the package's +# own module class outranks the instance ``__dict__``, so the shim stays +# authoritative no matter who imports first. The setter is a deliberate no-op +# that drops the import system's auto-binding — the submodule stays reachable +# via ``sys.modules`` / ``importlib`` / ``from mostlyright.research import x`` +# / ``mock.patch`` dotted targets (all resolve through the import system, not +# this attribute), which is how every internal and test call site addresses it +# (pinned by test_research_submodule_access_paths_survive_the_property_shim). +# +# KNOWN, ACCEPTED trade-off: ``import mostlyright.research as rm`` binds ``rm`` +# via THIS attribute and therefore yields the legacy callable, not the module. +# That form appears nowhere in the repo, docs, or published examples; the +# package attribute has been contractually reserved for the legacy callable +# since the 34-06 namespace inversion (whose byte-identity tests pin +# ``mostlyright.research is mostlyright.weather.research``, ruling out a +# callable-module design). Both names retire at 2.0. +# --------------------------------------------------------------------------- + + +class _MostlyrightModule(_types.ModuleType): + @property + def research(self): + # An INTENTIONAL write (monkeypatch.setattr / mock.patch in user test + # suites) is honored verbatim. Exactly ONE value is treated as + # non-authoritative and falls through to the shim: the auto-bound + # submodule object itself — the only thing the import system ever + # writes here (Python-Architect review: an unconditional no-op setter + # silently broke root-symbol patching and crashed mock teardown). + override = self.__dict__.get("research") + if override is not None and override is not _sys.modules.get("mostlyright.research"): + return override + _warn_legacy_shim("research") + return _resolve_legacy_shim("research") + + @research.setter + def research(self, value) -> None: + self.__dict__["research"] = value + + @research.deleter + def research(self) -> None: + self.__dict__.pop("research", None) + + +_sys.modules[__name__].__class__ = _MostlyrightModule diff --git a/packages/core/src/mostlyright/align.py b/packages/core/src/mostlyright/align.py index c2180ac7..d058bffe 100644 --- a/packages/core/src/mostlyright/align.py +++ b/packages/core/src/mostlyright/align.py @@ -467,6 +467,7 @@ def align( if equality: result = _equality_join(result, prefixed, spec) else: + _gate_asof_knowledge_dtype(prefixed, spec, kt_col, result) result = _asof_join(result, prefixed, spec, kt_col) # pit_fidelity="reconstructed" sources are knowingly not point-in-time # (highest-authority-available, e.g. the CLI settlement label) → exempt. @@ -489,6 +490,89 @@ def align( return result +def _gate_asof_knowledge_dtype( + prefixed: pd.DataFrame, spec: SourceContract, kt_col: str | None, spine: pd.DataFrame +) -> None: + """Refuse an as-of join whose knowledge column cannot be compared to + ``decision_time`` — BEFORE pandas raises a bare ``MergeError``. + + The trap this guards (README-validation find): a BYO spine WITHOUT the + source's equality-join key (``spec.event_time_col``, e.g. the obs source's + ``date``) silently falls to the as-of path, where the source's ISO-string + knowledge column cannot be compared to the datetime ``decision_time`` — + the user got ``pandas.errors.MergeError`` instead of error copy naming the + source, the column, and the fix. + + The compatibility predicate MIRRORS ``merge_asof`` exactly (empirically + probed on pandas 2.x, pinned by tests): the two key dtypes must be + IDENTICAL (including datetime resolution and timezone — ``int64/float64``, + ``ns/us``, ``UTC/US-Eastern`` and naive/aware pairs all raise in pandas) + and belong to an as-of-orderable family (numeric, datetime, or timedelta — + equal ``object`` dtypes also raise in pandas). The gate therefore only + ever raises where ``merge_asof`` would raise anyway — a previously-working + input (e.g. a BYO spine as-of-joining on int64 keys) never changes + behavior. + """ + if not kt_col or kt_col not in prefixed.columns or _DECISION_COL not in spine.columns: + return + kdt = prefixed[kt_col].dtype + ddt = spine[_DECISION_COL].dtype + orderable = ( + pd.api.types.is_numeric_dtype(kdt) + or pd.api.types.is_datetime64_any_dtype(kdt) + or pd.api.types.is_timedelta64_dtype(kdt) + ) + if kdt == ddt and orderable: + return + + from mostlyright.core.exceptions import ContractError + + if spec.event_time_col and spec.event_time_col not in spine.columns: + problem = ( + f"the spine is missing this source's equality-join key " + f"{spec.event_time_col!r}, so align fell back to an as-of join — but the " + f"source's knowledge column {spec.knowledge_time_col!r} (dtype {kdt}) " + f"cannot be as-of-compared to the spine {_DECISION_COL!r} (dtype {ddt})" + ) + fix = ( + f"add the {spec.event_time_col!r} column (the ISO settlement day) to your " + f"spine — weather.days() and the label factories carry it, and mr.spine() " + f"can map it from your own frame" + ) + elif not orderable: + problem = ( + f"knowledge_time_col {spec.knowledge_time_col!r} has dtype {kdt}, which is " + f"not an as-of-orderable key (numeric, datetime, or timedelta) and cannot " + f"be compared to the spine {_DECISION_COL!r} (dtype {ddt})" + ) + fix = ( + f"convert the source's {spec.knowledge_time_col!r} column to a " + f"timezone-aware datetime (e.g. pd.to_datetime(..., utc=True))" + ) + else: + problem = ( + f"knowledge_time_col {spec.knowledge_time_col!r} (dtype {kdt}) and the " + f"spine {_DECISION_COL!r} (dtype {ddt}) are not the identical dtype — " + f"merge_asof requires exact dtype equality (timezone, resolution, and " + f"numeric width included)" + ) + fix = ( + f"convert the source's {spec.knowledge_time_col!r} column to the spine's " + f"{_DECISION_COL!r} dtype (e.g. " + f'pd.to_datetime(..., utc=True).astype("{ddt}"))' + ) + raise ContractError( + contract_message( + source_id=spec.id, + field=spec.knowledge_time_col, + problem=problem, + fix=fix, + ), + field=spec.knowledge_time_col, + source=spec.id, + ) + + def _free_internal_name(base: str, *frames: pd.DataFrame) -> str: """Return ``base`` (or ``base_N``) guaranteed absent from every frame. diff --git a/packages/core/tests/test_align.py b/packages/core/tests/test_align.py index ef568cbd..4a8687d0 100644 --- a/packages/core/tests/test_align.py +++ b/packages/core/tests/test_align.py @@ -234,3 +234,117 @@ def test_align_missing_declared_column_raises_contract_error() -> None: align(spine, (frame, spec)) assert "temp_f" in str(exc_info.value) + + +def test_asof_naive_knowledge_time_vs_aware_decision_raises_contract_error() -> None: + """Error-copy regression: a tz-NAIVE knowledge column as-of-joined against + the tz-aware ``decision_time`` previously crashed with a raw pandas + ``MergeError``. It must raise a :class:`ContractError` naming the source + and the tz mismatch.""" + from mostlyright.core.exceptions import ContractError + + spec = SourceContract( + id="met", + prefix="met_", + entity_key=("station",), + knowledge_time_col="known_at", + pit_fidelity="exact", + ) + frame = pd.DataFrame( + { + "station": ["A"], + "known_at": pd.to_datetime(["2025-01-01T00:00:00"]), # NAIVE + "temp_f": [1.0], + } + ) + spine = pd.DataFrame( + { + "station": ["A"], + "decision_time": pd.to_datetime(["2025-01-05T00:00:00Z"], utc=True), + } + ) + + with pytest.raises(ContractError) as exc_info: + align(spine, (frame, spec)) + + assert "met" in str(exc_info.value) + + +def test_asof_numeric_keys_still_join_without_gate() -> None: + """Codex review-iter-1 CRITICAL guard: ``merge_asof`` legitimately supports + NUMERIC keys — a BYO spine as-of-joining on identical int64 keys must keep + working (the dtype gate mirrors pandas exactly, it does not require + datetimes).""" + spec = SourceContract( + id="met", + prefix="met_", + entity_key=("station",), + knowledge_time_col="seq", + pit_fidelity="exact", + ) + frame = pd.DataFrame({"station": ["A"], "seq": pd.array([1], dtype="int64"), "temp_f": [1.0]}) + spine = pd.DataFrame({"station": ["A"], "decision_time": pd.array([5], dtype="int64")}) + + out = align(spine, (frame, spec)) + + assert out.loc[0, "met_temp_f"] == 1.0 + + +def test_asof_equal_object_dtypes_raise_contract_error() -> None: + """Equal dtypes are not enough — equal ``object`` keys also raise in + pandas, so the gate must catch them with error copy (not a raw + MergeError).""" + from mostlyright.core.exceptions import ContractError + + spec = SourceContract( + id="met", + prefix="met_", + entity_key=("station",), + knowledge_time_col="day", + pit_fidelity="exact", + ) + frame = pd.DataFrame({"station": ["A"], "day": ["2025-01-01"], "temp_f": [1.0]}) + spine = pd.DataFrame({"station": ["A"], "decision_time": ["2025-01-05"]}) + + with pytest.raises(ContractError) as exc_info: + align(spine, (frame, spec)) + + assert "met" in str(exc_info.value) + + +def test_asof_datetime_resolution_mismatch_raises_contract_error() -> None: + """Codex review-iter-1 HIGH guard: both-aware but MIXED-RESOLUTION datetime + keys (``us`` vs ``ns``) raise in pandas — the gate must catch the pair with + error copy naming both dtypes.""" + from mostlyright.core.exceptions import ContractError + + spec = SourceContract( + id="met", + prefix="met_", + entity_key=("station",), + knowledge_time_col="known_at", + pit_fidelity="exact", + ) + # Units pinned EXPLICITLY on both sides: pandas 2 infers ns and pandas 3 + # infers us from to_datetime, so relying on the default would make the + # mismatch vanish under one major (pandas-3-suite caught exactly that). + frame = pd.DataFrame( + { + "station": ["A"], + "known_at": pd.to_datetime(["2025-01-01T00:00:00Z"], utc=True).as_unit("us"), + "temp_f": [1.0], + } + ) + spine = pd.DataFrame( + { + "station": ["A"], + "decision_time": pd.to_datetime(["2025-01-05T00:00:00Z"], utc=True).as_unit("ns"), + } + ) + + with pytest.raises(ContractError) as exc_info: + align(spine, (frame, spec)) + + msg = str(exc_info.value) + assert "met" in msg + assert "dtype" in msg diff --git a/packages/core/tests/test_align_lazy.py b/packages/core/tests/test_align_lazy.py index 0b0f9b69..52977d49 100644 --- a/packages/core/tests/test_align_lazy.py +++ b/packages/core/tests/test_align_lazy.py @@ -287,3 +287,36 @@ def test_lazy_obs_features_actually_attach_to_days_spine(monkeypatch) -> None: assert "obs_high_f" in out.columns assert out["obs_high_f"].notna().all() + + +def test_byo_spine_missing_source_event_key_raises_contract_error(monkeypatch) -> None: + """Error-copy regression (README validation find): a BYO spine WITHOUT the + obs source's equality-join key (``date``) previously crashed inside + pandas ``merge_asof`` with a raw ``MergeError`` (string ``date`` vs + datetime ``decision_time``). It must raise a style-guide + :class:`ContractError` naming the source, the column, and a concrete fix.""" + import importlib + + obs_mod = importlib.import_module("mostlyright.weather.obs") + monkeypatch.setattr( + obs_mod, + "_dispatch_strategy", + lambda *a, **k: [{"observed_at": "2025-01-06T18:00:00Z", "temp_f": 40.0}], + ) + import mostlyright as mr + + my_label_frame = pd.DataFrame( + { + "station": ["NYC"], + "cutoff": _ts(["2025-01-07T21:30:00Z"]), + "settle_high": [41.0], + } + ) + spine = mr.spine(my_label_frame, entity="station", decision_time="cutoff", y="settle_high") + + with pytest.raises(ContractError) as exc_info: + mr.align(spine, obs_mod.obs("NYC")) + + msg = str(exc_info.value) + assert "date" in msg + assert "spine" in msg.lower() diff --git a/packages/core/tests/test_shims.py b/packages/core/tests/test_shims.py index 6a5cc778..ccb8f1e0 100644 --- a/packages/core/tests/test_shims.py +++ b/packages/core/tests/test_shims.py @@ -17,6 +17,7 @@ from __future__ import annotations +import contextlib import warnings import pytest @@ -184,3 +185,108 @@ def test_d22_bridge_warning_constant_removed() -> None: research_mod = importlib.import_module("mostlyright.research") assert not hasattr(research_mod, "_D22_BRIDGE_WARNING") + + +def test_research_shim_survives_import_system_parent_binding() -> None: + """Post-1.17.0 regression (README-validation find): the import system binds + the ``mostlyright.research`` SUBMODULE onto the package when anything does + ``from mostlyright.research import ...`` FIRST (the eager ``obs()`` path + does exactly that internally) — which previously shadowed the ``research`` + shim and made ``mostlyright.research(...)`` raise + ``TypeError: 'module' object is not callable`` for the rest of the process. + The shim must stay authoritative over the auto-bound submodule attribute.""" + import importlib + import sys + + import mostlyright + + research_mod = importlib.import_module("mostlyright.research") + # Simulate the import system's parent binding (what a genuine FIRST + # ``from mostlyright.research import _x`` does in a fresh process). The + # fix may refuse the binding outright — equally acceptable. + with contextlib.suppress(AttributeError): + mostlyright.research = research_mod + assert callable(mostlyright.research), ( + "mostlyright.research must remain the callable legacy shim even after " + "the research SUBMODULE is auto-bound onto the package by the import " + "system" + ) + assert mostlyright.research is sys.modules["mostlyright.research"].research + + +def test_research_shim_callable_in_fresh_process_after_eager_obs() -> None: + """End-to-end fresh-process repro: eager ``weather.obs()`` (whose internals + import ``mostlyright.research`` first) must not break the legacy + ``mostlyright.research(...)`` entry point.""" + import subprocess + import sys + + code = ( + "import warnings; warnings.simplefilter('ignore')\n" + "from unittest.mock import patch\n" + "import importlib\n" + "import mostlyright\n" + "obs_mod = importlib.import_module('mostlyright.weather.obs')\n" + "with patch.object(obs_mod, '_dispatch_strategy',\n" + " lambda *a, **k: [{'observed_at': '2025-01-06T18:00:00Z'," + " 'temp_f': 40.0}]):\n" + " obs_mod.obs('KNYC', '2025-01-06', '2025-01-07')\n" + "assert callable(mostlyright.research), type(mostlyright.research)\n" + "print('SHIM-OK')\n" + ) + proc = subprocess.run([sys.executable, "-c", code], capture_output=True, text=True, timeout=120) + assert proc.returncode == 0, proc.stderr + assert "SHIM-OK" in proc.stdout + + +def test_research_submodule_access_paths_survive_the_property_shim() -> None: + """The package-attribute ``mostlyright.research`` is RESERVED for the + legacy callable (34-06 contract; the module-class property makes that + order-independent). Every supported way of addressing the SUBMODULE keeps + working: sys.modules, importlib, from-import, and mock.patch dotted + targets (which resolve via importlib, not the parent attribute).""" + import importlib + import sys + from unittest.mock import patch + + mod = importlib.import_module("mostlyright.research") + assert sys.modules["mostlyright.research"] is mod + from mostlyright.research import _resolve_station + + assert callable(_resolve_station) + with patch("mostlyright.research._resolve_station", lambda *a, **k: None): + assert mod._resolve_station("KNYC") is None + + +def test_root_research_symbol_remains_patchable() -> None: + """Python-Architect review (iter-1 HIGH): user test suites patch the root + ``mostlyright.research`` symbol — ``monkeypatch.setattr`` and every + ``mock.patch`` form must take effect AND restore cleanly (the first fix's + unconditional no-op setter silently dropped the stub and crashed mock's + ``delattr`` teardown). Only the import system's auto-bound SUBMODULE value + is treated as non-authoritative.""" + from unittest.mock import MagicMock, patch + + import mostlyright + + original = mostlyright.research + assert callable(original) + + sentinel = MagicMock(name="research_stub") + mostlyright.research = sentinel + try: + assert mostlyright.research is sentinel # intentional write honored + finally: + del mostlyright.research + assert callable(mostlyright.research) + assert mostlyright.research is original + + with patch("mostlyright.research") as mocked: + assert mostlyright.research is mocked + # Review-iter-2 (Python Architect): touching ANY other legacy symbol + # used to run the __getattr__ scrub and silently DELETE the active + # patch — the stub must survive unrelated legacy-shim traffic. + _ = mostlyright.CATALOG + _ = mostlyright.dataset + assert mostlyright.research is mocked + assert mostlyright.research is original # teardown restored, no AttributeError