Add design overview #730

perrygreenfield · 2020-01-02T20:45:54Z

This adds an initial design review document aimed at those trying to understand the internal workings of ASDF. The focus on this initial version is what happens when an ASDF file is read.

This addresses [#729]

nden · 2020-01-02T21:08:14Z

CHANGES.rst

+----------------
+
+- Add a developer overview document to help understand how ASDF works
+  internally. Still a work in progress. [#729] 


The PR number is 730.

OK, I put the the issue number (a bit of chicken and egg thing since one can't be sure of the PR number until it is submitted, can one?)

nden · 2020-01-02T21:11:02Z

docs/asdf/developer_overview.rst

+**Resolver:** Tools to map URIs and tags into actual locations of schema files,
+**which may be local directories (the usual approach) or an actual URL for
+**retrieval over the network. This is more complicated that it may seem for
+**reasons explained later.


Remove the ** from lines 38-40.

These appeared when wrapping the text (editor was too clever). I thought I had caught all of these, but obviously not.

codecov · 2020-01-02T21:18:12Z

Codecov Report

Merging #730 into master will not change coverage.
The diff coverage is 99.29%.

@@           Coverage Diff           @@
##           master     #730   +/-   ##
=======================================
  Coverage   93.94%   93.94%           
=======================================
  Files          43       43           
  Lines        4889     4889           
=======================================
  Hits         4593     4593           
  Misses        296      296

Impacted Files	Coverage Δ
asdf/generic_io.py	`94.36% <100%> (ø)`	⬆️
asdf/commands/info.py	`100% <100%> (ø)`	⬆️
asdf/types.py	`96.46% <100%> (ø)`	⬆️
asdf/extension.py	`91.12% <100%> (ø)`	⬆️
asdf/tags/core/__init__.py	`100% <100%> (ø)`	⬆️
asdf/schema.py	`93.48% <100%> (ø)`	⬆️
asdf/commands/__init__.py	`100% <100%> (ø)`	⬆️
asdf/exceptions.py	`100% <100%> (ø)`	⬆️
asdf/_convenience.py	`100% <100%> (ø)`	⬆️
asdf/asdf.py	`94.11% <100%> (ø)`	⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 349e20f...8e8444a. Read the comment docs.

mcara · 2020-01-02T21:20:33Z

docs/asdf/developer_overview.rst

+The design is somewhat complicated by the fact that in using pyyaml and
+jsonschema, one uses them by supplying code for these libraries to use, and
+without becoming familiar with how these packages work, the operation of the
+asdf internals isn't always very clear. This will try to provide a small amount


Suggested change

asdf internals isn't always very clear. This will try to provide a small amount

asdf internals isn't always very clear. This document will try to provide a small amount

mcara · 2020-01-02T21:32:27Z

docs/asdf/developer_overview.rst

+expected to grow organically so at the moment it should not be considered
+complete or comprehensive.
+
+The design is somewhat complicated by the fact that in using pyyaml and


This is already scary :) I just started reading this document and already discovered that I was supposed to know pyyaml and jsonschema and that these packages were used in asdf. I think reformulating this might help.

Also in using ..., one uses them by supplying code for these libraries to use This is very confusing formulation for me and verb "use" used three times in a single sentence.

@mcara Not sure what you mean by the first (regarding reformulating). As to the second, I blame a trancelike mode resulting in stream of gibberish text.

The design is somewhat complicated by the fact that in using pyyaml..." sounds to me like I am suddenly presented with this new package pyyaml. Maybe the sentence should be changed to (if one assumes that pyyaml is a prerequisite) something that introduces pyyaml in a more smooth way. Also, if it is a prerequisite, then state so at the beginning. If not - introduce it first.

Another thing that is not clear to me: "The design is somewhat..." Design of what? Of schemas, tags, or this document?

I've tried to write this more clearly in the version I just pushed.

nden · 2020-01-02T21:42:14Z

docs/index.rst

+Currently a work in progress. Intended to give an overview of how the various 
+parts of ASDF interact and which modules do what and how.
+
+.. toctree:


Needs a double colon, hopefully this will fix the error.

I was seeing single when I should have been seeing double!

mcara · 2020-01-02T21:46:06Z

What are the prerequisites assumed that reader knows before reading this doc? Maybe there should be a paragraph saying this from the beginning. Also, is it assumed that users know what schema and tags are?

perrygreenfield · 2020-01-02T21:49:46Z

@mcara That's a good question, and one I'm not terribly clear on myself. Being too specific about this may unduly scare people off. I figure that if they are familiar with using these features that they probably have dealt with schemas and tags already, and if they haven't they probably should play with them. So perhaps a paragraph that says that it assumes they have some familiarity with the use of both? But overall, it was targeted at my level of ignorance (i.e., moderately high)

mcara · 2020-01-02T22:38:43Z

docs/asdf/developer_overview.rst

+the pyyaml and jsonschema libraries use. Understanding what is going on
+thus means having some understanding of the relevant parts of the 
+internals of both of those libraries. This overview will try to provide 
+a small amount of context for these package to illuminate how the code 


Suggested change

a small amount of context for these package to illuminate how the code

a small amount of context for these packages to illuminate how the code

embray · 2020-01-03T13:23:50Z

Perhaps a somewhat related issue, but a lot of the YAML-specific hacks in ASDF are not entirely ASDF-specific and this might be worth emphasizing where true.

In particular, I've had a handful of unrelated discussions in recent months where I've told people about our YAML Schema which might have been of interest to them. But the only implementation I know of YAML Schema is the one buried in pyasdf, even though it could (and I think should) easily stand on its own.

perrygreenfield · 2020-01-03T14:22:21Z

@embray That's a good point. How much interest do you think there is in such a package? I haven't given any thought to how much work would be involved in splitting that out. I guess mostly in the validation.py and yamlutil.py areas.

nden · 2020-01-06T16:22:48Z

docs/asdf/developer_overview.rst

+retrieval over the network. This is more complicated that it may seem for
+reasons explained later.
+
+**Validation:** Tool to confirm that the YAML conforms to the schemas that


Should it be called Validator (a tool) or alternatively Validation: The process ...

nden · 2020-01-06T16:27:37Z

docs/asdf/developer_overview.rst

+parser converts the raw YAML into a custom Python structure. It is that
+structure that is validated. Then if no errors are found, the tree is
+converted into a tree where tagged nodes get converted into special Python
+objects (usually), e.g., WCS object or numpy arrays (well, not quite that


Why usually? When is it not converted? If you mean that it is not converted to Python objects when it fails validation, then usually should be removed as you said that happens when there are no errors.

Instead of get converted into special Python objects is it clearer to say something like get converted into the corresponding Python objects.

@nden Does the latest version address this? The context is when an option is supplied to prevent the conversion (_force_raw_types).

Ah, OK. I was not aware of that flag. And it's not documented.

nden · 2020-01-06T16:29:24Z

docs/asdf/developer_overview.rst

+
+Most of resolver tools and code is in ``resolver.py`` (but not all)
+
+Most of the validation code is in ``schema.py``


Add period at the end of the above two sentences.

nden · 2020-01-06T16:37:49Z

docs/asdf/developer_overview.rst

+The yaml parsing phase described below normally returns a "tagged_tree". That is
+(somewhat simplified), it returns the data structure that yaml would normally
+return without any object conversion (i.e., all nodes are either dicts, lists,
+or scalar values), except that they are objects that now sport a tag attribute


sport --> support

nden · 2020-01-06T16:54:49Z

docs/asdf/developer_overview.rst

+up the af object intrinsically). The function then passes the callback walker to
+treeutil.walk_and_modify() where the tree will be traversed recursively applying
+the tag code associated with the tag to the more primative tree representation
+replacing such nodes wtih Python objects. The tree travsersal starts from the


wtih --> with

nden · 2020-01-06T17:05:17Z

docs/asdf/developer_overview.rst

+it doesn't appear to vary except for that. Admittedly, this is only created at
+the top level. This is called by ``get_validator``.
+
+**class OrderedLoader:** Hnherits from the ``_yaml_base_loader``, but otherwise


Hnherits --> Inherits

embray · 2020-01-07T12:43:42Z

@perrygreenfield

I guess mostly in the validation.py and yamlutil.py areas.

Yep (I'm not sure what validation.py is; maybe you meant schema.py?). Looking at them again, there's really very little about it that's specific to ASDF. Some of the naming (e.g. AsdfLoader) makes it seem that way, but very little of it is particular to ASDF so much as to YAML Schema itself, including things like the "tagged" object wrappers. This stuff I think is broadly applicable and useful and I have wanted something like this for my own use.

It's hard to judge exactly how much demand there is for this more broadly-speaking, beyond some discussions I've seen on Twitter. But from what I can tell, although it has evolved very little there is still a lot of interest in and demand for YAML, and better tools for manipulating and validating YAML documents, and I think there's a lot of richness in what we've done here.

perrygreenfield · 2020-01-07T13:58:44Z

Yeah, schema.py is what I meant. In any case, it can't hurt to make it available (except for work on our end, of course).

eslavich · 2020-02-10T15:43:18Z

@perrygreenfield is this ready to merge?

perrygreenfield · 2020-02-10T15:46:06Z

I think so, aside from the CHANGES.rst item, but needs reviewer approval doesn't it?

eslavich · 2020-02-10T15:49:04Z

Oh, ha, it's waiting on me isn't it. I'll take a look now.

eslavich

Thanks for the write-up, I found it helpful! I pointed out some typos but other than that it looks good. I'm planning to add a section on versioning once this is merged.

docs/asdf/developer_overview.rst

eslavich · 2020-02-10T16:16:45Z

docs/asdf/developer_overview.rst

+top, but the objects are created from the bottom up due to  recursion.
+
+The result is what af.tree is set to, after doing another tree travseral looking
+for special type hooks for each node. It isn't cleaf if there is yet any use of that


cleaf --> clear

docs/asdf/developer_overview.rst

eslavich · 2020-02-10T16:17:54Z

docs/asdf/developer_overview.rst

+(This is the major reason that the tree isn't  directly converted to an object
+tree since jsonschema would not be able to use the  final object tree for
+validation, besides issues relate to the fact that things that don't validate
+may not be converable to the designated object.) 


converable --> convertable

docs/asdf/developer_overview.rst

perrygreenfield added 2 commits January 2, 2020 14:50

adding developers overview document

8828934

wrapped lines

e7dce99

nden reviewed Jan 2, 2020

View reviewed changes

nden added the Docs label Jan 2, 2020

nden added this to the 2.6 milestone Jan 2, 2020

nden requested review from eslavich, jdavies-st and mcara January 2, 2020 21:11

address review comments

7c65130

mcara reviewed Jan 2, 2020

View reviewed changes

nden reviewed Jan 2, 2020

View reviewed changes

address more review comments

becfe6d

mcara reviewed Jan 2, 2020

View reviewed changes

perrygreenfield added 2 commits January 6, 2020 10:52

address more review comments and remove use of first person in text

268558f

resolve conflict in CHANGES.rst due to recent commit

0b67882

nden reviewed Jan 6, 2020

View reviewed changes

edits to address Nadia's comments

882da34

eslavich approved these changes Feb 10, 2020

View reviewed changes

perrygreenfield added 2 commits February 10, 2020 13:14

corrections of various typos

5abac4c

filled in ... with specific conclusion

ccda5d1

perrygreenfield added 3 commits February 10, 2020 13:24

merge other changes made to CHANGES.rst

52eaa96

merge one more change made to CHANGES.rst

349e20f

Merge branch 'master' into designoverview

8e8444a

perrygreenfield merged commit 29033ac into asdf-format:master Feb 11, 2020

nden mentioned this pull request Apr 13, 2020

Add a developer's overview document #729

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add design overview #730

Add design overview #730

perrygreenfield commented Jan 2, 2020

nden Jan 2, 2020

perrygreenfield Jan 2, 2020

nden Jan 2, 2020

perrygreenfield Jan 2, 2020

codecov bot commented Jan 2, 2020 •

edited

Loading

mcara Jan 2, 2020

mcara Jan 2, 2020

perrygreenfield Jan 2, 2020

mcara Jan 2, 2020

perrygreenfield Jan 2, 2020 •

edited

Loading

nden Jan 2, 2020

perrygreenfield Jan 2, 2020

mcara commented Jan 2, 2020

perrygreenfield commented Jan 2, 2020

mcara Jan 2, 2020

embray commented Jan 3, 2020

perrygreenfield commented Jan 3, 2020

nden Jan 6, 2020

nden Jan 6, 2020

perrygreenfield Jan 6, 2020

nden Jan 6, 2020

nden Jan 6, 2020

nden Jan 6, 2020

nden Jan 6, 2020

nden Jan 6, 2020

embray commented Jan 7, 2020

perrygreenfield commented Jan 7, 2020

eslavich commented Feb 10, 2020

perrygreenfield commented Feb 10, 2020

eslavich commented Feb 10, 2020

eslavich left a comment

eslavich Feb 10, 2020

eslavich Feb 10, 2020

	asdf internals isn't always very clear. This will try to provide a small amount
	asdf internals isn't always very clear. This document will try to provide a small amount

	a small amount of context for these package to illuminate how the code
	a small amount of context for these packages to illuminate how the code


		Most of resolver tools and code is in ``resolver.py`` (but not all)

		Most of the validation code is in ``schema.py``

Add design overview #730

Add design overview #730

Conversation

perrygreenfield commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 2, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrygreenfield Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcara commented Jan 2, 2020

perrygreenfield commented Jan 2, 2020

Choose a reason for hiding this comment

embray commented Jan 3, 2020

perrygreenfield commented Jan 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embray commented Jan 7, 2020

perrygreenfield commented Jan 7, 2020

eslavich commented Feb 10, 2020

perrygreenfield commented Feb 10, 2020

eslavich commented Feb 10, 2020

eslavich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 2, 2020 •

edited

Loading

perrygreenfield Jan 2, 2020 •

edited

Loading