[MAINTENANCE] Declare metric values, render-content payloads, and metric keys as what they actually are - #12116
Open
joshua-stauffer wants to merge 5 commits into
Open
[MAINTENANCE] Declare metric values, render-content payloads, and metric keys as what they actually are#12116joshua-stauffer wants to merge 5 commits into
joshua-stauffer wants to merge 5 commits into
Conversation
MetricValue was a Union whose first member was Any, the carrier for deferred SQL/Spark expression objects. A union containing Any accepts every value on assignment, so it added no checking power there. What it did add was cost: each read of a MetricValue-typed attribute was checked against every other member in turn, so one read produced one diagnostic per member whenever the static type could not be proven to satisfy them all -- multiplying a single signal into many duplicate, non-actionable ones. Spelling the alias as Any keeps every existing annotation and assignment valid in both directions while collapsing that multiplication at its source, instead of leaving each caller to suppress it individually. The former member inventory is preserved as documentation at the definition site, along with the runtime consequence: the name, import path, and re-export are unchanged, but the object bound to the name is no longer a Union, so introspecting its members now returns none. No code in this repository does that, or uses the alias as a validated model field. One suppression in the fluent integration test utilities silenced a diagnostic this union produced; with the union gone the comment is dead, and the type-check configuration rejects unused ignores, so it goes too.
These tests build metric mappings by hand and key them with bare three-element tuples, while the interfaces they hand them to declare MetricConfigurationID. A mapping declared to use a named key type should be keyed with it: the reader learns what the key means, and the checker can tell a metric identifier from any other string triple. The substitution is behavior-preserving. MetricConfigurationID is a NamedTuple, so it is a tuple: it compares equal to, and hashes equal to, the bare tuple it replaces, and a lookup written either way finds the same entry. Only key expressions and the key slots of existing annotations change here -- no expected value, no assertion, and no test logic. Where a configuration object was already at hand, its own id property is used rather than respelling the tuple.
These call sites built a literal dict and splatted it into a render content constructor. A splatted mapping cannot be checked argument by argument: the checker compares the dict's joined value type against every candidate parameter, so one call produces a diagnostic per parameter the joined type does not satisfy -- many duplicates standing in for at most a few real mismatches, and no indication which argument is actually wrong. Written as direct keyword arguments, each value is checked against its own parameter. The rewrite is mechanical and behavior-preserving: same arguments in the same order, same value expressions, same evaluation order, the same TypeError for an unknown keyword, and the same object built. The render suites pass unchanged. Where normalization resolved a mismatch, the suppression that had covered the whole dict is now unused and is dropped; where a genuine per-argument mismatch remains, that same suppression moves to the one argument it applies to, so it no longer hides its neighbours.
With the splat call sites written as keyword arguments, the checker can finally evaluate each argument against its own parameter, and a handful of genuine mismatches surfaced underneath the noise. Each one is a case of the declaration being narrower than what callers legitimately pass, so each is fixed at the declaration. Every change here widens: a table's rows and a table's header row carry several shapes across the renderers that build them, a header may be plain text as well as rendered content, and so may a collapse toggle link. Nothing is narrowed, no parameter is added, removed or renamed, and no default changes -- these signatures are a public override point, and code that type-checked against them before still does. Three suppressions in the expectation renderers covered mismatches these declarations now accept; with the diagnostics gone the comments are dead and the type-check configuration rejects unused ignores, so they go too. Two renderers annotate a local they build incrementally, which the checker could not otherwise infer. The docstrings are corrected where they described the old, narrower promise.
The dispatch invokes prescriptive and diagnostic renderers with different argument sets on different branches: one path passes a configuration and omits the result, another passes a validation result and omits the configuration, and a third passes a configuration taken from a validation result that may not carry one. That is why both parameters are declared optional -- the signature is the dispatch contract, and a renderer that assumes either is present is reading a guarantee the dispatch does not make. This states that contract once, on the base declarations renderers override, so an author writing a new one can see it. The signatures themselves are unchanged: they are a public override point, and this commit adds no parameter, removes none, and changes no default or declared type. Existing renderers that read these parameters unconditionally are left as they are. Guarding them here would change what happens on the paths where the argument really is absent, which is a behavior change rather than a typing one, so those are reported separately for their own fix.
✅ Deploy Preview for niobium-lead-7998 canceled.
|
joshua-stauffer
marked this pull request as ready for review
August 28, 2026 00:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tightens four narrow but widely-repeated type declarations to what they actually admit, without changing any runtime behavior:
MetricValue(a resolved metric value, invalidator/computed_metric.py) is now spelled as the dynamic type it always effectively was, instead of a closedUnionthat every call site had to re-narrow before doing anything with it.render/components.pynow declare the parameter shapes their callers legitimately pass (widened, never narrowed).Why
A union wide enough to accept every assignment and permissive enough to satisfy every read checks nothing — it just multiplies the number of places a real defect could hide. Once metric values and render-content payloads are described accurately, the type checker can tell the difference between a legitimate call and a genuine mistake, instead of forcing every caller to defeat the checker with a local suppression.
None of these are behavior changes: every widened parameter accepts a strict superset of what it accepted before, every changed key construction produces the same runtime value as the bare tuple it replaces (the named identifier type is a
NamedTuple, so it compares and hashes equal), no renderer signature changed, and no renderer body changed behaviorally (many of the call-site rewrites above sit inside renderer methods, but each is the same call written differently).Known residue, deliberately not fixed here
Writing the dispatch contract down made it clear that a number of renderers read
configurationorresultunconditionally, even though the dispatcher has reachable branches that omit either one. Those methods raise when they hit such a branch. These are pre-existing defects, not introduced here, and they are left alone deliberately: adding a guard would change what happens on those paths, which is a behavior change and does not belong in a typing PR. They are tracked for separate follow-up, along with one render view whose signature is narrower than its own body and docstring allow.Because those defects are unfixed, this PR does not drive the renderer-nullability diagnostics to zero. That residue is expected and accounted for.
User impact
None beyond fewer false type-checker diagnostics for anyone type-checking code that calls into these APIs. No public name moved, no parameter was removed or renamed, no already-declared parameter type was narrowed, and no renderer's declared signature changed in any way — parameter names, order, and defaults are byte-identical before and after, verified by an automated before/after signature comparison across every render-content class the render package exports and every renderer method this change touches.
How to review
great_expectations/validator/computed_metric.py— theMetricValueredefinition and its compatibility rationale in the comment above it, including what changes at runtime (introspecting the alias's members now returns none) and why nothing in this repository does that.great_expectations/render/components.py— the four widened constructor parameters (table,header,header_row,collapse_toggle_link); confirm each widening is a strict superset of the prior type.great_expectations/expectations/expectation.py— the two added docstrings on the base renderer declarations. These are documentation only; the commit adds 24 lines and deletes none.# type: ignorecomments became unused once the declarations were corrected, and the type-check configuration rejects unused ignores.