Repository navigation
Conversation
Adds a write-conversion protocol for packages to extend allowing arbitrary Julia types to be embedded into ASDF documents.
49cb287 to
46c4bde
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 589 622 +33
=========================================
+ Hits 589 622 +33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hi Chris, thanks for this much needed PR! It adds a good bit of functionality, but I must admit that the added complexity has moved this review pretty far out of my area of expertise. To that end, I spent the last few days going back-and-forth with claude, and settled on this first pass at a review for you to take a look at.
Review - 46c4bde
Code review: write-converter branch
Commit under review: 46c4bde "Add to_tree for extendable type writing" (one commit on top of main, working tree clean)
Diff: git diff main...write-converter, 7 files, +316 / −2
Date: 2026-09-10
Model: Fable 5.1 (ultracode)
Scope and method
The branch adds a write-side conversion hook (ASDF.to_tree), an opaque ASDF.WriteContext, recursive tree conversion with cycle detection, a new docs page, and test/test-write-converters.jl.
Baseline on this branch, before looking for problems:
- The new tests, Aqua, and doctests all pass.
- A
main-vs-branch round-trip on a broad document (plain scalars, floats, nested dicts, vectors, tagged nodes, binary blocks) produces byte-identical files, so the change does not alter existing output.
Every finding below is therefore a case the tests do not cover. Each code snippet is self-contained: run it as-is from the repository's test project environment (julia --project=test), which provides ASDF.jl and OrderedCollections. Output shown in comments is what Julia 1.13.0 printed when each snippet was run from a fresh session. Findings were each checked by an independent verifier that tried to refute them; the three highest-impact ones were additionally reproduced a second time on Julia 1.12 before this document was written. Line numbers link to the reviewed commit.
Summary
| # | Severity | Location | Finding |
|---|---|---|---|
| 1 | High | src/ASDF.jl:1203 |
Hook output is never re-dispatched through to_tree; a hook returning another hooked object writes string(val) into the file silently |
| 2 | High | src/ASDF.jl:1186 |
A hook with an unannotated context argument is an ambiguity MethodError on every call |
| 3 | Medium | src/ASDF.jl:1202 |
Self-tagging AbstractDict/AbstractVector subtype throws a false "cyclic" error |
| 4 | Medium | src/ASDF.jl:1187 |
A wrong-arity one-argument hook works at the REPL but is silently ignored by write_file |
| 5 | Medium | src/ASDF.jl:1207 |
Walker skips Tuple/NamedTuple/Pair/AbstractSet and never validates leaves; a Pair produces an unloadable file |
| 6 | Medium | src/ASDF.jl:1232 |
map keeps a Matrix a Matrix; N-d arrays are written as text and the new test cements it |
| 7 | Medium | src/ASDF.jl:1870 |
to_tree then yaml_compliant rebuilds the whole tree twice, roughly doubling pre-write cost |
| 8 | Design | src/ASDF.jl:1165 |
WriteContext is public but carries only private cycle-guard state; a one-argument hook plus a private walker is the minimal form and removes findings 2 and 4 |
| 9 | Low | src/ASDF.jl:1200 |
A hook that recursively calls to_tree on cyclic input overflows the stack instead of raising the documented ArgumentError |
| 10 | Low | src/ASDF.jl:1226 |
Mapping keys are never passed through the converter |
| 11 | Low | src/ASDF.jl:1189 |
Cycle bookkeeping is spread over five sites; the TaggedScalar method is dead code |
| 12 | Low | docs/src/api.md:24 |
Hand-maintained Filter mirrors the manual @docs list; both are deletable |
| 13 | Low | docs/src/custom-types.md:16 |
Example uses an unordered Dict, so the written key order is reversed from the source |
| 14 | Low | test/test-write-converters.jl:60 |
Abstract-type fixtures and the save block test Julia dispatch, not ASDF |
| 15 | Low | src/ASDF.jl:1064 |
The Tagged* docstring never says the tag field must be the full URI |
Recommended direction
Findings 2, 4, 8, and 11 share one root cause: the public surface splits into a two-argument hook and a one-argument walker, joined by a WriteContext that no hook can actually use. A patched copy of the branch was built and tested with this shape instead (a sketch of the replaced internals in src/ASDF.jl, not a standalone snippet):
# The hook. Packages extend this one method.
to_tree(value) = value
# Private walker; `active` is the cycle guard.
function _convert_tree(value, active = Base.IdSet{Any}())
value in active && throw(ArgumentError("cyclic ASDF write conversion involving $(typeof(value)) is not supported"))
push!(active, value)
try
return _convert_tree_children(to_tree(value), active)
finally
delete!(active, value)
end
end
_convert_tree_children(value, active) = value
_convert_tree_children(v::TaggedMapping, active) = TaggedMapping(v.tag, _convert_tree_children(v.value, active))
_convert_tree_children(v::TaggedSequence, active) = TaggedSequence(v.tag, _convert_tree_children(v.value, active))
_convert_tree_children(v::AbstractDict, active) = OrderedDict{Any, Any}(k => _convert_tree(item, active) for (k, item) in v)
_convert_tree_children(v::AbstractArray, active) = map(item -> _convert_tree(item, active), v)With WriteContext deleted (48 lines fewer overall) the full Pkg.test() passes on Julia 1.10.12 and 1.12.7. The untyped-context hook problem disappears because there is no context argument. A mistaken hook arity becomes visible at the REPL. The single guard catches 9/9 cycle cases including the isbits CyclicWriteValue, flags 0/7 DAG (shared, non-cyclic) inputs, and converts the self-tagging AbstractDict subtype from finding 3 correctly. Base.IdSet is available on Julia 1.10 and wraps the same IdDict{Any, Nothing} the branch uses.
If a context is genuinely needed later (schema selection, array-storage policy), it can be added additively as to_tree(value, context) = to_tree(value) without breaking anyone. Under the project's alpha policy this is a routine minor-version change either way.
Findings 1, 5, 6, 7, 9, and 10 are independent of that choice and are discussed individually below.
Correctness
1. Hook output is never re-dispatched through to_tree — High
_convert_tree calls the hook once and passes the result straight to _convert_tree_children(converted, context). The result never re-enters to_tree. A hook that delegates to another hooked type, or that returns a hooked AbstractDict subtype, therefore leaves that object unconverted, and YAML.jl's fallback writes string(val) into the file with no error.
using ASDF, OrderedCollections
struct Kelvin; k::Float64; end
struct Celsius; c::Float64; end
ASDF.to_tree(k::Kelvin, ::ASDF.WriteContext) = ASDF.TaggedMapping("tag:example.org/kelvin-1.0.0", OrderedDict("k" => k.k))
ASDF.to_tree(c::Celsius, ::ASDF.WriteContext) = Kelvin(c.c + 273.15)
ASDF.to_tree(Celsius(20.0))
# Kelvin(293.15) not a TaggedMapping: the Kelvin hook never ran
f = tempname()
ASDF.write_file(f, OrderedDict("t" => Celsius(20.0))) # no error; the file contains t: Kelvin(293.15)
ASDF.load_file(f)["t"]
# "Kelvin(293.15)"An in-contract variant: a hook returns a custom AbstractDict subtype that has its own tagging hook. _convert_tree_children(::AbstractDict) iterates it as a plain mapping and the tag is silently dropped, while the same object placed as a child keeps its tag.
Suggested fix. Re-enter _convert_tree(converted, ctx) when converted !== value. Note that a naive re-dispatch turns a hook that returns a fresh object of its own type into unbounded recursion (this wedged Julia 1.12 during verification), so guard with typeof(converted) === typeof(value) or a depth limit, or alternatively make the leaf fallback error on types the writer does not support (see finding 5).
2. Unannotated context argument is an ambiguity MethodError — High
The fallback is to_tree(value, ::WriteContext) = value, with signature (Any, WriteContext). The idiomatic hook a package author will write, given that the docs say to treat the context as opaque, is (Measurement, Any). Neither method is more specific than the other.
using ASDF, OrderedCollections
struct Meas; v::Float64; end
ASDF.to_tree(m::Meas, context) = ASDF.TaggedMapping("tag:example.org/meas-1.0.0", OrderedDict("v" => m.v))
ASDF.to_tree(Meas(1.0))
# ERROR: MethodError: to_tree(::Meas, ::ASDF.WriteContext) is ambiguous.write_file fails the same way.
Suggested fix. Change the fallback to to_tree(value, context) = value, an (Any, Any) method. The typed hooks in the tests still pass and Test.detect_ambiguities(ASDF) stays empty. The recommended direction above removes the argument entirely.
3. Self-tagging container subtype throws a false "cyclic" error — Medium
_convert_tree registers value in the active set, then _convert_tree_children(::TaggedMapping) (line 1211) or (::TaggedSequence) (line 1218) registers converted.value. When a hook on an AbstractDict or AbstractVector subtype wraps itself, those are the same object.
using ASDF, OrderedCollections
struct Header <: AbstractDict{String, Any}
d::OrderedDict{String, Any}
end
Base.iterate(h::Header, state...) = iterate(h.d, state...)
Base.length(h::Header) = length(h.d)
Base.getindex(h::Header, k) = h.d[k]
Base.get(h::Header, k, default) = get(h.d, k, default)
ASDF.to_tree(h::Header, ::ASDF.WriteContext) = ASDF.TaggedMapping("tag:example.org/header-1.0.0", h)
h = Header(OrderedDict{String, Any}("a" => 1))
ASDF.to_tree(h)
# ERROR: ArgumentError: cyclic ASDF write conversion involving Header is not supported
# Wrapping a copy instead of `h` itself works, so there is no real cycle:
ASDF.to_tree(ASDF.TaggedMapping("tag:example.org/header-1.0.0", Dict(h)))
# ASDF.TaggedMapping{OrderedDict{Any, Any}}("a" => 1)
# Same for a vector subtype wrapped in a TaggedSequence:
struct MyVec <: AbstractVector{Any}; v::Vector{Any}; end
Base.size(x::MyVec) = size(x.v)
Base.getindex(x::MyVec, i::Int) = x.v[i]
ASDF.to_tree(x::MyVec, ::ASDF.WriteContext) = ASDF.TaggedSequence("tag:example.org/myvec-1.0.0", x)
ASDF.to_tree(MyVec(Any[1]))
# ERROR: ArgumentError: cyclic ASDF write conversion involving MyVec is not supportedCalling _convert_tree_children on the converted node directly also terminates, so the guard, not the data, is at fault.
Suggested fix. A single cycle guard in _convert_tree (finding 11) removes the double registration.
4. Wrong-arity hook works at the REPL but is ignored by write_file — Medium
The public one-argument walker to_tree(value) shares its generic function with the two-argument hook. A package author who defines the wrong arity gets a working REPL call, because their more specific method shadows the walker, while write_file only ever calls the two-argument form.
using ASDF, OrderedCollections
struct Oops; n::Int; end
# Wrong arity: this defines a one-argument method, which is the walker, not the hook.
ASDF.to_tree(o::Oops) = ASDF.TaggedMapping("tag:example.org/oops-1.0.0", OrderedDict("n" => o.n))
ASDF.to_tree(Oops(1))
# ASDF.TaggedMapping{OrderedDict{String, Int64}}("n" => 1) looks correct at the REPL
f = tempname()
ASDF.write_file(f, OrderedDict("o" => Oops(1))) # no error; the file contains o: Oops(1)
ASDF.load_file(f)["o"]
# "Oops(1)"Neither Aqua nor Julia warns about the extra arity.
Suggested fix. Give the walker a distinct or private name, or make the leaf fallback reject non-writer-supported types with an error naming ASDF.to_tree(::T, ::WriteContext). The recommended direction does the former.
5. Walker skips Tuple/NamedTuple/Pair/AbstractSet and never validates leaves — Medium
The generic fallback _convert_tree_children(value, context) = value does not descend into these containers, so hooked objects inside them are never converted and cycles through them go undetected. It also does not validate leaves, so unsupported values reach YAML.jl's string(val) fallback.
using ASDF, OrderedCollections
struct Q; n::Int; end
ASDF.to_tree(q::Q, ::ASDF.WriteContext) = ASDF.TaggedMapping("tag:example.org/q-1.0.0", OrderedDict("n" => q.n))
struct NoHook; n::Int; end # no to_tree method
doc = OrderedDict(
"t" => (Q(1), 2),
"nt" => (q = Q(1),),
"s" => Set([Q(1)]),
"p" => ("a" => 1),
"raw" => NoHook(7),
)
f = tempname()
ASDF.write_file(f, doc) # succeeds
print(read(f, String))
# ...
# t: (Q(1), 2)
# nt: (q = Q(1),)
# s: Set(Q[Q(1)])
# p: a: 1
# raw: NoHook(7)
# ...
ASDF.load_file(f)
# ERROR: while parsing a block mapping at line 8, column 0: expected <block end>,
# but found YAML.BlockMappingStartToken at line 11, column 6 (the Pair line)
delete!(doc, "p")
ASDF.write_file(f, doc)
af = ASDF.load_file(f)
af["t"], af["nt"], af["s"], af["raw"]
# ("(Q(1), 2)", "(q = Q(1),)", "Set(Q[Q(1)])", "NoHook(7)") all StringsThe unvalidated leaf and the Pair output are byte-identical on main, so that part is pre-existing. What is new is the prose: README.md:13, custom-types.md:4, and index.md:129 now promise objects convert "anywhere" in a document, while the docstrings correctly say only "mappings, arrays, or tagged nodes".
Suggested fix. Either descend into these containers, error on unsupported leaves, or narrow the prose to match the docstrings.
6. map keeps a Matrix a Matrix; N-d arrays written as text — Medium
_convert_tree_children(::AbstractArray) uses map, which preserves shape. YAML.jl only serializes AbstractVector, so an unwrapped N-d array is written as its show text.
using ASDF, OrderedCollections
f = tempname()
ASDF.write_file(f, OrderedDict("m" => [1 2; 3 4])) # the file contains m: [1 2; 3 4]
ASDF.load_file(f)["m"]
# ["1 2; 3 4"] identical on main
struct Q; n::Int; end
ASDF.to_tree(q::Q, ::ASDF.WriteContext) = ASDF.TaggedMapping("tag:example.org/q-1.0.0", OrderedDict("n" => q.n))
ASDF.write_file(f, OrderedDict("qv" => [Q(1), Q(2)], "qm" => [Q(1) Q(2)]))
print(read(f, String))
# ...
# qv:
# - !<tag:example.org/q-1.0.0>
# n: 1
# - !<tag:example.org/q-1.0.0>
# n: 2
# qm: ASDF.TaggedMapping{OrderedDict{Any, Any}}[ASDF.TaggedMapping("n" => 1) ASDF.TaggedMapping("n" => 2)]
# ...
af = ASDF.load_file(f; extensions = true)
af["qv"] # 2-element Vector of TaggedMapping, as expected
af["qm"] # one String: "ASDF.TaggedMapping{OrderedDict{Any, Any}}[ASDF.TaggedMapping(\"n\" => 1) ...]"The plain-Matrix case is byte-identical on main. The new surface is a Matrix of hooked objects: the hook runs on every element, map returns a Matrix{TaggedMapping}, and that is written as its show text, while the same values in a Vector write correctly. The new test at test/test-write-converters.jl:73 (@test plain["matrix"] == source["matrix"]) asserts the pass-through as intended behaviour.
Suggested fix. The new walk is the natural place to reject non-vector arrays, or to route them through NDArrayWrapper(...; inline = true), rather than cementing the current behaviour in a test.
7. to_tree then yaml_compliant rebuilds the whole tree twice — Medium (efficiency)
write_file now runs two structurally identical full-tree rebuilds back to back, and the new walk reallocates every container even when nothing changed.
| Document | to_tree |
yaml_compliant |
YAML.write |
|---|---|---|---|
| 20k small dicts | 28–34 ms | 31–33 ms | 29 ms |
So the new pass adds roughly 50% to write_file. On 100k dicts, yaml_compliant(to_tree(doc)) takes 430–520 ms / 186 MiB versus 137–186 ms / 100–115 MiB for one guarded walk that does both. A copy-on-write walker that returns value when every child is === unchanged drops the no-custom-object case from 182 ms / 77 MiB to 22 ms / 10 MiB, and a 1e6-element inline Vector{Float64} is no longer copied twice. The OrderedDict{Any, Any}(k => f(v) for ...) generator at lines 1212 and 1226 (and pre-existing 1265) is also 3–4x slower than a sizehint! plus setindex! loop.
Suggested fix. Fold the float rule into the walker as _convert_tree_children(v::AbstractFloat, ctx) = YAMLScalar(yaml_float_string(v)) and call one walker from write_file.
9. Recursive hook on cyclic input overflows the stack — Low
The hook runs before value is marked active (and only on the converted !== value branch), and the public one-argument to_tree always starts a fresh WriteContext. A hook that recursively converts a child therefore turns cyclic input into a StackOverflowError instead of the documented ArgumentError.
using ASDF, OrderedCollections
mutable struct Node; next; end
a = Node(nothing); a.next = a
# A hook that recurses through ASDF itself instead of returning the child:
ASDF.to_tree(n::Node, ::ASDF.WriteContext) =
OrderedDict("next" => n.next === nothing ? nothing : ASDF.to_tree(n.next))
ASDF.to_tree(a)
# Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
# ERROR: StackOverflowError segfaulted outright in one Julia 1.12 run
# The shallow form the docs describe is detected correctly (best run in a fresh session after the overflow):
ASDF.to_tree(n::Node, ::ASDF.WriteContext) = OrderedDict("next" => n.next)
ASDF.to_tree(a)
# ERROR: ArgumentError: cyclic ASDF write conversion involving Node is not supportedThe same happens if the hook calls the private _convert_tree(n.next, ctx) instead. Low severity because the docs say hooks are shallow, but the one-argument form is advertised for "inspecting".
Suggested fix. Add a doc caveat. Wrapping only the hook call in _with_active fixed the threaded-context variant with zero test regressions.
10. Mapping keys are never passed through the converter — Low
Keys are copied verbatim (key => _convert_tree(item, context), also at line 1212), so a custom key with a hook is written via YAML.jl's string(pair[1]).
using ASDF, OrderedCollections
struct K; n::Int; end
ASDF.to_tree(k::K, ::ASDF.WriteContext) = "k$(k.n)"
ASDF.to_tree(OrderedDict(K(1) => K(2)))
# OrderedDict{Any, Any}(K(1) => "k2") value converted, key not
f = tempname()
ASDF.write_file(f, OrderedDict(K(1) => K(2))) # the file contains K(1): "k2"
first(ASDF.load_file(f).metadata)
# "K(1)" => "k2"This is pre-existing (main's yaml_compliant(::AbstractDict) has the same shape) and ASDF restricts keys to bool/int/str.
Suggested fix. A one-line doc note ("keys are not converted") is proportionate.
Design and simplification
8. WriteContext is public but carries only private state — Design
context._active is read only inside _with_active (lines 1190–1195). Every hook in the tests and docs names context and never uses it, and there is no context-threaded recursive entry point a hook could pass it to. The two-argument-hook plus one-argument-walker split it forces is the root cause of findings 2 and 4. See Recommended direction above for the tested alternative.
11. Cycle bookkeeping is spread over five sites; dead TaggedScalar method — Low
The guard lives in _with_active, in the two-branch _convert_tree, and in a _with_active wrapper inside each of the four container methods, with an asymmetric value.value key for the Tagged* wrappers. A single guard inlined in _convert_tree plus four one-liners (the same shape as yaml_compliant at lines 1263–1266) is equivalent, about 25 lines shorter, and incidentally fixes finding 3. That version passes all 38 tests, catches 9/9 cycle cases including the isbits CyclicWriteValue, and flags 0/7 DAG inputs.
_convert_tree_children(::TaggedScalar) at line 1208 is dead code: TaggedScalar is neither an AbstractDict nor an AbstractArray, so it duplicates the fallback at line 1207. Deleting it leaves the WriteScalar test and the round-trip passing.
12. Hand-maintained Filter mirrors the manual @docs list — Low
Filter = value -> value ∉ (…5 names…) exists only to suppress Documenter's duplicate-docs error caused by the new manual @docs block at lines 12–18 that lists the same five names. Two lists that must stay in sync; each future write-side name needs edits in both or the build fails.
A scratch Documenter build with the "Custom type writing" section and the Filter line both removed builds strictly (exit 0, no docerrors), and every [ASDF.to_tree](@ref), [ASDF.WriteContext](@ref) and [ASDF.TaggedMapping](@ref) link in custom-types.md and index.md still resolves, with the targets back in the Private section as on main.
The diff also repeats the to_tree contract prose: custom-types.md:50–61 closely paraphrases the docstring at src/ASDF.jl:1176–1184, and README.md:13, index.md:128, and custom-types.md:4 carry the same sentence.
Documentation and tests
13. Example uses an unordered Dict, so written key order is reversed — Low
The worked example builds the TaggedMapping payload with Dict("value" => ..., "unit" => ...). Running it verbatim gives keys ["unit", "value"] (hash order) and the written YAML has unit: "s" before value: 1200.0. The tests, write_file, and the TaggedMapping docstring all standardise on OrderedDict for deterministic key order, and neither custom-types.md nor the README mentions order, so users copying the example get nondeterministic layouts from a package that went to some length to preserve order.
using ASDF, OrderedCollections
struct Measurement
value::Float64
unit::String
end
# As written in docs/src/custom-types.md, with a plain Dict:
ASDF.to_tree(m::Measurement, ::ASDF.WriteContext) =
ASDF.TaggedMapping("tag:example.org/measurement-1.0.0", Dict("value" => m.value, "unit" => m.unit))
collect(keys(ASDF.to_tree(Measurement(1200.0, "s"))))
# Any["unit", "value"] hash order, reversed from the source
f = tempname()
ASDF.write_file(f, OrderedDict("exposure" => Measurement(1200.0, "s")))
print(read(f, String))
# ...
# exposure:
# !<tag:example.org/measurement-1.0.0>
# unit: "s"
# value: 1200.0
# ...Suggested fix. Use OrderedDict in the example. Also fix the inaccurate half of the comment at src/ASDF.jl:1869: blocks are collected during YAML.write (line 1781), not in a pre-write pass.
14. Abstract-type fixtures and save block test Julia dispatch, not ASDF — Low
test/test-write-converters.jl:60
AbstractWriteValue/SpecialWriteValueand the asserts at lines 59–60 only check that Julia picks the more specific method. ASDF has no dispatch registry;_convert_treejust callsto_tree(value, context). Line 59 duplicates line 45 and line 60 duplicates line 107.- Lines 38–40 ("shallow") call the test's own two-argument method, not ASDF code.
- The
saveblock at lines 114–118 re-tests a one-line delegation already covered bytest-write.jl:20/62/69; only the value assert at line 118 is new and can move into thewrite_fileblock. - Lines 111–112 duplicate the non-mutation check at line 76, since
OrderedDict{Any, Any}(doc)["roman"] === doc["roman"]. - The new
tag:example.org/write/...namespace diverges from the existingtag:example.org:mylib/...fixtures.
Collapsing to one WriteValue, dropping the abstract type, and deleting lines 114–118 and 111–112 loses no ASDF coverage (38/38 pass either way).
15. Tagged* docstring never says the tag must be the full URI — Low (plausible)
The shared TaggedMapping/TaggedSequence/TaggedScalar docstring is now rendered as the API reference for to_tree return values (api.md:12–18, custom-types.md:54–58), but still describes them only as load-side products and mentions tags in !core/... shorthand.
using ASDF, OrderedCollections
f = tempname()
sw = OrderedDict("name" => "x", "version" => "1")
ASDF.write_file(f, OrderedDict("sw" => ASDF.TaggedMapping("!core/software-1.0.0", sw)))
# the file contains !<!core/software-1.0.0> because shorthand_tag only rewrites the tag:stsci.edu:asdf/ prefix
ASDF.load_file(f)
# ERROR: could not determine a constructor for the tag '!core/software-1.0.0' at line 9, column 2
ASDF.load_file(f; extensions = true)["sw"].tag
# "!core/software-1.0.0" loaded as an unrecognized tag, with a warning
ASDF.write_file(f, OrderedDict("sw" => ASDF.TaggedMapping("tag:stsci.edu:asdf/core/software-1.0.0", sw)))
# the file contains !core/software-1.0.0
ASDF.load_file(f)["sw"].tag
# "tag:stsci.edu:asdf/core/software-1.0.0" round-trips as the core tagOnly "tag:stsci.edu:asdf/core/software-1.0.0" round-trips as the core tag. The mechanism is pre-existing and every example in the diff uses the full-URI form.
Suggested fix. One sentence in the docstring: the tag field is the full tag URI; the writer emits the !core/ shorthand itself.
Not reported
- Unasserted test warnings. The new tests load files with
extensions = trueand emit "unrecognized tag" warnings without@test_logs. This follows the repository's existing convention, so it was dropped. - Two candidate findings (an
IdDictfalse-positive on equal isbits values, andNDArraybeing walked bymap) were investigated and found not to be defects: only hooked values and containers are tracked, andNDArrayis not anAbstractArray.
Attached version: code-review-46c4bde.md
I've verified each code example by hand and would be happy to continue to iterate on this with you if you agree with the general direction.
|
Yeah I'm not that great at this type of thing either. There's a lot to look at here so it'll take me a while to go through but I appreciate the effort! P.S. The write-up mentions Julia v1.13, are you using 1.13 now? I haven't even tried it yet |
|
Oh, yea, it already landed in |
|
I revised the implementation around the simpler one-argument A minor annoyance; compatible multidimensional numerical arrays nested in converter output are not automatically converted to |
|
Thanks, Chris! I just pushed a commit (563a977) addressing the remaining items brought up in my next round of reviews by the 🤖: DetailsCode review:
|
| # | Severity | Location | Finding |
|---|---|---|---|
| 1 | High | src/ASDF.jl:1176 |
which runs twice per node; the pre-write pass is 15–1800x slower on Julia 1.12 and 1.13 |
| 2 | High | src/ASDF.jl:1187 |
Date and DateTime are rejected; files with YAML timestamps cannot be re-saved |
| 3 | Medium | src/ASDF.jl:1179 |
Same-type guard rejects an unwrapping hook and misses alternating hooks, which hang |
| 4 | Medium | src/ASDF.jl:1215 |
Re-saving a loaded chunked array fails with a misdirecting hint |
| 5 | Medium | src/ASDF.jl:1205 |
Symbol keys and values are rejected, though named-tuple keys are converted |
| 6 | Low | src/ASDF.jl:1246 |
Three dead yaml_compliant methods and an untested nothing leaf: the four Codecov misses |
| 7 | Low | src/ASDF.jl:1187 |
Runic fails on eight whitespace-only lines |
| 8 | Low | src/ASDF.jl:1186 |
Redundant leaf methods |
| 9 | Low | docs/src/index.md:7 |
Docs promise scalar Complex; PR description is stale; error hint recommends type piracy |
Status of the 15 first-round findings
| # | First-round finding | Status in 50985e6 |
|---|---|---|
| 1 | Hook output never re-dispatched | Fixed |
| 2 | Unannotated context is ambiguous |
Gone with the context argument |
| 3 | Self-tagging container subtype | Fixed |
| 4 | Wrong-arity hook silently ignored | Gone; one arity |
| 5 | Tuple/NamedTuple/Pair/Set/unhooked leaves |
Tuples and named tuples convert; the rest raise |
| 6 | N-d arrays written as text | Rejected, pointing at NDArrayWrapper |
| 7 | Two full-tree passes | One pass, but see finding 1 |
| 8 | Public WriteContext with private state |
Removed |
| 9 | Recursive hook on cyclic input | Documented |
| 10 | Mapping keys not converted | Keys validated |
| 11 | Guard spread over five sites | One guard |
| 12 | Hand-maintained docs Filter |
Removed |
| 13 | Unordered Dict in the example |
OrderedDict |
| 14 | Fixtures testing Julia dispatch | Simplified |
| 15 | Tagged* docstring silent on full URI |
Sentence added |
1. which on every node — High
When a hook returns its input, _convert_tree calls which twice to check whether a package method exists. Every plain scalar and container takes that branch. The pair costs about 0.3 µs on Julia 1.10 but about 8 µs and 2.6 KiB on 1.12 and 1.13, so CI on the minimum version did not notice.
using ASDF, OrderedCollections
doc = OrderedDict("v" => collect(1:10^6))
f = tempname()
ASDF.write_file(f, doc) # warm up
@elapsed ASDF.write_file(f, doc)
# 6.24 on 50985e6, Julia 1.13.1
# 0.20 on main, Julia 1.13.1
# 0.46 on 50985e6, Julia 1.10.12Pre-write pass only, best of five:
| Document | main 1.13 |
50985e6 1.13 |
50985e6 1.12 |
50985e6 1.10 |
563a977 1.13 |
|---|---|---|---|---|---|
| 2000 mappings × 50 scalars | 49 ms | 738 ms | 823 ms | 68 ms | 29 ms |
10⁶ Int vector |
3.6 ms | 6463 ms | 6173 ms | 288 ms | 3.8 ms |
10⁶ Float64 vector |
377 ms | 8223 ms | 8895 ms | 991 ms | 477 ms |
10⁵ String vector |
0.6 ms | 707 ms | 651 ms | 39 ms | 0.5 ms |
Removing which alone gives 48 ms and 145 ms for the first two rows. The rest is the IdSet push and delete on every scalar leaf, which our first-round sketch also had.
which also reads the latest world while the hook call uses the caller's, so a hook defined after the caller started produces a wrong message:
using ASDF, OrderedCollections
struct Late; x::Int; end
function define_then_convert()
@eval ASDF.to_tree(l::Late) = OrderedDict("x" => l.x)
return ASDF._convert_tree(Late(1))
end
define_then_convert()
# ERROR: ArgumentError: ASDF.to_tree(::Late) must return a supported ASDF tree node, not another LateFix. Drop the reflection; a hook returning its own unsupported input falls through to the existing "not supported" error. Let plain scalars skip the guard: value isa Union{Nothing, Bool, Int, Float64, String} && return _convert_tree_children(value, active). Those types cannot carry a hook without type piracy.
2. Date and DateTime rejected — High
YAML.jl loads timestamps as Date and DateTime, and on main they round-trip. Python asdf writes a time: timestamp into every history entry, so real files load but cannot be re-saved.
using ASDF
src = tempname() * ".asdf"
write(src, """
#ASDF 1.0.0
#ASDF_STANDARD 1.6.0
%YAML 1.1
%TAG ! tag:stsci.edu:asdf/
--- !core/asdf-1.1.0
history:
entries:
- !core/history_entry-1.0.0
description: calibrated
time: 2024-03-05 06:07:08
observation_date: 2024-03-05
...
""")
af = ASDF.load_file(src; extensions = true)
ASDF.write_file(tempname() * ".asdf", af.metadata)
# main: succeeds; both values reload as DateTime and Date
# 50985e6: ERROR: ArgumentError: value of type Dates.DateTime is not supported by the ASDF writer; define ASDF.to_tree(::Dates.DateTime)Fix. Add Date and DateTime to the leaf union. This needs a Dates dependency, which YAML.jl already loads. Dates.Time stays out: 06:07:08 is a sexagesimal integer in YAML 1.1.
3. Same-type guard — Medium
typeof(converted) === typeof(value) rejects a hook that peels one layer off a nested wrapper:
using ASDF, OrderedCollections
struct Boxed; value::Any; end
ASDF.to_tree(b::Boxed) = b.value
ASDF.write_file(tempname(), OrderedDict("x" => Boxed(Boxed(1))))
# ERROR: ArgumentError: ASDF.to_tree(::Boxed) must return a supported ASDF tree node, not another BoxedIt misses two hooks that trade fresh objects. Run this under a timeout:
using ASDF
mutable struct Ping; n::Int; end
mutable struct Pong; n::Int; end
ASDF.to_tree(p::Ping) = Pong(p.n)
ASDF.to_tree(p::Pong) = Ping(p.n)
ASDF._convert_tree(Ping(1))
# Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
# Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
# ... then silence; still running when killed after 120 sFix. Bound the size of active instead: length(active) < 1000 || throw(ArgumentError(...)). Boxed(Boxed(1)) converts, and both runaway cases raise an ArgumentError. A document nested 1000 deep still converts in a spawned task on 1.10 and 1.13.
4. Chunked arrays cannot be re-saved — Medium
load_file produces ChunkedNDArray, which is not on the allow-list. main silently wrote its show text, so an error is better, but the hint points at an ASDF.jl type.
using ASDF
af = ASDF.load_file(joinpath(pkgdir(ASDF), "test", "data", "chunking.asdf"))
ASDF.write_file(tempname() * ".asdf", af.metadata)
# 50985e6: ERROR: ArgumentError: value of type ASDF.ChunkedNDArray is not supported by the ASDF writer; define ASDF.to_tree(::ASDF.ChunkedNDArray)
# main: succeeds; the value reloads as the String "ASDF.ChunkedNDArray([4, 4], \"float16\", ..."Fix. Materialise it, as YAML._print(::NDArray) does for plain loaded arrays: _convert_tree_children(value::ChunkedNDArray, active) = NDArrayWrapper(value[]; compression = C_None). The data round-trips as one contiguous block; chunking is not preserved. This is a judgment call, so say if you would rather raise a dedicated error.
5. Symbols rejected — Medium
Named-tuple keys become strings, but plain symbols are refused:
using ASDF, OrderedCollections
f = tempname()
ASDF.write_file(f, OrderedDict(:exposure => 1200.0))
# ERROR: ArgumentError: ASDF mapping key :exposure has unsupported type Symbol; keys must be booleans, integers, or strings
ASDF.write_file(f, OrderedDict("unit" => :s))
# ERROR: ArgumentError: value of type Symbol is not supported by the ASDF writer; define ASDF.to_tree(::Symbol)
ASDF.write_file(f, OrderedDict("meta" => (exposure = 1200.0,))) # acceptedFix. Write a symbol as its string, for keys and values. YAML.jl then quotes it, which also fixes two main quirks: Symbol("a: b") produced an unparseable file and Symbol("1e3") reloaded as 1000.0. Also a judgment call; the alternative is to document that symbols are excluded.
6. Dead code and an untested leaf — Low
Codecov's four misses:
- 1246–1248:
yaml_compliantforTaggedMapping,TaggedSequenceandAbstractDict.write_fileno longer calls it on the document; the only caller left passes inline array slices. - 1186: the
Nothingleaf. No test writes anothing.
Fix. Delete the three methods, correct the comment above them, and add a nothing to the round-trip test.
7. Runic — Low
Eight whitespace-only lines: missing spaces after commas in Union{Bool,Integer,AbstractString} and OrderedDict{Any,Any} (source lines 1187, 1191, 1203, 1205), and around = in four test-file lines.
8. Redundant leaf methods — Low
Bool <: Integer, soBoolis redundant in both unions.- The
Nothingmethod can join the union. TaggedScalar <: AbstractString, so the union method already returns it; the dedicated method only rebuilds it around a walk of itsString.
9. Documentation — Low
docs/src/index.md:7listsComplexas supported out of the box. A scalar1.0 + 2.0imnow raises; onmainit was written as a string. Only complex arrays work.- The fallback error says "define
ASDF.to_tree(::T)", which forSet,PairorCharis type piracy. - The custom-types page does not list dates or symbols.
- The PR description still says
to_tree(value, context). Not changed by the commit.
Fix. Qualify the Complex claim, reword the error to "convert it to a supported value or, for a type you own, define ASDF.to_tree(::T)", and update the contract list.
Fixes
563a977 fixes findings 1–9, apart from the PR description. Verified on that commit:
| Check | Result |
|---|---|
Pkg.test() on Julia 1.10.12 and 1.13.1, including Aqua |
350 of 350 pass |
runic --check |
clean |
Coverage of src/ASDF.jl |
complete, apart from the COV_EXCL_LINE assert |
| Documenter build of the home, custom-types and API pages | builds |
| CI on the pushed head | all 14 checks pass, including Runic and both Codecov gates |
New tests: a self-tagging mapping, a symbol key and value, a Date, a DateTime and a nothing in the round-trip document, and a chunked re-save.
The change is 7 files, +69 / −32; see the commit for the diff.
On automatic NDArrayWrapper conversion
_convert_tree_children(value::AbstractArray{<:Number}, active) = NDArrayWrapper(value) worked in a quick test on 1.13; vectors still take the AbstractVector method and stay YAML sequences. I would keep it out of this PR: it picks block storage and compression silently, while the current error says exactly what to write. It can be added later without breaking anything.
Not reported
All identical on main:
Float32values such as1.0f10are written as1.0f10, which a YAML 1.1 parser reads as a string.BigIntvalues write digits YAML.jl cannot parse back.- The docs example's
save("custom-types.asdf", document)writes into the current directory.
This branch has not been deployed
2 participants
This adds a public write-conversion API (
to_tree) so arbitrary Julia objects can be embedded in ASDF documents.Packages extend
ASDF.to_tree(value, context), while ASDF.jl recursively converts returned nodes, preserves tags and ordering, detects cycles, and routes arrays through the existing block writer.This establishes the write-side foundation for future extension support. Unknown tags can already be loaded as generic tagged nodes with
extensions = true; read-side object reconstruction, schema validation, and extension provenance can build on this protocol later.