DM-55281: Add lsst-images-admin diagram subcommand for model layouts - #57
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 77.24% 78.36% +1.12%
==========================================
Files 121 124 +3
Lines 14626 15426 +800
==========================================
+ Hits 11298 12089 +791
- Misses 3328 3337 +9 ☔ View full report in Codecov by Harness. |
57782d1 to
5c7eda4
Compare
| "SS05", # pydocstyle is better at finding infinitive verb. | ||
| "GL03", # ruff format of docstrings disagrees. | ||
| "PR04", # We use type annotations. | ||
| "PR01", # Not all parameters need to be documented. |
There was a problem hiding this comment.
@TallJimbo I am thinking that we might want to re-enable this and fix the public methods that do not document parameters. It seems like disabling this can lead to public methods we really care about missing parameter docs.
There was a problem hiding this comment.
I'm fine with turning this check back on. I turned it off because I got annoyed writing repetitive docstrings for single-argument functions, but I agree that we want this check in other cases, and the cost of those repetitive docstrings is not high.
Render the composition layout of lsst.images serialization models as Mermaid, Graphviz dot, or an ASCII tree. The core lives in a reusable lsst.images._diagram module (build_graph / build_instance_graph / graph_from_file / render) with a thin click wrapper. Type mode walks a model's annotations (resolving PEP 695 aliases and Annotated wrappers); instance mode (--from-file) walks a file's reference tree and reports only what is stored, collapsing unions such as the PSF to the concrete type present. A curated policy collapses serialization-helper leaves by default, adjustable via --collapse/--expand/--expand-leaves, and scalar fields are hidden unless --attributes is given. The --hide-type drops a type from the diagram entirely, removing every edge that points at it, matching by public or serialization name. Unlike --collapse (which keeps the type as a leaf), --hide-type removes the node and any reference left with no remaining targets, in both type and instance modes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
class_for_schema resolves third-party lsst.images.schemas entry points lazily by name, but --list only unioned the registry with the built-in providers, so an entry-point-only schema worked when named explicitly yet never appeared in the listing. Enumerate the entry point group by name (without importing the providers) so the listing matches what can actually be diagrammed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Handle optional containers of model types when classifying annotations so those fields render as references instead of scalar attributes. Merge observations from repeated instances of the same model class when building concrete file diagrams, preserving all nested concrete union members that appear in the file. Co-authored-by: Codex 5.5 <codex@openai.com>
Reject files containing merge conflict markers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Run the full .pre-commit-config.yaml suite via prek on push to main and on pull requests, covering hooks not exercised by the ruff-only formatting workflow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@TallJimbo The new diagramming code was all added by Claude. I attached the work to the Jira ticket for me writing the talk (since I did this for the SPIE talk) -- I did not put the Jira ticket in review because I haven't quite finished the talk yet. Not sure if you want to really look in detail at the Probably best to focus on the public diagramming API and the command line interface. Additionally I have:
Main issue with the validation is whether to turn on PR01 (parameters must be documented for public methods) which I had to turn off since there are lots of internal methods that look like public methods but don't document much. I can try to fix those and I think not fixing them will lead to real public APIs potentially having problems. |
TallJimbo
left a comment
TallJimbo
left a comment
There was a problem hiding this comment.
Looks good. I just skimmed the guts of the implementation.
I am a bit worried that overall we're accumulating a lot of code to maintain, between utilities like this and archive implementations we don't have concrete plans to utilize. But I also can't point to anything specific that I want to drop - this certainly looks useful for documentation! - and I think it's not clear yet whether AI helps enough with maintenance to offset how much it helps with feature-creation. So no action needed on this now, but it's a concern I wanted to share so we can both keep an eye on it.
| # Per-backend array-payload pointers seen when diagramming a file. | ||
| "JsonRef", | ||
| "PointerModel", | ||
| "NdfPointerModel", |
There was a problem hiding this comment.
Do we have any tests that guard against forgetting to update this list when we add a new archive implementation?
There was a problem hiding this comment.
I think this is not something that can be automated. This is a list of the models that we don't want to expand by default. When you add a new model you can't automatically determine whether it is meant to be expanded or not so I think you end up doing a diagram and then getting annoyed that there is a lot of extra detail that you didn't want by default.
Remove PR01 from the numpydoc_validation ignore list so public methods are required to document their parameters again, and add the parameter docstrings that this surfaces across the package. Parameters are documented without a type, since Sphinx picks up the type from the annotation (PR04 remains disabled for the same reason). Click command functions carry a `# numpydoc ignore=PR01` comment instead of a Parameters section, because click embeds the docstring into its --help output where a numpydoc Parameters block renders incorrectly; the option help text lives in the click declarations. Also fixes two pre-existing docstring issues uncovered by the check: a wrong-axis summary on SkyFrame.standardize_y and the parameter order in the CellField class docstring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
250 methods/functions had parameters added... |
2 participants
Render the composition layout of lsst.images serialization models as Mermaid, Graphviz dot, or an ASCII tree. The core lives in a reusable lsst.images._diagram module (build_graph / build_instance_graph / graph_from_file / render) with a thin click wrapper.
Type mode walks a model's annotations (resolving PEP 695 aliases and Annotated wrappers); instance mode (--from-file) walks a file's reference tree and reports only what is stored, collapsing unions such as the PSF to the concrete type present. A curated policy collapses serialization-helper leaves by default, adjustable via --collapse/--expand/--expand-leaves, and scalar fields are hidden unless --attributes is given.
Example
$ lsst-images-admin diagram --from-file dp2-new.fits --hide-field data --collapse ButlerInfo --collapse Transform --hide-type TableModel --format dot --collapse Amplifier --collapse DetectorAttributes | dot -Tpng -o visit.pngGives:
Checklist
doc/changes