Allow typing.NamedTuple to be serialized

[vmarkovtsev]

This allows to serialize typing.NamedTuple-s and other crazy inheritors from list and tuple which do not preserve the original constructor signature.

We catch TypeError in the constructor and use the auxiliary list.

[stsci-bot]

Hi there @vmarkovtsev 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

[coveralls]

Coverage increased (+0.02%) to 93.864% when pulling 975503f on vmarkovtsev:patch-3 into 317a4ee on spacetelescope:master.

[drdavella]

Hi @vmarkovtsev, thanks for the PR. I am fully in favor of supporting serialization of namedtuple. I am a little wary of the fact that true roundtrip behavior is not preserved by this change. However I'm not sure whether there's any way around it since I don't know if it will be possible to write an actual tag for these types.

[drdavella]

Overall this looks good, modulo the requested changes and the philosophical considerations mentioned above. Ideally we could implement a tag for these cases, but I'm not sure it's possible.

Maybe @perrygreenfield has an opinion about whether it's okay to serialize these types without preserving true roundtrip behavior?

[drdavella]

There is extraneous whitespace on these two lines.

[drdavella]

There is extraneous whitespace on this line as well.

[drdavella]

Can you please add a test showing that recursive serialization of namedtuple members is handled correctly? For example, you could create a namedtuple where one of the members is an np.ndarray.

[vmarkovtsev]

Added more tests and removed whitespaces.

[vmarkovtsev]

@drdavella ping

[drdavella]

@vmarkovtsev I'd like some input from @perrygreenfield before merging. I have some lingering concerns over whether this is okay since it does not preserve true round-trip behavior, although maybe it doesn't matter too much.

Just for the sake of argument: what would prevent a user from performing this conversion explicitly before passing to ASDF? I realize it's probably less convenient and/or elegant, but it would put the burden of information loss on the end user rather than requiring ASDF to strip away information implicitly under the hood.

[vmarkovtsev]

Yep, I see the point. On the other side, the exact subtypes of namedtuple are always defined in the source code (namedtuple and NamedTuple are abstract) and it is impossible to recover them without saving the source code - thus we arrive at pickles. But the ultimate parent of those is tuple and it is possible to recover the value after serialization because the order of elements persists.

ASDF already strips items with None values btw.

[drdavella]

the exact subtypes of namedtuple are always defined in the source code (namedtuple and NamedTuple are abstract) and it is impossible to recover them without saving the source code

This seems like a design flaw to me, but there's not much we can do about it. Unfortunately it means we can't write a tag for these types, which would be the ideal solution.

ASDF already strips items with None values btw.

Interesting point, although I think it's really a separate issue (e.g. #536 😉).

[perrygreenfield]

I’ll take a look at this today (though I have github authentication issue due to getting a new phone…)

[drdavella]

@vmarkovtsev maybe as a compromise for now we could add a top-level option like implicit_type_conversion to enable this behavior (defaults to False)? Also it would be useful to emit a warning when this occurs.

[drdavella]

I believe this was changed by mistake.

[vmarkovtsev]

Yep

[vmarkovtsev]

How about implicit_lists? It is only used for those types which inherit from list or tuple and do not preserve the __init__ signature. -1 to warnings because when the user sets this flag to True they know what they are doing.

[perrygreenfield]

If we support that, why not implicit_dicts as well? I tend to agree with not requiring the warning. These mechanisms would be advertised as an easy way to save python info, but only in one direction (without the user reconstructing the objects outside of ASDF).

[vmarkovtsev]

Do we have an example in the wild when this code fails in treeutil.py:

if isinstance(tree, dict):
    result = tree.__class__()
    seen.add(id_tree)
    for key, val in tree.items():
        val = recurse(val)
        if val is not None:
            result[key] = val
    seen.remove(id_tree)

All dicts have items() and __setitem__, otherwise something is really screwed up on the user's side.
I guess there will be no problems afterwards...

[drdavella]

Sorry to waffle, but here's one more proposal: perform the conversion by default, but with a warning, and add a top-level option to suppress the warnings like ignore_implicit_conversion=False or some such.

In this case I like the generality of "implicit conversion" vs "implicit list" or "implicit dict" since it lets us overlook the dict case and other possible cases for now (although I agree with @vmarkovtsev that namedtuple is probably fairly unique).

This also parallels existing top-level options ignore_version_mismatch and ignore_unrecognized_tag. (Btw, eventually it might be nice to have some other configuration mechanism for these kinds of options).

[vmarkovtsev]

I like this proposal.

[drdavella]

@vmarkovtsev cool. I can finish up this PR if you want, as long as I'm able to push to your branch.

[vmarkovtsev]

I sent you the push permission invitation just in case.

[drdavella]

Thanks @vmarkovtsev!