Type checking harness + fix invalid-argument-type errors#113
Type checking harness + fix invalid-argument-type errors#113vinamarora8 merged 30 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a GitHub Actions workflow to run the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Benchmark comparison vs mainBaseline: |
vinamarora8
changed the title
vinamarora8
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a ty-based type-checking harness to CI and updates core constructors’ type hints to eliminate invalid-argument-type findings (primarily around **kwargs value types and "auto" domain handling).
Changes:
- Introduces a new GitHub Action workflow to run
ty checkon PRs and main pushes. - Updates type annotations in
ArrayDict,Interval,IrregularTimeSeries, andRegularTimeSeriesto better reflect runtime expectations. - Adds
tyconfiguration topyproject.tomland applies a targetedtyignore in a test that intentionally passes an invalid type.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_arraydict.py |
Adds a targeted ty ignore to keep a negative test while enforcing invalid-argument-type globally. |
temporaldata/arraydict.py |
Tightens **kwargs annotation to value-type (np.ndarray) to match runtime assertions. |
temporaldata/interval.py |
Tightens **kwargs annotation to np.ndarray for consistency with ArrayDict’s runtime checks. |
temporaldata/irregular_ts.py |
Makes timekeys explicitly optional and aligns **kwargs typing with ArrayDict. |
temporaldata/regular_ts.py |
Adds "auto" to the domain type and aligns **kwargs typing with ArrayDict. |
pyproject.toml |
Adds ty to dev deps and configures rules + source roots for type checking. |
.github/workflows/typecheck.yml |
New CI workflow to run ty check and annotate PRs with typing failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temporaldata/regular_ts.py (1)
24-25:⚠️ Potential issue | 🟡 MinorDocstring contradicts implementation.
Line 25 states "It is not possible to set domain to :obj:
\"auto\"", but the type annotation on line 57 now includesLiteral["auto"]and the implementation at lines 65-73 explicitly handlesdomain == "auto". The docstring should be updated to reflect the actual behavior.📝 Suggested fix
Args: sampling_rate: Sampling rate in Hz. domain: an :obj:`Interval` object that defines the domain over which the - timeseries is defined. It is not possible to set domain to :obj:`"auto"`. + timeseries is defined. If set to :obj:`"auto"`, the domain will be + automatically computed from the data length and sampling rate. **kwargs: Arbitrary keyword arguments where the values are arbitrary🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temporaldata/regular_ts.py` around lines 24 - 25, The docstring incorrectly forbids domain="auto" despite the type annotation (includes Literal["auto"]) and implementation handling domain == "auto" on lines where the domain param is processed; update the parameter description for domain to state it accepts either an Interval or the string "auto" and document that passing "auto" triggers automatic domain inference/creation (match the behavior implemented in the domain handling code around the domain type annotation and the domain == "auto" branch).
🧹 Nitpick comments (2)
.github/workflows/typecheck.yml (1)
1-31: LGTM!The workflow correctly sets up Python 3.10 with uv, installs dev dependencies, and runs the ty type checker with GitHub-formatted output for inline annotations.
Consider simplifying the workflow by using
uv runwhich handles virtual environment management automatically:♻️ Optional simplification
- name: Install project run: | - uv venv venv -p 3.10 - source venv/bin/activate - uv pip install -e ".[dev]" + uv pip install -e ".[dev]" --python 3.10 - name: Run ty run: | - source venv/bin/activate - ty check --output-format github + uv run ty check --output-format github🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/typecheck.yml around lines 1 - 31, The workflow works but can be simplified by using uv's built-in runner instead of manually creating/activating a venv; replace the steps that call "uv venv venv -p 3.10" and "source venv/bin/activate" plus the separate "uv pip install -e '.[dev]'" and "ty check" invocation with a single uv run invocation that runs the type checker in a managed environment (so remove the manual venv creation/activation and direct ty call and instead run ty via uv run, keeping the job name "typecheck" and step names like "Install project"/"Run ty" recognizable for locating the change).pyproject.toml (1)
26-26: Consider pinning the ty version.Since ty is in beta (latest version is 0.0.24), an unpinned dependency could cause unexpected CI failures if a new ty release introduces breaking changes or new diagnostics. Consider specifying a version constraint such as
ty<1to allow patch updates within the 0.0.x series while preventing major version breaks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 26, The dev dependency list currently includes an unpinned "ty" which can introduce CI instability; update the entry in pyproject.toml's dev = [...] list to constrain the "ty" package (e.g., change "ty" to "ty<1" or pin to a specific known-good release like "ty==0.0.24") so that the dev dependencies for the project are stable; locate the "dev" array containing "pytest<9", "black==24.2.0", "pre-commit>=3.5.0", "flake8", "pytest-cov", "ty" and replace the "ty" token with the chosen version constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@temporaldata/regular_ts.py`:
- Around line 24-25: The docstring incorrectly forbids domain="auto" despite the
type annotation (includes Literal["auto"]) and implementation handling domain ==
"auto" on lines where the domain param is processed; update the parameter
description for domain to state it accepts either an Interval or the string
"auto" and document that passing "auto" triggers automatic domain
inference/creation (match the behavior implemented in the domain handling code
around the domain type annotation and the domain == "auto" branch).
---
Nitpick comments:
In @.github/workflows/typecheck.yml:
- Around line 1-31: The workflow works but can be simplified by using uv's
built-in runner instead of manually creating/activating a venv; replace the
steps that call "uv venv venv -p 3.10" and "source venv/bin/activate" plus the
separate "uv pip install -e '.[dev]'" and "ty check" invocation with a single uv
run invocation that runs the type checker in a managed environment (so remove
the manual venv creation/activation and direct ty call and instead run ty via uv
run, keeping the job name "typecheck" and step names like "Install project"/"Run
ty" recognizable for locating the change).
In `@pyproject.toml`:
- Line 26: The dev dependency list currently includes an unpinned "ty" which can
introduce CI instability; update the entry in pyproject.toml's dev = [...] list
to constrain the "ty" package (e.g., change "ty" to "ty<1" or pin to a specific
known-good release like "ty==0.0.24") so that the dev dependencies for the
project are stable; locate the "dev" array containing "pytest<9",
"black==24.2.0", "pre-commit>=3.5.0", "flake8", "pytest-cov", "ty" and replace
the "ty" token with the chosen version constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3638210b-d673-4ee7-8f4d-52b31bfb507c
📒 Files selected for processing (7)
.github/workflows/typecheck.ymlpyproject.tomltemporaldata/arraydict.pytemporaldata/interval.pytemporaldata/irregular_ts.pytemporaldata/regular_ts.pytests/test_arraydict.py
…y/temporaldata#113) Type checker used: ty
This PR:
1) Adds a type-checking CI action that will show an error if typing rules are violated in a PR (example error). It uses Astral's
ty* under the hood (same folks that madeuv).2) Clears all
invalid-argument-typeerrors. And so, only this error is enabled at the moment. We can enable others as we solve them in the existing codebaseThis starts to address #65 but will need more work to fully close.
*Other typecheckers: I tried
pyright,mypy, andpyreflyas well, buttyseemed to be the one I most agreed with and it has the most readable error log. All type-checkers are opinionated at the end of the day.tyis in beta stage, so we are taking some small risk when going with it. But it's Astral; they have delivered quite well in the past withuv. Worst case: we have to move to another type checker in the future; should be an easy job for a coding agent.Summary by CodeRabbit
Chores
Tests