Repository navigation
feat: accept string-typed boolean task parameters #1097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,51 @@ | |
| from .validation import Field, validate_object | ||
|
|
||
|
|
||
| # The case-insensitive boolean vocabulary defined by the Open Job Description | ||
| # specification. Wire values are lowercased and membership-tested against these | ||
| # sets; keeping them as explicit sets (rather than a regex) makes the accepted | ||
| # tokens greppable and self-documenting. | ||
| _TRUE_STRINGS = frozenset({"true", "yes", "on", "1", "1.0"}) | ||
| _FALSE_STRINGS = frozenset({"false", "no", "off", "0", "0.0"}) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This hand-enumerated vocabulary is now a hard gate: any spelling the service emits that is not in these two sets fails the action ( The set mixes two different rules. Two things worth doing:
Also worth noting: |
||
|
|
||
|
|
||
| def _bool_from_api_response(value: str | bool) -> bool: | ||
| """Coerces a wire-format boolean parameter value into a native Python bool. | ||
|
|
||
| The service is migrating boolean parameters from native JSON booleans to | ||
| string-typed booleans, but jobs created before the flip still carry native | ||
| booleans in persisted parameters that are returned verbatim, so both forms | ||
| may arrive during and after rollout. Coercing at the single wire-decode | ||
| choke point produces the native Python type that Open Job Description's | ||
| model documents expression-extension parameters to carry, and normalizes | ||
| both wire forms to one representation. It also validates the value, so an | ||
| out-of-vocabulary string fails fast here rather than reaching a consumer | ||
| that would render it verbatim. | ||
|
|
||
| A native bool is accepted and passed through unchanged. String values are | ||
| matched case-insensitively against the Open Job Description specification's | ||
| boolean vocabulary: "true", "yes", "on", "1", and "1.0" are True; "false", | ||
| "no", "off", "0", and "0.0" are False. Any other value -- including "maybe", | ||
| "", a string with surrounding whitespace, or a non-str, non-bool type such | ||
| as a native int 0 or 1 -- raises ValueError. | ||
| """ | ||
| # bool is a subclass of int, so match a native bool first and pass it | ||
| # through unchanged; the string membership tests below reject ints such as | ||
| # 0/1 because the vocabulary sets contain only str tokens. | ||
| if isinstance(value, bool): | ||
| return value | ||
| if isinstance(value, str): | ||
| lowered = value.lower() | ||
| if lowered in _TRUE_STRINGS: | ||
| return True | ||
| if lowered in _FALSE_STRINGS: | ||
| return False | ||
| raise ValueError( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new
So a bare Before this change, Suggest moving the |
||
| f"Expected a boolean parameter value of True, False, or one of the " | ||
| f"case-insensitive strings {sorted(_TRUE_STRINGS | _FALSE_STRINGS)} but got {value!r}" | ||
| ) | ||
|
|
||
|
|
||
| def parameters_from_api_response( | ||
| params: dict[ | ||
| str, | ||
|
|
@@ -83,7 +128,9 @@ def parameters_from_api_response( | |
| param_value = ParameterValue(type=ParameterValueType.CHUNK_INT, value=value["chunkInt"]) | ||
| elif "bool" in value: | ||
| value = cast(BoolParameter, value) | ||
| param_value = ParameterValue(type=ParameterValueType.BOOL, value=value["bool"]) | ||
| param_value = ParameterValue( | ||
| type=ParameterValueType.BOOL, value=_bool_from_api_response(value["bool"]) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High-level question on the approach: after this change Look at what the surrounding branches do with the other scalar types whose wire form is a string:
So the consumer downstream of This matters because the coercion is not free. It is what introduces the vocabulary gate, and therefore the new "unrecognized spelling fails the action" behaviour that did not exist before. If the string form is in fact accepted downstream, the lower-risk change is: widen the type annotations (which this PR already does), leave the value alone, and widen the validator to accept both forms — no coercion, no new failure mode, no worker-side copy of the spec vocabulary to keep in sync. Two concrete things that would settle it:
If the native bool genuinely is required, this all stands as-is and the only open item is the vocabulary itself (separate comment). If it is not, dropping the coercion removes the entire class of rollout risk. |
||
| ) | ||
| elif "rangeExpr" in value: | ||
| value = cast(RangeExprParameter, value) | ||
| param_value = ParameterValue( | ||
|
|
@@ -107,7 +154,18 @@ def parameters_from_api_response( | |
| ) | ||
| elif "boolList" in value: | ||
| value = cast(BoolListParameter, value) | ||
| param_value = ParameterValue(type=ParameterValueType.LIST_BOOL, value=value["boolList"]) | ||
| bool_list = value["boolList"] | ||
| # Shape-check before iterating so a non-list (e.g. None) raises the | ||
| # same ValueError vocabulary the rest of this function uses rather | ||
| # than a TypeError that callers catching only ValueError would miss. | ||
| if not isinstance(bool_list, list): | ||
| raise ValueError( | ||
| f"Expected a boolList parameter value to be a list but got {bool_list!r}" | ||
| ) | ||
| param_value = ParameterValue( | ||
| type=ParameterValueType.LIST_BOOL, | ||
| value=[_bool_from_api_response(item) for item in bool_list], | ||
| ) | ||
| elif "intListList" in value: | ||
| value = cast(IntListListParameter, value) | ||
| param_value = ParameterValue( | ||
|
|
@@ -499,19 +557,28 @@ def _validate_job_parameters(cls, job_parameters: dict[str, Any]) -> None: | |
| def _is_str_list(value: Any) -> bool: | ||
| return isinstance(value, list) and all(isinstance(item, str) for item in value) | ||
|
|
||
| def _is_bool(value: Any) -> bool: | ||
| # Transitional: accept native JSON booleans (persisted before the | ||
| # flip) and the string form defined by the Open Job Description | ||
| # specification's case-insensitive boolean vocabulary. Matches what | ||
| # _bool_from_api_response coerces at wire-decode time. | ||
| if isinstance(value, bool): | ||
| return True | ||
| return isinstance(value, str) and value.lower() in _TRUE_STRINGS | _FALSE_STRINGS | ||
|
|
||
| value_shape_checks: dict[str, Callable[[Any], bool]] = { | ||
| "string": lambda v: isinstance(v, str), | ||
| "path": lambda v: isinstance(v, str), | ||
| "int": lambda v: isinstance(v, str), | ||
| "float": lambda v: isinstance(v, str), | ||
| "chunkInt": lambda v: isinstance(v, str), | ||
| "bool": lambda v: isinstance(v, bool), | ||
| "bool": _is_bool, | ||
| "rangeExpr": lambda v: isinstance(v, str), | ||
| "stringList": _is_str_list, | ||
| "pathList": _is_str_list, | ||
| "intList": _is_str_list, | ||
| "floatList": _is_str_list, | ||
| "boolList": lambda v: isinstance(v, list) and all(isinstance(item, bool) for item in v), | ||
| "boolList": lambda v: isinstance(v, list) and all(_is_bool(item) for item in v), | ||
| "intListList": lambda v: isinstance(v, list) and all(_is_str_list(item) for item in v), | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
trycloses theValueErrorhole but leaves the adjacentTypeErrorhole open, so the session-teardown failure mode this block exists to prevent is still reachable.parameters_from_api_responsedispatches on"string" in value,"bool" in value, etc. (job_details.py:111-171) without first checking thatvalueis a dict. Task parameters are never run through_validate_job_parameters—task_parameters_datacomes straight from the unvalidatedAssignedSessionpayload at line 534 — so if the service ever sends a parameter whose value is not a container (e.g.{"p": null}or{"p": 5}), the membership test raisesTypeError: argument of type NoneType is not iterable, notValueError. That escapes thisexcept (ValueError, RuntimeError), propagates out ofdequeue(), and sinceSession._start_action(sessions/session.py:653-658) only catchesSessionActionError, it tears down the whole session and cancels every remaining queued action.That is precisely the concern the PR itself raises one file over: the
boolListshape-check comment at job_details.py:155-157 says it exists so callers "catching only ValueError" do not miss aTypeError. But only that oneboolListinstance was patched, while the dispatch chain above it has the same exposure for every parameter type.Two cheap ways to close the class rather than the single instance:
TypeErrorhere:except (ValueError, RuntimeError, TypeError) as e:, orparameters_from_api_responseloop, e.g.if not isinstance(value, dict): raise ValueError(f"Parameter {name} -- expected a dict but got {value!r}"), which also yields a message naming the offending parameter.The second seems preferable since it fixes both call sites and keeps
ValueErroras the single decode-failure contract that the docstring and the newboolListcheck already assume.