fix(parameters): repair the type-mismatch message for list-typed deploy-time fields - #3370
fix(parameters): repair the type-mismatch message for list-typed deploy-time fields#3370kevin9327 wants to merge 1 commit into
Conversation
…oy-time fields
_check_type looked up TYPES[self.parameter_type] on the branch where
parameter_type is a *list* of acceptable types. A list is unhashable, so
building the message raised
TypeError: unhashable type: 'list'
before ParameterFieldTypeMismatch was ever constructed. The user never saw
the message the code was trying to give them.
Reachable from three call sites in events_decorator.py, all of which pass a
list: @trigger(event=<fn>) and @trigger_on_finish(flow=<fn>) use [str, dict],
and event={"parameters": <fn>} uses [list, dict, tuple].
TYPES also has no entry for str, so even a hashable lookup would have raised
KeyError; join the per-type names with a __name__ fallback instead. The
sibling branch three lines below guards its lookup with
`elif self.parameter_type in TYPES`, which is why only this one was affected.
Greptile SummaryThis PR repairs error-message construction for list-typed deploy-time fields without changing type-acceptance behavior.
Confidence Score: 5/5The PR appears safe to merge; no actionable correctness, security, or repository-rule issues were identified. The formatter handles every type used by the affected event-decorator call sites, preserves successful value handling, and now raises the intended domain exception on mismatches.
|
| Filename | Overview |
|---|---|
| metaflow/parameters.py | Safely renders each allowed type in list-typed deploy-time mismatch messages while preserving existing validation behavior. |
| test/unit/test_deploy_time_field.py | Adds focused regression and compatibility coverage for list-typed and single-type DeployTimeField validation. |
Reviews (1): Last reviewed commit: "fix(parameters): repair the type-mismatc..." | Re-trigger Greptile
Shriprasad-P
left a comment
There was a problem hiding this comment.
Reviewed the DeployTimeField type-mismatch fix and the associated regression tests. I reproduced the current TypeError: unhashable type: 'list' behavior on master and verified that this change restores the intended ParameterFieldTypeMismatch for both list-typed trigger cases without changing accepted-value behavior.
The focused tests and pre-commit checks pass locally, and I did not find any substantive code issues. I’m leaving this as a comment rather than an approval for now since the normal CI checks have not run yet and the repository’s issue/process requirements for this area still need to be satisfied.
Shriprasad-P
left a comment
There was a problem hiding this comment.
Review
Nice fix for list-typed DeployTimeField mismatch messages. Using TYPES[self.parameter_type] when parameter_type is a list would KeyError (or produce a useless message) and hide the real type mismatch from @trigger users.
Joining human-readable type names with a __name__ fallback is the right UX, and the new tests cover list-of-types and single-type paths.
Approve.
Shriprasad-P
left a comment
There was a problem hiding this comment.
Review
Nice fix for list-typed DeployTimeField mismatch messages. Using TYPES[self.parameter_type] when parameter_type is a list would KeyError (or produce a useless message) and hide the real type mismatch from @trigger users.
Joining human-readable type names with a __name__ fallback is the right UX, and the new tests cover list-of-types and single-type paths.
Approve.
PR Type
Summary
If a
@trigger(event=<function>)returns the wrong type, the user getsTypeError: unhashable type: 'list'from inside Metaflow instead of theParameterFieldTypeMismatchtelling them what type was expected. Thetype-mismatch message could not be built, so it never got raised.
Issue
No existing issue -- happy to file one first if you prefer that order.
Reproduction
Runtime: local (the failure is in message construction, before any
orchestrator call)
Commands to run:
pip install -e . python repro.pyrepro.py-- constructs theDeployTimeFieldexactly asevents_decorator.py:127does for a callableevent=:Where evidence shows up: parent console, during
argo-workflows create/step-functions create(or anydeploy_time_evalof the decorator's attributes).
Before (on master) -- all three list-typed call sites
After
Root Cause
metaflow/parameters.py:184, inDeployTimeField._check_type:The guard establishes that
self.parameter_typeis a list, and then usesthat list as a dict key. Lists are unhashable, so the
%operand raisesTypeErrorwhile the mismatch is being reported --ParameterFieldTypeMismatchis never constructed.
_check_typeis called from__call__'selseclause,outside its
try, so theTypeErrorpropagates raw to the user.Two things are wrong with the lookup, not one: even if
parameter_typewerehashable,
TYPEShas nostrentry, so[str, dict]could not be renderedfrom
TYPESanyway.Reachable from every list-typed
DeployTimeFieldin the tree, all inmetaflow/plugins/events_decorator.py:event[str, dict]@trigger(event=<fn>)parameters[list, dict, tuple]@trigger(event={"parameters": <fn>})fq_name[str, dict]@trigger_on_finish(flow=<fn>)Why This Fix Is Correct
Render one name per entry instead of looking the whole list up:
TYPES.getkeeps the existing spellings for the five types the table covers,and
__name__supplies"str"(and anything else a future call site lists)rather than raising
KeyError. Only the message text changes -- theisinstancecheck that decides whether to raise is untouched, so no value thatused to be accepted is now rejected or vice versa.
The sibling branch three lines below already does this safely:
which is why only the list branch was broken.
Failure Modes Considered
if not any(isinstance(...))body, i.e. on the path that was already committed toraising.
test_list_of_types_accepts_any_listed_typepins thatstr/dictvalues still pass for
[str, dict]andlist/tuple/dictfor[list, dict, tuple].getattr(t, "__name__", str(t))falls back tostr(t)rather than raising, so a typing construct orany object without
__name__degrades to a readable repr instead of turningthe error report into a second exception -- which is the bug being fixed.
TypeErrorarounddeploy_time_evalor_check_type(I grepped); thenearest
except TypeErroris in__call__, and it wraps only the userfunction invocation, not
_check_type. Callers that already handleParameterFieldTypeMismatchnow receive it as intended.Tests
New file
test/unit/test_deploy_time_field.py(pytest, module-level functions,monkeypatchfor the module-level parameter context, parametrized over the tworeal list shapes), covering:
test_list_of_types_reports_the_mismatch-- the bug, for[str, dict]and[list, dict, tuple]test_list_of_types_accepts_any_listed_type(pin)test_single_type_mismatch_still_reports/test_single_type_match_still_passes(pin for the guarded sibling branch)
Whole of
test/unit, same invocation, before and after:The delta is exactly the eight new tests.
What I could not run: I develop on Windows, which Metaflow does not support
(
setup.pyclassifiers list macOS and Linux only) -- several core modulesimport fcntlat module scope, so every test that spawns a real run fails hereregardless of this patch. That is the 21 failures / 103 errors above; the counts
are identical before and after. I also did not deploy a flow to Argo or Step
Functions to watch this message appear there, since I have no cluster -- the
reproduction drives
DeployTimeFielddirectly, which is the codedeploy_time_evalcalls. I am relying on CI for the Linux/macOS matrix.black25.12.0 with the target list from.pre-commit-config.yamlreports bothfiles unchanged.
Non-Goals
I did not add
strto theTYPEStable (that would change the wording of theother branch's messages too), did not touch the
isinstancevstype(val) !=asymmetry between the two branches, and did not change any of the
events_decorator.pycall sites.AI Tool Usage
Claude Code, used to enumerate the
DeployTimeFieldcall sites and to draftthis description. The reproduction and the before/after runs are commands I
actually ran; I understand why a list cannot be a dict key here and why the
guarded sibling branch was unaffected.