Burn down the coord, protobuf and forwarding-mismatch diagnostics - #821
Conversation
The coord methods which canonicalize declared they return Self, but a coord routinely comes back as a different class: empty() always gives a CoordPartial, index() and snap() and sort() give a CoordRange from an array coord, and select() does too once the selection turns out to be evenly sampled. Those six say BaseCoord now. The type variable stays where the class really is preserved, such as convert_units. The base also disagreed with every one of its implementations about two parameter names -- arg against args, unit against units -- so the base moved, which is the side nothing calls. CoordPartial aliased update_limits and set_units to update, whose only parameter is **kwargs. That made set_units unusable: patch.set_units on any dimension whose coord holds no values raised TypeError rather than recording the units. They are spelled out now, each keeping the signature its base declares.
The protobuf attrs were collected in a plain dict and splatted into the model, so every field was offered the dict's value union; building the model directly and applying the family's extras with new() leaves each field its own type. Two helpers there also under-declared: a packet with no header time contributes None, and the record parser only iterates. Patch's coords parameter was narrower than the CoordManagerInput it forwards to, and get_coord_manager took only a tuple of dims while its callers have a Sequence -- which also meant a list of dims never compared equal to a CoordManager's tuple.
get_quantity has always taken a bare number as dimensionless and a pint Unit as itself, but three of its neighbours declared narrower subsets of the same idea, so passing a value from one to another was an error. They share one alias now. unbyte only decodes bytes and hands everything else back untouched, which its bytes | str signature could not say.
The repo lints with ruff and type-checks with ty in pre-commit, so say so where everything else is said.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughChangesThe pull request updates typing and input normalization across coordinate, quantity, IO, and utility APIs. It adds explicit API and input contract updates
Possibly related PRs
Suggested labels: 🚥 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
IO
![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 (1)
dascore/core/attrs.py (1)
141-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse an explicit exception instead of
assertfor input validation.
assert isinstance(out, Mapping)enforcesfrom_dict's type contract. Python removesassertstatements when the interpreter runs with the-Oflag. If that flag is used and a caller passes a value that violates theMapping | PatchAttrs | Nonetype hint, the assertion is skipped anddict(out)receives an unexpected value, producing a less clear error or unintended behavior.Raise a
TypeError(or another explicit exception) instead of usingassert, so the contract holds regardless of the optimization flag.♻️ Proposed fix
- # Anything not already a mapping came from model_dump, which - # returns one, so this only restates the contract for the checker. - assert isinstance(out, Mapping), "attr_map must resolve to a mapping" + # Anything not already a mapping came from model_dump, which + # returns one, so this only restates the contract for the checker. + if not isinstance(out, Mapping): + msg = f"attr_map must resolve to a mapping, got {type(out)}." + raise TypeError(msg) out = dict(out)🤖 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/attrs.py` around lines 141 - 146, Replace the assert in the from_dict flow with an explicit TypeError when out is not a Mapping. Preserve the existing validation message and continue converting valid mappings with dict(out), removing dims, and constructing the class unchanged.
🤖 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/coords.py`:
- Around line 1243-1252: Update CoordPartial.update_limits to translate min to
start and max to stop before forwarding the limits, rather than passing min/max
unchanged through self.update. Preserve step and kwargs forwarding, ensuring
update_limits(min=...) produces a partial with the requested start value instead
of start=np.nan.
In `@dascore/io/index/indexer.py`:
- Line 139: Remove the eager non-UPath-to-Path conversion before the
local-directory validation in the indexer initializer. Preserve the original
URI-capable input through the requires_local_directory check, then pass it to
coerce_to_local_path and assign the resulting absolute path to self.path,
keeping Path inputs supported without bypassing URI handling.
---
Nitpick comments:
In `@dascore/core/attrs.py`:
- Around line 141-146: Replace the assert in the from_dict flow with an explicit
TypeError when out is not a Mapping. Preserve the existing validation message
and continue converting valid mappings with dict(out), removing dims, and
constructing the class unchanged.
🪄 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: e1710a82-6835-4c07-878d-676d6c2562b0
📒 Files selected for processing (17)
dascore/core/attrs.pydascore/core/coordmanager.pydascore/core/coords.pydascore/core/patch.pydascore/core/summary.pydascore/io/core.pydascore/io/index/indexer.pydascore/io/sintela/protobuf_utils.pydascore/io/terra15/utils.pydascore/units.pydascore/utils/downloader.pydascore/utils/misc.pydascore/viz/spectrogram.pypyproject.tomlreadme.mdtests/test_core/test_coordmanager.pytests/test_core/test_coords.py
| # Other operations that normally modify data do not in this case; | ||
| # they are spelled out rather than aliased so each keeps the | ||
| # signature its base declares. | ||
| def update_limits(self, min=None, max=None, step=None, **kwargs) -> BaseCoord: | ||
| """No values to change, so only the metadata in kwargs is applied.""" | ||
| return self.update(min=min, max=max, step=step, **kwargs) | ||
|
|
||
| def set_units(self, units) -> Self: | ||
| """No values to change, so this only records the new units.""" | ||
| return self.update(units=units) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check get_coord's shape-handling branch when both shape and min/max are supplied.
rg -n -A 20 'if segments is not None' dascore/core/coords.py
rg -n 'shape' dascore/core/coords.py | rg -n 'get_coord'Repository: DASDAE/dascore
Length of output: 1440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== get_coord signature and alias handling =="
rg -n -A 60 '^def get_coord' dascore/core/coords.py
rg -n -B 20 -A 80 'min=.*start|min.*start|max.*stop|start=.*min|stop=.*max|min=None|max=None|start=None|stop=None' dascore/core/coords.py | head -220
echo "== CoordPartial update/update_limits =="
sed -n '1200,1270p' dascore/core/coords.py
echo "== tests for CoordPartial update_limits =="
rg -n -A 40 'update_limits|test_update_limits' tests/test_core/test_coords.pyRepository: DASDAE/dascore
Length of output: 11391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== full get_coord alias resolution and body =="
sed -n '2832,3015p' dascore/core/coords.py
echo "== focused search for alias branches =="
rg -n -A 12 -B 8 'min is not None|start if min|if stop is None|stop = max|max is not None' dascore/core/coords.py
echo "== existing update_limits tests =="
rg -n -A 50 -B 10 'update_limits|test_update_limits|set_units|CoordPartial' tests/test_core/test_coords.py | sed -n '1,240p'Repository: DASDAE/dascore
Length of output: 24479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate existing update_limits tests =="
rg -n 'def test_.*[Uu]pdate_[Ll]imits|update_limits' tests/test_core/test_coords.py tests -g '*.py'
echo "== search for min/max partial tests around coordination =="
rg -n -A 25 -B 10 'CoordPartial\(shape=.*start|get_coord\(shape=.*start|start=.*stop=.*step=.*units|test_.*partial' tests/test_core/test_coords.pyRepository: DASDAE/dascore
Length of output: 19791
Fix the update_limits forwarding logic for CoordPartial.
get_coord(min=...) maps min to start, but CoordPartial.update_limits(min=...) forwards min through get_coord unchanged, which creates a partial with start=np.nan even though the documented intent is to set the start. Drop min/max by merging them into start/stop before forwarding, or avoid passing them through self.update(...).
🧰 Tools
🪛 Ruff (0.16.1)
[error] 1246-1246: Function argument min is shadowing a Python builtin
(A002)
[error] 1246-1246: Function argument max is shadowing a Python builtin
(A002)
🤖 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/coords.py` around lines 1243 - 1252, Update
CoordPartial.update_limits to translate min to start and max to stop before
forwarding the limits, rather than passing min/max unchanged through
self.update. Preserve step and kwargs forwarding, ensuring
update_limits(min=...) produces a partial with the requested start value instead
of start=np.nan.
| path = UPath(path).absolute() if isinstance(path, UPath) else Path(path) | ||
| requires_local_directory(path, label="DBDirectoryIndexer") | ||
| self.path = Path(path).absolute() | ||
| self.path = coerce_to_local_path(path).absolute() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve URI inputs until coerce_to_local_path runs.
Line 137 converts every non-UPath input to Path before Line 139 calls coerce_to_local_path. For file:///tmp/data or local:///tmp/data, the helper receives a Path and returns it unchanged. self.path then points to a literal file:/... path instead of the intended local path. A remote-scheme string can also bypass requires_local_directory after this conversion.
Remove the eager Path conversion and apply coerce_to_local_path after the local-directory check. dascore/utils/paths.py Lines 80-93 define this Path fast path.
Proposed fix
- path = UPath(path).absolute() if isinstance(path, UPath) else Path(path)
requires_local_directory(path, label="DBDirectoryIndexer")
self.path = coerce_to_local_path(path).absolute()🤖 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/indexer.py` at line 139, Remove the eager non-UPath-to-Path
conversion before the local-directory validation in the indexer initializer.
Preserve the original URI-capable input through the requires_local_directory
check, then pass it to coerce_to_local_path and assign the resulting absolute
path to self.path, keeping Path inputs supported without bypassing URI handling.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #821 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17906 17914 +8
=========================================
+ Hits 17906 17914 +8
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:
|
Two of the changes were regressions and three of the annotations were
false.
CoordPartial.update_limits forwarded min, max and step whether or not
the caller passed them, and a None reaching the nullish validator
overwrites the stored scalar with nan -- so stacking along a dimension
whose coord holds no values quietly lost its step, and with it the
coord's fingerprint. It forwards only what it was given. The test that
claimed to cover this used a coord with nothing to lose; it now uses one
with a step, and fails without the fix.
PatchAttrs.from_dict grew an assert whose comment was wrong: the branch
it replaced took arbitrary caller input, so a pandas Series of attrs
stopped working, the error escaped the TypeError handler in the scan
path, and python -O removed the check entirely. The guard is back.
drop_coords takes bare names -- the body makes a set of its varargs, so
a collection is either unhashable or silently ignored -- and Patch takes
every mapping get_coord_manager does, including the {name: list} form
its own tests pass. Both said otherwise. quantity_like left out bytes
and Ellipsis, which get_quantity opens by handling, while promising
get_quantity_str a numpy time it stringifies into a date.
Self was correct for change_length and for a segmented coord's snap, so
they keep it, and CoordPartial's set_units override goes away: the base
rebuilds the same class and already takes its argument positionally.
Three of the changes turned out to do nothing at runtime and are
reverted rather than left as noise.
The four remaining ones were each a real disagreement, not noise. BaseCoord's shape validator was named for a job it does not do, and the name collided with CoordPartial's start/stop/step validator -- pydantic lets the subclass replace it, so a partial coord silently lost the int to tuple coercion every other coord has. Renaming it to what it does restores that and removes the collision. PlanResolver.live_entries promised a Mapping where the registry it returns is popped from. CoordManager.new named three fields where its base takes any. sensible_model_equals declared an other it cannot require, and now returns NotImplemented for anything that cannot carry the same fields, which is what __eq__ is supposed to do. With those gone the rule holds at zero, so it comes out of the ignore list.
1 participant
Description
Continues the ty burn-down after #816, combining the four remaining planned pieces. Across the three still-ignored rules the count goes 156 → 65:
invalid-argument-typeinvalid-return-typeinvalid-method-overrideCoords say what they actually return
The coord methods which canonicalize declared
-> Self, but a coord routinely comes back as a different class —empty()always gives aCoordPartial,index()/snap()/sort()give aCoordRangefrom an array coord, andselect()does too once the selection turns out to be evenly sampled. Those six sayBaseCoordnow; the type variable stays where the class really is preserved, such asconvert_units.The base also disagreed with every one of its own implementations about two parameter names —
argagainstargs,unitagainstunits— so the base moved, which is the side nothing calls (all 70 in-repoconvert_unitscalls are positional).A bug fell out of that.
CoordPartialaliasedupdate_limitsandset_unitstoupdate, whose only parameter is**kwargs, which madeset_unitsunusable:Both are spelled out now, each keeping the signature its base declares. Two tests cover it and fail on the old code.
The sintela protobuf block
23 diagnostics on a single line, and the same root cause as #807 and #812: attrs collected in a plain
dictand splatted into the model, so every field was offered the dict's value union. Building the model directly and applying the family's extras withnew()leaves each field its own type. Two helpers there also under-declared — a packet with no header time contributesNone, and the record parser only iterates.Things that were narrower than what they forward to
Patch.__init__'scoordswas narrower than theCoordManagerInputit hands toget_coord_manager, andget_coord_managertook only atupleof dims while its callers have aSequence— which also meant a list of dims never compared equal to aCoordManager's tuple, so a rename was silently skipped.get_quantityhas always accepted a bare number as dimensionless and a pintUnitas itself, but three of its neighbours declared narrower subsets of the same idea; they share onequantity_likealias now.unbyteonly decodes bytes and hands everything else back untouched, whichbytes | strcould not say.Three
Path(...)calls on a value that may be aUPathnow go through the existingcoerce_to_local_path, which is also more correct: barePath("file:///tmp/x")keeps the scheme as a literal path segment, and that helper exists precisely to strip it.Capstone
Ruff and ty badges in the readme.
What is not done
The rules stay ignored: 31 / 30 / 4 remain. They are one-offs now, not clusters — no two share a root cause — and several turn on whether a value can really be
Noneat that point, which is a question about behaviour rather than annotation. I stopped rather than guess at those; the review of #816 was a good reminder of what rushing one produces.Changelog
none
Checklist
I have (if applicable):
Summary by CodeRabbit
Enhancements
Documentation
Tests