Python context layer: pin dictionary prose as retrievable chunks - #284
Open
jat255 wants to merge 1 commit into
Open
Python context layer: pin dictionary prose as retrievable chunks#284jat255 wants to merge 1 commit into
jat255 wants to merge 1 commit into
Conversation
The dictionary already turns its prose into retrieval chunks in both packages, but nothing held the two to the same answer. Add a dictionary_context_chunks section to the shared context_layer fixture and a runner in each suite, driving each package's own reader from the same parsed shape. The fixture exposed one divergence: R named its table chunks after their tables, because vapply() over names() carries them, and those names rode into the layer's documents. Chunks are text, so drop the names. Neither call site read them. Governed definitions carry no case. The two packages compile them at different constructors, so they disagree on whether a dictionary that is not attached to a source has any; definitions.json pins the chunk text. Python needs no new chunking function: the dictionary owns it as DataDictionary.context_chunks(). What was missing was the guard for a source with no dictionary at all, which is what _dictionary_chunks() adds and what folding source prose into a layer needs next.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third of four PRs for M4, the Python context layer (kata
vp7e). Stacked on #241; #285 folds the chunks into an agent's layer on top.Both packages already turn a dictionary's prose into retrieval chunks, and nothing held them to the same answer.
tests/shared/context_layer.jsongains adictionary_context_chunkssection and both suites run it against their own reader: dataset details, then tables in declaration order, then glossary terms, then governed definitions, with a table that has no prose contributing nothing and column-level content staying out.Python needs no new chunking function. The dictionary already owns it as
DataDictionary.context_chunks(), which is where it belongs now that it owns all three of its channels. The milestone plan expected adictionary_context_chunks()in_context_layer.py; writing one would have been a second implementation of the same thing. What was actually missing is the guard for a source carrying no dictionary at all, which is_dictionary_chunks()and is what the next PR needs.Governed definitions get no fixture case, deliberately. Both packages render the chunk text the same way and
definitions.jsonpins it, but they compile definitions at different constructors, so they disagree on whether a dictionary not yet attached to a source has any. A case here would pin that disagreement rather than the contract.R changes
One functional change in
pkg-r/R/context-layer.R, found by the new fixture rather than by reading:dictionary_context_chunks()returned a named character vector, and now returns an unnamed one.The names came from
vapply(names(dictionary$tables), ...), which labels each table's chunk with its table name. Becausec()keeps names once any element has them, the whole returned vector was named, with""for the dataset details and the glossary entries. Those names then rode intoaugment_context_layer()and became names on the layer'sdocsvector. Nothing read them, so nothing behaved differently, but the R side was carrying metadata the Python side has no way to carry, which made the shared fixture unsatisfiable for both suites at once.unname()on the filtered result is the whole change. Chunk text and chunk order are untouched.Blast radius is two call sites, and neither reads names.
augment_context_layer()concatenates the vector into a layer's documents. The one test caller,test-definitions.R:401, usesgrepl()and positional subsetting. A named-vector consumer would have had to index by table name, and no code does.Why it is safe:
unname()cannot change length, order, or element text, so the only observable difference is the absence ofnames(). I checked both callers by reading them rather than assuming, and rantest-context-layer.R(33 passing),test-data-dictionary.R(68 passing, 3 skipped on CRAN),test-definitions.R(114 passing) andtest-citations.R(84 passing), which are the files that touch this function oraugment_context_layer(). I did not run the full R suite: it currently hangs for about ten minutes onrun_r's uncancelledlatercallbacks, which #268 fixes.Worth your scrutiny: the new runner in
test-context-layer.Rbuilds each case withnew_data_dictionary(case$dictionary), so the fixture drives the real reader and not a shape adapter written for the test. That relies onnew_data_dictionary()accepting the same raw parsed shape the Python model validates, which it does because both read the authoreddata-dict.yaml. If that stops being true the runner is where it will show up.No hand-written R tests were deleted; the new test is additive.
pkg-r/tests/testthat/fixtures/shared/context_layer.jsonis generated byscripts/sync-shared-fixtures.shand needs no review.