Changes needed to support info and search methods on roman_datamodels #997
Conversation
| @@ -365,11 +370,11 @@ def _handle_immutable_sequence(node, json_id): | |||
| return result | |||
|
|
|||
| def _handle_children(node, json_id): | |||
| if isinstance(node, dict): | |||
| if isinstance(node, Mapping): | |||
There was a problem hiding this comment.
It's not safe to change this to match any Mapping or MutableSequence, since walk_and_modify relies on being able to construct a new instance of the container with no arguments. For example, a tree containing an equivalency fails because Equivalency.__init__ has a required argument:
import asdf
from astropy.units import equivalencies
tree = {"equiv": equivalencies.spectral()}
asdf.treeutil.walk_and_modify(tree, lambda n: n)TypeError: __init__() missing 1 required positional argument: 'equiv_list'
| @@ -82,8 +83,9 @@ def from_root_node(cls, root_identifier, root_node): | |||
| next_nodes = [] | |||
|
|
|||
| for parent, identifier, node in current_nodes: | |||
| if (isinstance(node, dict) or isinstance(node, list) or isinstance(node, tuple)) and id(node) in seen: | |||
| info = _NodeInfo(parent, identifier, node, current_depth, recursive=True) | |||
| if (isinstance(node, Mapping) or isinstance(node, MutableSequence) or isinstance(node, tuple)) and id(node) in seen: | |||
There was a problem hiding this comment.
I'm starting to have some reservations about crawling all subclasses of Mapping and MutableSequence. I assume we're skipping Sequence here because that would match strings -- are there other types that will end up being similarly inconvenient to have split into separate nodes in the info display?
What do you think about having the user or the extension opt in each traversable class? On an extension that might look something like:
class SomeExtension:
traversable_types = [
"roman_datamodels.stnode.DNode",
"roman_datamodels.stnode.LNode",
]There was a problem hiding this comment.
Alternatively we could create a marker interface, something like:
class AsdfTraversable:
passand include that among DNode and LNode's parent classes:
from asdf.extension import AsdfTraversable
class DNode(UserList, AsdfTraversable):
# blah blahI like that better from an implementation point of view, but an extension author wouldn't be able to mark classes that they don't control.
There was a problem hiding this comment.
Belatedly, I don't see that as a problem normally. It's needed here since we are building a tree of objects, and if someone else has a similar need, they can use a similar solution.
There was a problem hiding this comment.
As I try to sweep the cobwebs away on this one. Is there any reason that DNode and LNode shouldn't automatically be traversed?
As for other possibly traversable objects, we could have such objects provide a method to return whatever structure they think appropriate for their contents. Any reason not to let them control that completely? The transversing code checks for the presence of such a method, and if it exists, calls it. Should there be reference objects within that also support that interface, they can also be called, recursively. I suppose we could use this for DNode and LNodes as a uniform interface.
There was a problem hiding this comment.
I like the idea of a method, __asdf_traverse__?
There was a problem hiding this comment.
But how to handle classes that are implemented by a 3rd party and not necessarily modifiable by asdf developers or the extension author?
There was a problem hiding this comment.
OK, I'll give it a try. That seems like a good name. I was going to go with __asdf_the_doors_open_so_come_in_and_look_around__ but that might be a hair too long.
There was a problem hiding this comment.
I suppose I should close this and open a new PR. What should I base it off of given all the changes going on?
There was a problem hiding this comment.
As for 3rd party classes that don't implement it, tough. They don't want to be introspected, would be the simple answer. Otherwise we could implement a registry of some sort that knows about them and has code to deal with that. But not for this PR.
There was a problem hiding this comment.
I recommend branching off of master so that we can release this sooner in a 2.x version.
|
Hey Perry, is this modification something that might help with the Roman data_models and validation meeting for Thursday. Should we try and discuss this as well? |
|
I'm not sure it helps in the issues that Paul raised. But I prefer Ed's solution to this issue. Normally these utilities stop at anything that is turned into a special object, and since the whole data model is a special object, they will stay opaque to these tools unless we indicate that it should dig deeper. |
|
This has been superseded by #1052 |
Since roman_datamodels has nodes that are dict and list like, but not actually dicts and lists, the asdf info and search machinery will not work with them. These changes allow use of any Mapping and MutableSequence classes in their place, allowing the functionality to extend to roman_datamodels.
One test fails, but seems unrelated to this.
Not sure how to handles supposed changes to asdf-standard.
And I see my editor has done some PEP 8 reformatting. Sigh.