Select a spool on the facts an inventory states - #877
Conversation
Every policy knob now draws from one word set: raise/warn/ignore, plus 'null', which is genuinely an action rather than a volume setting. Nothing is released, so no compatibility spelling is kept.
Patch.enrich and Spool.enrich take the same arguments, and describing them twice is how the two drift. Their parameter text moves into shared constants which both compose, with Spool.enrich listing the forwarded ones under Other Parameters instead of pointing at Patch.enrich.
Inventory.get_names returns them split by destination: attrs for the observing-system facts and coords for the names taking a value per channel. The attrs side is read off the models, so a field added to an acquisition or interrogator is selectable without a second list to update, and a test pins it to the reader vocabulary. The coords side comes from the inventory itself -- its CRS axes, the tracks its paths describe, and their annotation groups. The one hand-written piece is the track-name to identity-field map, which is what a bare track name means; it now also states the track vocabulary enrichment projects and annotation groups may not shadow.
Naming an acquisition or interrogator fact now filters the spool on what the inventory says, so an archive whose files never recorded their gauge length can still be selected by it. Precedence is per row: a patch which states the name is judged by the index exactly as before, and only the rows leaving it unstated are resolved -- once per epoch rather than once per patch. The inventory writes nothing into the index; the filter rewrites the contents of a new spool, so len and get_contents stay exact and no data is read. A selector means the same thing on either side of that split: the in-memory predicate is the twin of the SQL one, taking the same shapes and giving unit-bearing values the same error against these unitless columns. Naming a coordinate the inventory defines along the fiber now says so, replacing the hedge which could not tell an inventory field from a misspelled attr.
Saying what a spool does not want -- one bad tag, an instrument being serviced -- took spelling the rest of the archive as a selection. The complement is taken against select itself rather than by negating each predicate, so a keyword cannot come to mean one thing in one and something else in the other, and an attached inventory's names work here because they work there. Coordinates are refused: a range decides how much of each patch to keep rather than which patches, so its complement is a hole in the middle rather than a filter.
Hive-style directories are how a layout corrects metadata, so the path winning is deliberate. It is also what a mis-sorted archive looks like from the inside, and the two are indistinguishable once indexed. One warning per update names each overridden attr and a path to go look at. Restating what a file already says overrides nothing and stays silent.
A selector now means one thing on either side of the index/inventory split. The in-memory predicate types its values the way the index types a stored one, so a numeric range against a string attr matches nothing instead of raising and 1 no longer matches a stored True; a regex searches, as the index's residual does; and a glob is translated from SQLite's own semantics rather than handed to fnmatch, which read a negated class as its complement. A fixed membership no longer swallows the predicates composed on top of it. PatchCatalog._ordered_ids returned the membership unfiltered whenever one was set, so unselect emptied every windowed, sliced or chained spool, and inventory-backed select kept rows whose own headers contradicted the selector. The membership now filters, keeping its order -- an integer array may have arranged it. Rows with nothing to resolve with are answered rather than asserted about: a spool whose patches carry no acquisition_key, and one whose time axis holds lag rather than instants, describe no row and select none, where the latter previously picked an epoch from an offset. Also: names the accessor lists are names enrichment understands, so a bare track name now means the field the blessed map points at, and a field a component states as several values is not listed; an optional collection and an Annotated union are read for what they hold; the path-override warning compares meaning rather than spelling; and the forwarded-argument docs render as a table.
Attaching stays free, which is the whole point of it being a separate step. Every select on a spool carrying an inventory was walking the inventory twice and reading the index's name lists three times, and a name the index stated for every row was still realizing the whole relation to discover that. The names are now read once per call, the model introspection behind them is cached on the class rather than redone per track item, and which rows state a name is asked of the index, so a spool whose headers are complete never realizes anything. Measured on a 10k-patch spool: selecting on an ordinary index name with an inventory attached went from 3.7x the plain cost to 1.4x (29x to 1.4x against an inventory with 250 optical paths), and selecting on a name every patch states went from 225x to 11x. A bare None or ... selector also stays the no-op it is everywhere else rather than failing to coerce.
A row with no instant of its own no longer inherits a context: resolving at NaT holds every epoch effective, which is the whole inventory answering rather than the one entry describing the row. The in-memory attr predicate is held closer to the index's. A range now goes through the same reading, so one with no usable bounds is the error it is there rather than a selector which quietly keeps every row; a unit-bearing selector converts to whatever units the index records for the attr, so attaching an inventory can no longer turn a working selector into a UnitError; the inventory's units are rendered the way a patch states them, since the same fact stored as a quantity and as a string would never match; a wildcard crosses a newline; and a reversed character range matches its low endpoint, which is what SQLite does with it -- verified against SQLite over 27000 fuzzed patterns. The relation and the id list are aligned by patch id rather than by position. They are realized by different routes and can present different rows, which an assert turned into a crash and -O turned into a silent misalignment. unselect refuses a selection made only of no-ops, for the reason it refuses an empty one: an empty spool and an unchanged one are both readings, and it should not guess.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds inventory-derived names, batch enrichment, inventory-aware selection and unselection, typed predicate parity, membership-preserving catalog filtering, semantic conflict warnings, shared enrichment documentation, and related tests and tutorial coverage. ChangesInventory selection and enrichment
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_io/test_index/test_hive_attrs.py (1)
185-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the silence assertions to the override warning.
warnings.simplefilter("error", UserWarning)turns everyUserWarningraised during indexing into an error, not only the override warning. An unrelated warning from the index (for example the attr-clobber warning inSQLIndexBackend._apply_attr_columns) would fail these tests with a message that points at the wrong cause.The
_indexhelper added at line 411 already uses the precise pattern: record all warnings, then filter for"override attrs". Reusing that pattern here keeps the assertion about the behavior under test.♻️ Proposed change for both silence tests
- with warnings.catch_warnings(): - warnings.simplefilter("error", UserWarning) - spool = Spool.from_directory(tmp_path).update(progress=None) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + spool = Spool.from_directory(tmp_path).update(progress=None) + assert not [x for x in caught if "override attrs" in str(x.message)] assert spool.get_contents()["station"].iloc[0] == "A" spool.indexer.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_io/test_index/test_hive_attrs.py` around lines 185 - 200, Update both test_unstated_attr_is_silent and the preceding override-silence test to capture warnings and assert only that no warning matching “override attrs” is emitted, rather than converting every UserWarning into an exception. Reuse the warning-recording and message-filtering pattern already used by the _index helper, while preserving the existing spool content assertions and cleanup.dascore/io/index/backend.py (1)
1103-1122: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle the
json_eachdependency consistently.
build_sqlalso usesjson_eachwhenpatch_idsis provided. Replace both predicates with batching, or declare and validate JSON1 as a backend requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dascore/io/index/backend.py` around lines 1103 - 1122, Update attr_stated_ids and the corresponding build_sql patch_ids predicate to handle json_each consistently: either replace both JSON-based filters with the established batching approach, or declare and validate SQLite JSON1 as a required backend capability before these queries run. Keep patch ID filtering behavior unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dascore/core/spool.py`:
- Around line 857-862: Update the unselect flow around _ordered_ids and
_catalog.restrict so the positional keep mask is applied to the exact ids array
it was built from by passing ids=ids. Keep the existing mask computation and
_new_from_catalog behavior unchanged, and align this call with
_select_from_inventory.
- Around line 999-1007: Update _index_matches to distinguish an unknown
attribute name from an invalid selector: check whether name exists in the
catalog/index before calling PatchCatalog.select, return an empty int64 array
only for an unknown name, and allow InvalidSpoolQueryError from malformed
selectors such as reversed bounds to propagate. Add coverage for an attached
inventory where every patch states the name and selector (20.0, 5.0) still
raises.
---
Nitpick comments:
In `@dascore/io/index/backend.py`:
- Around line 1103-1122: Update attr_stated_ids and the corresponding build_sql
patch_ids predicate to handle json_each consistently: either replace both
JSON-based filters with the established batching approach, or declare and
validate SQLite JSON1 as a required backend capability before these queries run.
Keep patch ID filtering behavior unchanged.
In `@tests/test_io/test_index/test_hive_attrs.py`:
- Around line 185-200: Update both test_unstated_attr_is_silent and the
preceding override-silence test to capture warnings and assert only that no
warning matching “override attrs” is emitted, rather than converting every
UserWarning into an exception. Reuse the warning-recording and message-filtering
pattern already used by the _index helper, while preserving the existing spool
content assertions and cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe22dba5-2514-48cb-b7cc-04b31bf90d77
📒 Files selected for processing (15)
dascore/constants.pydascore/core/inventory.pydascore/core/spool.pydascore/io/index/backend.pydascore/io/index/catalog.pydascore/io/index/ingest.pydascore/io/index/query.pydascore/proc/inventory.pydocs/tutorial/spool.qmdscripts/_render_api.pytests/test_core/test_inventory.pytests/test_core/test_spool.pytests/test_io/test_index/test_catalog.pytests/test_io/test_index/test_hive_attrs.pytests/test_proc/test_proc_inventory.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 430b264684
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if index + 2 < size and body[index + 1] == "-": | ||
| high = body[index + 2] | ||
| out.append(re.escape(low)) | ||
| if low <= high: | ||
| out.append(f"{re.escape(low)}-{re.escape(high)}") |
There was a problem hiding this comment.
] as a literal in glob classes
For valid SQLite classes that include ] as their first member and then a range-like sequence, _class_body incorrectly treats that literal as the lower endpoint of a range. For example, SQLite evaluates '-' GLOB '[]-a]' as true, while the generated regex does not match '-' and instead admits characters in the unintended ]–a range. Consequently, the same selector can reject an inventory-supplied value while accepting an identical index-stored value; preserve the leading literal ] before parsing the remainder of the class.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1d82adb.
Reproduced exactly as described: SELECT '-' GLOB '[]-a]' is true in SQLite and the generated regex rejected it, while ^ and _ matched when they should not. The leading ] is now peeled off as a literal before the rest of the class is parsed, so a dash following it is a member rather than a range operator. Re-fuzzed against SQLite with an alphabet biased toward class punctuation (40,000 patterns, zero mismatches) and added []-a], [^]-a], []a] to the parity cases. My original fuzz used a uniform alphabet, so this shape essentially never came up.
| def _check_channel_level(self, requested, coords, known) -> None: | ||
| """Raise for a name the inventory defines along the fiber.""" | ||
| if channel_level := sorted(requested & coords - known): | ||
| msg = ( | ||
| f"{channel_level} name coordinates the attached inventory " | ||
| "defines along the fiber, which selection cannot trim to " | ||
| "yet. Enrich the patches and select on each one instead." | ||
| ) | ||
| raise InvalidSpoolQueryError(msg) |
There was a problem hiding this comment.
When an optical-path annotation group has the same name as an acquisition attribute, such as gauge_length, get_names() legitimately reports the name in both attrs and coords, but this precheck raises before the documented attribute-first namespace resolution runs. Thus even bare select(gauge_length=10) or an explicit _attrs query is rejected as an unsupported along-fiber selection, despite the acquisition supplying the scalar value. Exclude inventory attribute names from this channel-level check unless the caller explicitly selected the coordinate namespace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1d82adb.
Reproduced with an annotation group named gauge_length: get_names() reported it in both namespaces and both select(gauge_length=10.0) and select(_attrs={"gauge_length": 10.0}) were rejected as along-fiber, though the acquisition supplies the scalar. The precheck now counts inventory attr names as known, so a colliding name resolves to the attr as bare names always do, and only a name the caller put in _coords is read as the coordinate. Covered for the mapping form and both tag forms.
| if existing.kind == "bool": | ||
| return path_value.strip().lower() != str(existing.value).lower() | ||
| return str(existing.value) != path_value |
There was a problem hiding this comment.
For a file carrying a datetime-valued extra attribute, a hive segment that restates the same instant in normal text is always reported as a conflict because indexed time values are nanosecond integers and this fallback compares that integer's string to the path text. For example, an attr equal to np.datetime64('2020-01-01') under observed=2020-01-01 emits the new override warning even though the values have the same meaning, contrary to the helper's stated comparison contract. Parse time path values to nanoseconds (and similarly handle stored duration kinds) before deciding they differ.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1d82adb.
An attr equal to np.datetime64("2020-01-01") under observed=2020-01-01/ warned, because the stored value is integer nanoseconds and the fallback compared that integer as text against the path text, so a restated instant could never compare equal. _states_something_else now parses the segment with to_datetime64 / to_timedelta64 for the time and dur kinds, and treats an unparseable segment as a change. Test covers restated, different, and unparseable.
| multi.update( | ||
| f"{track}.{x}" for x in names if _is_multi_valued(item, x) | ||
| ) |
There was a problem hiding this comment.
The multi set is accumulated globally across every optical path, so one tuple-valued occurrence suppresses that qualified field for the entire inventory. If one acquisition's component records multi-wavelength loss_db while another path records a scalar loss_db, get_names().coords omits optical_components.loss_db even though the latter path can project it, causing callers to treat a valid name as unknown. Track whether a field has any scalar occurrence and exclude it only when every stated occurrence is multi-valued.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1d82adb.
The multi set was accumulated globally, so one component recording a multi-wavelength loss_db suppressed the qualified name for the entire inventory. It now records which shapes a field is stated in and drops it only where every stated occurrence is multi-valued, so a path recording a scalar keeps the name. Test asserts all three: multi-only omits it, scalar-only lists it, mixed lists it.
|
✅ Documentation built: |
Python 3.13 dedents a docstring as it compiles it, so the depth a parameter name sits at is not the same on every interpreter; the test now looks for the name on a line of its own however far it is indented. Also spells the sibling test's name the way the typo checker reads it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_proc/test_proc_inventory.py`:
- Around line 825-827: Update the forwarded-parameter name assertion in the
relevant test to search both spool_doc and patch_doc, preserving the existing
regex and name validation so removals from either Patch.enrich.__doc__ or the
spool doc fail the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 87c870bc-2c11-4b6c-ab4c-5a0f196f3a4a
📒 Files selected for processing (1)
tests/test_proc/test_proc_inventory.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #877 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 177 177
Lines 20153 20515 +362
==========================================
+ Hits 20153 20515 +362
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
unselect restricts against the ids its mask was built from, as inventory-backed select already does; reading them twice is two chances to get a different list on a catalog whose syncer can invalidate between the calls. _index_matches asks whether the index knows the name rather than trying it and catching the refusal. The catch was narrow only by accident of timing -- a malformed selector is rejected when the rows are realized, outside the try -- and a name check says what is meant. Test hygiene: the silence assertions look for the override warning rather than turning every UserWarning into an error.
The refusal was justified as though a coordinate complement were ill-defined. It is not: selecting on a coordinate trims each patch as well as dropping the ones which miss the range, so the complement is every patch cut into the pieces outside it -- one row becoming two. That is subdivision, which unselect cannot do yet and conform_to_inventory will bring; the old wording argued against a feature that is scheduled.
A ']' opening a glob class is a plain member, so the dash which may follow it is one too: '[]-a]' matched the wrong set of characters, and the same selector could reject an inventory value while accepting an identical stored one. Verified against SQLite over 40000 more patterns. An annotation group may be named after an acquisition field, and the channel-level precheck rejected the field along with the group. Bare names resolve to attrs first, so only a name the caller put in _coords is read as the coordinate. A track field is dropped only where nothing states it as one value: one path recording a multi-wavelength loss was silencing the name for every other path, including those recording a scalar. A time is stored as integer nanoseconds, which no path segment spells that way, so the override warning called every restated instant a disagreement; the segment is now read as an instant before comparing. The forwarded-argument test checks both docstrings rather than only the spool's, so a name cannot go missing from Patch.enrich unnoticed.
| # The typed tracks of an optical path, each mapped to the field a bare use | ||
| # of its name means: `coupling="trench"` asks about coupling_type, and a | ||
| # geometry or component is asked about by name. Every other field of a | ||
| # track is reached by its qualified name (`coupling.medium`). This is the | ||
| # one place the mapping is written down; a test pins each entry to the | ||
| # model it names. |
There was a problem hiding this comment.
Too verbose, and the test sounds like a bad idea.
There was a problem hiding this comment.
Agreed on both counts — reworked in c5c5b44.
The comment is down to what a bare track name means. The map itself is gone as a hand-written thing: each track model now declares its own identity field beside the field it names,
class CouplingCondition(_IntervalModel):
_identity_field: ClassVar[str] = "coupling_type"
coupling_type: CouplingType = Field(...)and TRACK_IDENTITY_FIELDS is read off those declarations by walking OpticalPaths track fields. ClassVar keeps it out of model_fields, the dump, and the round trip.
|
|
||
| # The observing-system facts, read off the models rather than listed, so a | ||
| # field added to either automatically becomes something an inventory can | ||
| # contribute. Pinned to INVENTORY_ATTRS by a test: the two are one |
There was a problem hiding this comment.
Better way to pin together than just a test? A test that enforces structure has a smell.
There was a problem hiding this comment.
You were right that it smelled. The old test only asserted that a field of that name existed on the model, so renaming coupling_type while any field called name survived elsewhere would still have passed — it enforced structure and caught very little.
Reworked in c5c5b44: with the map derived from the models there is no second copy to pin. What remains is a check inside the derivation itself rather than a test —
assert field in model.model_fields, (model, field)so a declaration naming a field the model does not have fails where the map is built, on import, in every environment. That is stronger than the test was, and it is the defensive-assert form rather than a structural test.
Behaviour is covered behaviourally instead: a bare track name must give what its qualified identity field gives (coupling and coupling.coupling_type agree), and one test asserts the guard fires.
Net 29 lines removed, 42 added, and the constant is no longer something to keep in step with anything.
The track-name to identity-field map was written down by hand and held to the models by a test, which is a test enforcing structure rather than behaviour: it only checked that a field of that name existed somewhere on the model, so a rename which left any 'name' field behind would pass. Each track model now declares its identity beside the field it names, and the map is read off those declarations. There is nothing to keep in step, the comment shrinks to what a bare track name means, and the derivation asserts that a declaration names a real field -- so a bad one fails where it is built rather than resolving to NaN somewhere later. The behaviour is already covered: a bare track name has to give what its qualified identity field gives, which the round-trip test asserts.
coderabbitai
Bot
removed
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dascore/core/inventory.py`:
- Line 1009: Replace the assertion validating field membership in
model.model_fields with an explicit exception raise, ensuring invalid
_identity_field values cannot be registered in TRACK_IDENTITY_FIELDS even when
Python runs with optimization enabled. Preserve the existing validation context
and include the model and field details in the raised error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f6d70c7-49a0-4748-9a94-48593b7a362f
📒 Files selected for processing (2)
dascore/core/inventory.pytests/test_core/test_inventory.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_core/test_inventory.py
| continue | ||
| # The model names one of its own fields, or the map it builds | ||
| # would point at nothing. | ||
| assert field in model.model_fields, (model, field) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python -O - <<'PY'
assert False, "This assertion should be removed in optimized mode."
print("Assertions are disabled under -O.")
PYRepository: DASDAE/dascore
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target function and validation ---'
sed -n '960,1030p' dascore/core/inventory.py
printf '%s\n' '--- related identity-field declarations and mapping uses ---'
rg -n -C 3 '_identity_field|TRACK_IDENTITY_FIELDS|_track_identity_fields' dascore testsRepository: DASDAE/dascore
Length of output: 9370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- track enrichment and selection behavior ---'
sed -n '600,650p' dascore/proc/inventory.py
sed -n '1985,2055p' dascore/core/inventory.py
printf '%s\n' '--- existing validation and behavioral tests ---'
sed -n '1785,1820p' tests/test_core/test_inventory.py
sed -n '1875,1905p' tests/test_core/test_inventory.py
printf '%s\n' '--- optimized assertion behavior ---'
python3 - <<'PY'
source = """
def validate(field, fields):
assert field in fields, ("model", field)
return {"track": field}
"""
normal = compile(source, "<normal>", "exec", optimize=0)
optimized = compile(source, "<optimized>", "exec", optimize=1)
for label, code in (("normal", normal), ("optimized", optimized)):
namespace = {}
exec(code, namespace)
try:
result = namespace["validate"]("not_a_field", {"name"})
except AssertionError as exc:
print(f"{label}: raises AssertionError: {exc}")
else:
print(f"{label}: returns {result!r}")
PYRepository: DASDAE/dascore
Length of output: 9034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- track coordinate lookup implementation ---'
rg -n -C 8 'def _get_track_coord|def _value_field_names|def _value_shape' dascore
printf '%s\n' '--- optimized import-time validation context ---'
sed -n '1745,1775p' dascore/core/inventory.pyRepository: DASDAE/dascore
Length of output: 4593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '566,600p' dascore/proc/inventory.pyRepository: DASDAE/dascore
Length of output: 1782
Make identity-field validation explicit.
At dascore/core/inventory.py:1009, assert is removed under Python -O. An invalid _identity_field can then enter TRACK_IDENTITY_FIELDS, and track lookup treats the missing field as an absent coordinate. Replace the assertion with an explicit raise.
Proposed fix
- assert field in model.model_fields, (model, field)
+ if field not in model.model_fields:
+ raise AssertionError((model, field))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert field in model.model_fields, (model, field) | |
| if field not in model.model_fields: | |
| raise AssertionError((model, field)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dascore/core/inventory.py` at line 1009, Replace the assertion validating
field membership in model.model_fields with an explicit exception raise,
ensuring invalid _identity_field values cannot be registered in
TRACK_IDENTITY_FIELDS even when Python runs with optimization enabled. Preserve
the existing validation context and include the model and field details in the
raised error.
1 participant
Description
The first of three PRs implementing inventory-backed spool selection (#857). This one covers the acquisition-level half — whole-patch metadata — and the pieces the other two need. Selecting on the coordinates an inventory defines along the fiber (coupling, geometry, annotation groups) is PR (c), and
conform_to_inventory, which owns row subdivision, is PR (b).Selecting on what the inventory states.
spool.select(gauge_length=10.0)andspool.select(**{"interrogator.model": "FI-1"})now filter an archive whose files never recorded either. Precedence is per row: a patch which states the name is judged by the index exactly as it would be without an inventory, and only the rows leaving it unstated are resolved — once per(acquisition_key, epoch)rather than once per patch. A spool whose headers are complete never consults the inventory at all, and which rows state a name is asked of the index rather than read off a realized relation, so that spool is never realized either.A patch the inventory does not describe, or describes twice because it straddles an epoch boundary, is simply not selected. Select is a filter, and a patch with no single answer is no more selected than one lacking the attr entirely — the one deliberate exception to loud-by-default, because warning here would make composed selects noisy on intentionally partial inventories. Subdividing the straddlers is
conform_to_inventory's job.The inventory never writes into the SQLite index. The filter rewrites the contents of a new spool, so
lenandget_contentsstay exact, no data is read, and swapping inventories cannot leave a stale derived value behind. The one route by which inventory facts become index facts is still the ordinary round trip: enrich, write, rescan.One selector, one meaning.
evaluate_attr_predicateis the in-memory twin of the SQLbuild_attr_clauseand is held to it: it types its values the way the index types a stored one (so a numeric range against a string attr matches nothing rather than raising, and1does not match a storedTrue), a regexsearches as the index's residual does, and a glob is translated from SQLite's own semantics rather than handed tofnmatch—[!x]and[^x]are each other's complement in the two, which would have made one pattern select opposite halves of a spool depending on which side answered it.Inventory.get_namesreturns the names an inventory could contribute to a patch, split intoattrsandcoordsthe wayPatch.enrichand select's_attrs/_coordsalready split them. The attrs side is read off the pydantic models, so a field added to an acquisition or interrogator is selectable without a second list to maintain, and a test pins it toINVENTORY_ATTRS— the two are one vocabulary, and a new field has to reach the readers as well. The coords side comes from the inventory itself. This is what lets a spool tell an inventory field from a misspelled attr, replacing a hedge that could not.Spool.unselectis the complement ofselect, taken againstselectitself rather than by negating each predicate, so one keyword cannot come to mean different things in the two, and an attached inventory's names work here because they work there. Coordinates are refused: a range decides how much of each patch to keep rather than which patches, so its complement is a hole in the middle rather than a filter.Two fixes found on the way.
PatchCatalog._ordered_idsreturned a fixed membership unfiltered whenever one was set, ignoring every predicate composed after it — sodiv[2:8].select(tag="big_gaps")[0:1]came back empty ondev. It now filters the membership while keeping its order, since an integer array may have arranged it. AndPatch.enrichaccepted a bare track name (coords=("coupling",)) without implementing what it means, silently filling NaN instead of the coupling type.Performance
Attaching stays free, which is the point of it being a separate step. On one 10k-patch in-memory spool, best of 9:
select(tag=...)select(gauge_length=...), every patch states itselect(gauge_length=...), no patch states itThe last row is the feature's real cost: resolving rows the index cannot answer for realizes the spool's contents, where an index-only select stays lazy. It is linear in the spool (~25 µs/row) and pays for one resolution per epoch, not per patch. The first two rows are what an adversarial review turned up and this branch then fixed — before it, the same three cases cost 3.7×/29× and 225× their no-inventory counterparts, because every select walked the inventory twice, re-read the index's name lists three times, and realized the relation to discover the index had already answered.
Also here
Patch.enrich'son_missing="skip"is renamed"ignore", so every policy knob draws from one word set (raise/warn/ignore, plus"null", which is an action rather than a volume). Nothing is released, so no compatibility spelling is kept.Patch.enrichandSpool.enrichdocument the same arguments from one source instead of describing them twice.key=valuepath segments override attrs the files themselves state. The path winning is deliberate — renaming a directory is how a layout corrects metadata — but it is also what a mis-sorted archive looks like from the inside. The comparison is on meaning rather than spelling, sogauge_length=10over a file stating10.0stays silent.Changelog
Spool.selectfilters on the observing-system facts an attached inventory states (spool.select(gauge_length=10.0),spool.select(**{"interrogator.model": "FI-1"})), even when no file recorded them. A patch which states the name is judged by the index as before and only the rest are resolved, once per epoch; a patch the inventory does not describe, or describes twice, is not selected. The inventory never writes into the index, solenandget_contentsstay exact and no data is read. Unlike an index-only select, this realizes the spool's contents.Spool.unselect, the complement ofselect—spool.unselect(tag="bad")returns everything the matching selection would have removed. Every keyword means what it means inselect. Coordinates are refused, since a range trims each patch rather than filtering the spool.Inventory.get_namesreturns the names an inventory could contribute to a patch, split intoattrsandcoords. The attrs side is read off the models, so a field added to an acquisition or interrogator becomes selectable without a second list to maintain.key=valuepath segments override attrs the files themselves state, naming each attr and one path carrying it. A segment restating what a file already says is silent.spool[2:8].select(tag="x")[0:1]came back empty.Patch.enrich(coords=("coupling",))and the other bare track names now give the track's identity field (the coupling type, the geometry or component name) instead of silently filling missing values.Patch.enrich'son_missing="skip"is renamed"ignore", so every policy argument draws from one word set. Breaking only against unreleased work ondev.Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):
Summary by CodeRabbit
New Features
unselectfor removing matching patches.on_missing="ignore"during enrichment.Bug Fixes
Documentation