Parameter handling while signing views

[marcverhagen]

When signing views some unintuitive things can happen to the parameters stored in the metadata of the view.

First, if a parameter was not defined for the app you get a warning but the parameter is still stored in the metadata (these both make sense). But if the parameter is multi-valued then only the first value will be stored in the metadata.

This is easy to change by fiddling with the two instances of this code in the sign_view() method:

if k in params_map and params_map[k].multivalued:
    view.metadata.add_parameter(k, str(v))
else:
    view.metadata.add_parameter(k, v[0])

Second, if parameters were handed in via an envelope then it is maybe better to not make the values always a string. That made sense for parameters handed in via command line options and GET variables, less so for values that initially came from a JSON file that used dictionaries and lists as it pleased.

[keighrim]

tl;dr, the first is a real bug, the second I think is mostly cosmetic once we make the envelope contract explicit. Below is the writeup where I landed, since this touches a design decision from #287 that's currently implicit and should live somewhere. My proposal for further fixes is at the bottom.

setup

sign_view writes params into two places, and they're not redundant;

• view.metadata.parameters: the "raw" params the user actually passed (transparency; "what was typed"), before defaults.
• view.metadata.appConfiguration: the fully refined config; every declared param with its default filled and value cast (reproducibility; "what actually ran").

clarification: envelop format

An envelope's parameters block uses native json (e.g., {"labelMap": {"B":"bars"}, "temperature": 0.7, "pretty": true}). The "raw" form, on the other hand, is Dict[str, List[str]] (a list of strings per key), and it's list-of-str because that's just the natural shape of query-string and CLI input; everything on the wire is a string, and a repeated key (e.g., ?labels=a&labels=b) collects into a list, so request.args.to_dict(flat=False) (and argparse the same way) hands us List[str] values.

The two steps that reconcile these run at different points, and that's the part worth being explicit about;

• normalize_params runs early, and only in the envelope path, right at the boundary in unwrap_if_envelope() (top of annotate(), before anything else). It only flattens native json into the same Dict[str, List[str]] the other channels produce; basically it's conforming the envelope to the same shape as query-string and CLI input.
• _refine_params runs later, once, downstream of all channels (query string, CLI, envelope alike), and does the actual work (cast + default-fill + validate).

Again, the envelope is just another input surface along with cli and query-string; it does not get its own refinement path, and it must go through _refine_params against whatever app is receiving it (so e.g. a new optional param added in a newer app version still gets its default filled).

limitation: We can't take both json-valued and "raw" as valid envelope input

The two readings collide on real values; e.g., ["B:bars"] is a multivalued param with one value under json-reading, but a map under raw-reading, and ["0.7"] is a one-element string list vs. a scalar 0.7. To disambiguate we'd have to consult the param spec inside normalization, which re-couples the exact thing normalization exists to de-couple (the caster used in _refine should be the single authority), and is impossible for undefined params anyway. So it's one or the other, and json-valued is the one that matters; it's what lets you hand-write long prompts and map params, and it's what create_envelope is built for (passing native python dicts straight in, e.g. clams-kitchen's param_sets).

stringification is only cosmetic issue

Because we've committed to json-valued envelopes, the section that round-trips back into an envelope is view.metadata.appConfiguration, not view.metadata.parameters, and it already does today, since v.m.appConfiguration stores native json. Namely, to replay a run you copy from v.m.appConfiguration, not from v.m.parameters. v.m.parameters is the human-facing "what was typed" record; storing "['a','b']" vs a real ["a","b"] there changes nothing about reproducibility. (we could still store clean json there mostly for readability (though the implementation for wiring pre-normalization values after refinment step would be a bit messy and tricky), but I don't think it should block anything, since nothing downstream depends on it.) And TBH, considering the messy implementation tradeoffs, I think it's a bit of a red herring.

1st point is a real bug (plus more I found)

Multivalued values for undefined params were getting truncated to v[0], which is data loss. And while digging into the envelope format above, I found another one; a non-multivalued param handed an empty array or object crashes sign_view with IndexError at v[0] (nothing to index). For example, {"x": []} (or {"x": {}}) normalizes to [], and then v[0] blows up. In normal circumstances this shouldn't happen at all; only the envelope route can produce an empty list (a query string always gives a length-1 list, even empty string "" --> [""]), so it takes a hand-built envelope. But as long as we allow arbitrary json in the envelope, users can pass this (imo) ill-formed input, so we should add a simple guardrail; treat empty as unset, which is what the caster already does with it.

And the current fix in 296- branch over-corrects the other way; a single-value undefined param now records with the normalize-stage singleton wrapper leaking through. e.g., an envelope {"p1": "v1"} normalizes to "p1": ["v1"], and at recording time, if p1 is declared it gets unwrapped back to "v1", but an undefined one gets str(["v1"]) --> "['v1']", brackets and all. Namely, the same single value reads differently depending only on whether the app declared it. single values should stay clean ("v1") either way; serialize only when there's genuinely more than one; i.e., branch on len(v), not on declared-vs-undefined. (I know it's going to be messy nested ifs...)

proposal

1. fix add_parameters_to_metadata; branch on len(v) (0 --> treat as unset, same as the caster; 1 --> clean value; >1 --> serialized) uniformly for defined and undefined params, instead of indexing v[0]. add regression tests for undefined-multivalued and empty-array/object inputs (both untested at the moment).
2. more clearly document the dichotomy (in the sign_view docstring and the rst) so parameters = raw/transparency, appConfiguration = resolved/reproducible is stated somewhere explicit.
3. state the envelope contract in the rst; the envelope parameters block must be json-native, and if you're building an envelope from an existing MMIF to reproduce a run, copy from appConfiguration, not parameters.

[marcverhagen]

[edited because I forgot a sentence]
Just for clarification. Say you take a view's appConfiguration and put it in the parameters part of an envelope where the MMIF part only contains the views prior to the view. Then when you run the app on the envelope it should create the view as before, right?

If view.metadata.parameters only include those that were typed then that would exclude parameters from an envelope, which would be fine if the above is the case.

Incidentally, in my experiments where I hand parameters in as strings I do not get to see a view.metadata.parameters property. Not sure why.

[keighrim]

one more, tangential to the above; when no runtime params are passed through any channel (query/CLI/envelope), the signed view omits the parameters key entirely (only appConfiguration shows up, filled with defaults). I think we should always keep parameters present as an empty {}, so downstream consumers can rely on the key existing instead of guarding for its absence. should be a small fix in add_parameters_to_metadata, write the key unconditionally even when the raw dict is empty.

[keighrim]

Say you take a view's appConfiguration and put it in the parameters part of an envelope where the MMIF part only contains the views prior to the view. Then when you run the app on the envelope it should create the view as before, right?

yes; drop an existing view's v.m.appConfiguration as parameters in the envelope, trim the mmif part to the views before that view, run the app, and it should recreate the view, with two caveats:

• exact-replay holds only under the same app version. For example, if the original view was created with an older app version, and a new version added a new default parameter, the appConfiguration in the new view will be different from the old pasted one.
• the recreated view's own v.m.parameters record won't match the original parameters in the envelope, since the envelope's parameters are expected to be valid JSON, whereas once written to v.m.parameters they're normalized to lists of strings.

If view.metadata.parameters only include those that were typed then that would exclude parameters from an envelope, which would be fine if the above is the case.

No; envelope params are recorded in parameters just like query-string and CLI params. unwrap_if_envelope() merges the envelope params into the runtime params before _refine_params, so by recording time they're indistinguishable from anything user-typed. Also note that users can pass enveloped params and hand-typed params in the same run (cat <some_mmif> | clams envelop <param_json> | python cli.py --param1=foo ...), and in such a case the parameters record will contain both (conflicts resolved by hand-typed params winning).

Incidentally, in my experiments where I hand parameters in as strings I do not get to see a view.metadata.parameters property. Not sure why.

we traced this to an unrelated bug in the clams shack code during our meeting last week.

on always keeping an empty {}

I dug into this and we can't do it at the clams-python level, and I now think we might not need to.

• the mmif json schema has parameters as an optional field
• mmif-python's default serialization behavior is to omit optional fields that are empty
• mmif-python's default deserialization behavior is to add empty data for omitted optional fields, so when you load a view with no v.m.parameters, internally (in memory) it will have v.m.parameters = {}. so consumers (code) can rely on the key being there and the value being empty. human readers won't see any parameters because none is passed at runtime anyway.
• if we really wanted to "fix" this, the easiest way would be to change the json schema to make parameters required, but that would be a breaking change.
• the next easiest way would be to change the mmif-python serialization to always write an empty {} for parameters, but I don't think that's worth it.