Remove mypy ignore_errors from orchestrator/core modules (#68)#72
Remove mypy ignore_errors from orchestrator/core modules (#68)#72bradjin8 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR tightens type annotations across core and utility modules, adds runtime shape checks in YAML query helpers, refactors orchestrator callback wiring, and updates the pre-commit mypy hook plus mypy override configuration. ChangesStrict mypy typing cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cli/localci/core/orchestrator.py (1)
294-299: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSilent job drop if
poolis None mid-dispatch.If
self._poolwere everNonehere, the job already popped viaself.queue.next_ready()is dropped without being marked failed/cancelled or requeued, which could leaveself.queue.is_donenever becoming true (job neither counted as completed nor retried), stalling the dispatch loop indefinitely. In practiceself._poolis set once inexecute()before_dispatch_loop()runs and is never reset toNone, so this branch is likely unreachable today — but it's worth guarding intentionally (e.g. logging and marking the job failed/cancelled) rather than silently continuing, in case future changes ever null outself._poolconcurrently.Optional defensive fix
pool = self._pool if pool is None: + logger.warning("Pool unavailable, cannot dispatch %s", job.matrix_entry.name) + self.queue.mark_completed(job, success=False) continue🤖 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 `@cli/localci/core/orchestrator.py` around lines 294 - 299, The dispatch path in orchestrator._dispatch_loop currently drops a job silently when self._pool is None after queue.next_ready() has already returned it. Add an explicit defensive handling branch in _dispatch_loop (or the surrounding dispatch logic) that logs the condition and marks the job as failed/cancelled or requeues it instead of just continuing, using the existing _execute_job, _make_done_callback, and queue handling flow so the job is not lost and is still accounted for.cli/localci/core/progress.py (1)
411-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
RenderableTypeinstead ofAnyfor render helpers.These methods return Rich renderables (
Panel,Text,Table,Group);rich.console.RenderableType(ConsoleRenderable | RichCast | str) is the precise type Rich itself uses for such returns and forLive/Live.update()arguments. UsingAnysatisfies strict mypy but discards type-checking on these composition points.♻️ Example
+from rich.console import RenderableType - def _render_live(self) -> Any: + def _render_live(self) -> RenderableType: """Render the complete live display.""" from rich.console import Group - parts: list[Any] = [] + parts: list[RenderableType] = []Also applies to: 422-422, 439-439, 457-457, 474-474
🤖 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 `@cli/localci/core/progress.py` around lines 411 - 420, The render helper return types are too loose: `_render_live` and the related render methods should use Rich’s `RenderableType` instead of `Any` so type-checking is preserved for these composition points. Update the return annotations on `_render_live`, `_render_header`, `_render_progress_bar`, `_render_priority_levels`, and `_render_job_table` to the precise Rich renderable type used by `Live` and `Live.update()`, keeping the existing return values unchanged.cli/localci/utils/yq.py (1)
376-389: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
job_runs_on/job_timeoutlack the shape validation applied to sibling methods.Unlike
concurrency,job_container,job_defaults, andmatrix_strategy, which now validate the queried value's type before returning,job_runs_on(->str) andjob_timeout(->int) still just doquery(...) or default. Sincequery()returnsAny, mypy won't flag a malformed workflow (e.g.,timeout-minutes: "10"or a non-scalarruns-on) returning the wrong runtime type, unlike the pattern used elsewhere in this diff.♻️ Suggested fix for consistency
def job_runs_on(self, file: Path, job_id: str) -> str: """Extract job ``runs-on``.""" - return self.query(file, f".jobs.{job_id}.runs-on") or "ubuntu-latest" + result = self.query(file, f".jobs.{job_id}.runs-on") + return result if isinstance(result, str) else "ubuntu-latest" def job_container(self, file: Path, job_id: str) -> dict[str, Any] | None: ... def job_timeout(self, file: Path, job_id: str) -> int: """Extract job timeout in minutes.""" - return self.query(file, f".jobs.{job_id}.timeout-minutes") or 60 + result = self.query(file, f".jobs.{job_id}.timeout-minutes") + return result if isinstance(result, int) else 60🤖 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 `@cli/localci/utils/yq.py` around lines 376 - 389, `job_runs_on` and `job_timeout` still return raw `query()` results with a fallback, so they can silently pass through malformed workflow values instead of enforcing `str`/`int` like the sibling validators. Update these methods in `Yq` to mirror the existing type-checking pattern used by `concurrency`, `job_container`, `job_defaults`, and `matrix_strategy`: fetch the value, validate its shape/type, return the typed value only when valid, and otherwise fall back to the default (`"ubuntu-latest"` / `60`).
🤖 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.
Nitpick comments:
In `@cli/localci/core/orchestrator.py`:
- Around line 294-299: The dispatch path in orchestrator._dispatch_loop
currently drops a job silently when self._pool is None after queue.next_ready()
has already returned it. Add an explicit defensive handling branch in
_dispatch_loop (or the surrounding dispatch logic) that logs the condition and
marks the job as failed/cancelled or requeues it instead of just continuing,
using the existing _execute_job, _make_done_callback, and queue handling flow so
the job is not lost and is still accounted for.
In `@cli/localci/core/progress.py`:
- Around line 411-420: The render helper return types are too loose:
`_render_live` and the related render methods should use Rich’s `RenderableType`
instead of `Any` so type-checking is preserved for these composition points.
Update the return annotations on `_render_live`, `_render_header`,
`_render_progress_bar`, `_render_priority_levels`, and `_render_job_table` to
the precise Rich renderable type used by `Live` and `Live.update()`, keeping the
existing return values unchanged.
In `@cli/localci/utils/yq.py`:
- Around line 376-389: `job_runs_on` and `job_timeout` still return raw
`query()` results with a fallback, so they can silently pass through malformed
workflow values instead of enforcing `str`/`int` like the sibling validators.
Update these methods in `Yq` to mirror the existing type-checking pattern used
by `concurrency`, `job_container`, `job_defaults`, and `matrix_strategy`: fetch
the value, validate its shape/type, return the typed value only when valid, and
otherwise fall back to the default (`"ubuntu-latest"` / `60`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 084a88d0-dd64-4208-bb5e-e82e10b0bbf3
📒 Files selected for processing (12)
.pre-commit-config.yamlcli/localci/core/orchestrator.pycli/localci/core/progress.pycli/localci/core/queue_builder.pycli/localci/core/registry.pycli/localci/core/results.pycli/localci/core/serialization.pycli/localci/core/workflow.pycli/localci/utils/docker.pycli/localci/utils/resources.pycli/localci/utils/yq.pycli/pyproject.toml
1 participant
Summary
ignore_errorssuppression for 10 in-scope modules:localci.core.orchestrator,progress,queue_builder,registry,results,serialization,workflow, andlocalci.utils.docker,resources,yqlocalci.cli.*modules on the override list (CLI typing deferred to a follow-up)python -m mypyinstead ofbash -c) so it runs on Windows without WSLCloses #68
Test plan
cd cli && mypy localci/— Success (47 files)cd cli && pytest -m "not integration"— 587 passedpre-commit run --all-files— all hooks passed (ruff + mypy)Summary by CodeRabbit