Skip to content

feat(serve): load published filings from the TAVI first; keep INF precision - #72

Merged
jfrench9 merged 2 commits into
mainfrom
feature/published-tavi-load
Sep 24, 2026
Merged

jfrench9 merged 2 commits into
mainfrom
feature/published-tavi-load

Conversation

@jfrench9

Copy link
Copy Markdown
Member

Summary

The follow-up to #71: load_filing now reads a published filing from its TAVI model first, and a TAVI-loaded fact keeps its precision. These two commits were meant for #71 but reached its branch after it merged.

Changes

  • fix(tavi): read INF precision and abstract members back (deserialize/tavi.py)
    • A numeric fact with no decimals reads as INF. The draft leaves the property out for an infinitely precise value; read as unknown, an exact fact lost to its own rounded duplicate. JPM's shares issued at 2024-12-31 read 4,104,900,000 instead of 4,104,933,895.
    • Domains and members read back abstract: TAVI makes them objects of their own, so no fact reports against one.
  • feat(serve): load a published filing from its TAVI model first (serve/session.py)
    • PublishedFiling carries tavi_url beside holon_url (now optional), and model_urls orders them TAVI first. A manifest or catalog entry that lists only a TAVI is still a published filing.
    • _load_published tries each in order and falls back to the holon when the TAVI cannot be fetched or read. The TAVI carries its text blocks inline, so no fragments are fetched after it.
  • Tests: INF precision on read; a TAVI-only manifest; the TAVI-first load; the fallback when the TAVI is unreachable.

Output Impact

INTERNAL for every emitted file; the writers are untouched. What load_filing answers from changes for a published filing: the TAVI instead of the holon. Measured against the holon load of the same filings from the live CDN:

NVDA 10-K JPM 10-K
Load time, TAVI / holon 1.0s / 1.7s 3.1s / 5.5s
disclosures identical identical
information_block on dimensional sections, identical 32 / 36 123 / 128

describe_filing differs only in counts, and there the TAVI is the faithful one: it carries every fact the filing tags (NVDA 1,305, which is what xbrlkit's own parse of the filing counts), where the holon collapses repeated tags to 1,219. The remaining information_block differences are orderings, a suffixed duplicate role the holon lists as merged, and the default member of us-gaap:FairValueByMeasurementBasisAxis, which a cube cannot carry.

Testing

  • just test-all: 550 passed, 2 skipped; ruff, format and basedpyright clean.
  • The figures above: the MCP tools over a FilingSession load from public.robosystems.ai, TAVI first against holon only.

A TAVI numeric fact with no decimals is infinitely precise (the fact value
object leaves the property out for INF), but the reader read it as unknown,
so an exact fact lost to a rounded duplicate of itself: JPM's shares issued
at 2024-12-31 read 4,104,900,000 instead of 4,104,933,895.

Domains and members are objects of their own in TAVI, not concepts, so no
fact can report against one; they now read back abstract, as the parse has
them.
The public catalog lists each filing's tavi.json beside its holon.
PublishedFiling carries both (holon_url is now optional, and a filing
published as a TAVI alone is still a published filing), and the load tries
the TAVI first and falls back to the holon when it cannot be fetched or read.

The TAVI carries its text blocks inline, so there is nothing to fetch after
it, and it loads in about half the time: NVDA 1.0s against 1.7s, JPM 3.1s
against 5.5s, from the live CDN. Its fact count matches the parse of the
filing (NVDA 1,305), where the holon collapses repeated tags.
@jfrench9

Copy link
Copy Markdown
Member Author

@claude please review this PR

@claude

claude Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Claude finished @jfrench9's task in 2m 17s —— View job


Reviewing PR #72

  • Gather PR metadata, diff, and existing review feedback
  • Read repo conventions (xbrlkit/serve/README.md, deserialize/)
  • Review xbrlkit/deserialize/tavi.py changes
  • Review xbrlkit/serve/session.py changes
  • Review test changes
  • Post review summary

PR Summary

Title: feat(serve): load published filings from the TAVI first; keep INF precision
Author: jfrench9 | Branch: feature/published-tavi-load → main
Changes: +127 / -22 across 4 files

Two focused fixes originally meant for #71 that landed after it merged: (1) deserialize/tavi.py now reads a missing decimals on a numeric fact as INF instead of unknown, and reads domain/member concepts back as abstract; (2) serve/session.py lets PublishedFiling carry an optional tavi_url alongside holon_url, tries the TAVI first via a new model_urls ordering, and falls back to the holon on fetch/read failure.

Existing Review Feedback

No human or bot reviews, inline comments, or check runs were present in the pre-fetched context (this is typical for this repo — formal GitHub reviews aren't usually used here). I was unable to reach gh/network tools in this sandbox to pull live CI status, so I read the diff directly from the local git history (git diff origin/main HEAD) instead.

My Review

Issues (should fix before merge)

None found. The change is small, well-scoped, and consistent with the existing patterns in the file.

Suggestions (non-blocking improvements)

  1. No dedicated test for the abstract-members fix. xbrlkit/deserialize/tavi.py now passes is_abstract=True when building domain/member concepts (lines ~354-368), but tests/test_deserialize.py only gained a new test for the INF-precision fix (test_tavi_reads_an_exact_fact_as_infinitely_precise). The existing test_round_trip_keeps_the_concept_facts (line 704) asserts is_domain_member is True for us-gaap:NorthAmerica but not is_abstract, so nothing in the suite would fail if that part of the fix regressed. A one-line addition (assert got.concepts["us-gaap:NorthAmerica"].is_abstract is True) would close the gap and matches this repo's convention of a fixture-backed regression test per fix.
  2. xbrlkit/serve/session.py:1017 — into = self._tmp / (published.accession or Path(urls[0].split("?", 1)[0]).stem) indexes urls[0] unconditionally. This is safe today because _published_from (the only production constructor) guarantees at least one of tavi_url/holon_url is set, but it's an implicit invariant enforced far from this call site — worth a short comment noting the guarantee, or an assertion, so a future direct PublishedFiling(...) construction doesn't hit an IndexError here.

Questions

None — the PR description's benchmark numbers and rationale (TAVI is more faithful to the filer's own tag count, and faster) are convincing, and the fallback-to-holon path is directly tested (test_an_unreachable_tavi_falls_back_to_the_holon).

Verified in the diff

  • PublishedFiling.holon_url is now Optional, but all construction sites (session.py:304, both test cases) use keyword args, so no positional-argument breakage.
  • The fetch loop in _load_published (session.py:1025-1034) correctly re-raises on the last URL's failure and only falls back to the next candidate on requests.RequestException or SourceError.
  • _inline_external_text still runs for both TAVI and holon loads — a no-op for TAVI since it carries text inline, so no behavior regression for the holon fragment-fetch path.
  • The INF fix only fires when decimals is absent and the fact is numeric with a value (unit is not None and value_str is not None), so it won't misfire on nil facts or non-numeric facts.

Verdict

APPROVE — the changes are correct, narrowly scoped, and match the fallback/precision behavior described in the PR body. The one suggestion (test coverage for the abstract-member fix) is non-blocking and could be a fast follow-up.

@jfrench9
jfrench9 merged commit 2484401 into main Sep 24, 2026
4 checks passed
@jfrench9
jfrench9 deleted the feature/published-tavi-load branch September 24, 2026 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant