Ensure runtime type-checking with beartype #143
Conversation
…ctor() parameter "data" PEP 585 type hint dict[str | typing.Any] not subscripted (indexed) by 2 child type hints (i.e., subscripted by 1 != 2 child type hints).
for more information, see https://pre-commit.ci
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces comprehensive enhancements to type annotations, runtime type checking, and type alias definitions across the codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModelDF
participant AgentSetDF
participant SpaceDF
User->>ModelDF: create ModelDF()
ModelDF->>AgentSetDF: create AgentSetDF(model)
AgentSetDF->>SpaceDF: interact with SpaceDF (typed as concrete)
AgentSetDF->>AgentSetDF: add/discard/remove agents (runtime checked by @beartype)
AgentSetDF->>AgentSetDF: get/set agents, use advanced indexing
ModelDF->>AgentSetDF: get_agents_of_type(type) returns AgentSetDF
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
mesa_frames/abstract/agents.py (2)
43-52:⚠️ Potential issueMissing imports – unresolved names break
beartypeat import-time
@beartyperesolves annotations immediately when the decorator is executed.
Becausemesa_framesand its sub-modules are never imported in this file, every
fully-qualified annotation such asmesa_frames.concrete.model.ModelDFis
currently a dangling forward reference. This triggers aNameErrorduring
module import and is also the root-cause of the RuffF821warnings.-from abc import abstractmethod -from collections.abc import Callable, Collection, Iterable, Iterator, Sequence +from abc import abstractmethod +from collections.abc import Callable, Collection, Iterable, Iterator, Sequence from contextlib import suppress from beartype import beartype from numpy.random import Generator from typing import Self, overload, Any, Literal + +# --------------------------------------------------------------------------- # +# Imports required only for annotations but *must* exist at runtime so that # +# `beartype` can resolve them via `typing.get_type_hints`. Using `# noqa` # +# silences the “unused import” warnings without disabling strict type checks. # +# --------------------------------------------------------------------------- # +from mesa_frames.concrete.model import ModelDF # noqa: F401 +from mesa_frames.concrete.agents import AgentSetDF # noqa: F401 +from mesa_frames.abstract.space import SpaceDF # noqa: F401This keeps the readable, fully-qualified annotations while guaranteeing that
names exist in the module namespace for both static analysis and runtime
inspection. (If circular-import concerns arise, move these imports inside a
if TYPE_CHECKING:block and convert annotations back to strings, but that
negates the stated PR goal of avoidingTYPE_CHECKINGblocks.)
271-299:⚠️ Potential issue
select()lost its@abstractmethoddecorator – subclasses get a stub that returns...The method body is only
..., yet the decorator was removed. As a result:
AgentContaineris no longer abstract with respect toselect.- Any concrete subclass that does not override
selectwill inherit a
method that returns anEllipsis, leading to hard-to-trace runtime errors.- def select( + @abstractmethod + def select(Either re-apply
@abstractmethodor provide a concrete, working implementation.
🧹 Nitpick comments (1)
mesa_frames/abstract/agents.py (1)
932-945: Edge-case inremove()– magic string & duplicated traversal
Overloading
agents="active"mixes ID semantics with a command keyword and
can silently clash with a legitimate agent whose unique_id is the literal
string"active". Consider an explicit boolean flag instead, e.g.
remove_active: bool = False.The loop that searches the returned
agentsdffor anAgentSetDFof the
same class isO(n)per call. Ifagentsdf.agentswere keyed by the
agent-set name (or type) the lookup could be constant-time.🧰 Tools
🪛 Ruff (0.8.2)
941-941: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/build.yml(1 hunks).pre-commit-config.yaml(1 hunks)mesa_frames/abstract/agents.py(27 hunks)mesa_frames/concrete/mixin.py(15 hunks)pyproject.toml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/build.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- mesa_frames/concrete/mixin.py
🧰 Additional context used
🪛 Ruff (0.8.2)
mesa_frames/abstract/agents.py
71-71: Undefined name mesa_frames
(F821)
80-80: Undefined name mesa_frames
(F821)
81-81: Undefined name mesa_frames
(F821)
107-107: Undefined name mesa_frames
(F821)
108-108: Undefined name mesa_frames
(F821)
134-134: Undefined name mesa_frames
(F821)
139-139: Undefined name mesa_frames
(F821)
176-176: Undefined name mesa_frames
(F821)
187-187: Undefined name mesa_frames
(F821)
250-250: Undefined name mesa_frames
(F821)
251-251: Undefined name mesa_frames
(F821)
399-399: Undefined name mesa_frames
(F821)
400-400: Undefined name mesa_frames
(F821)
489-489: Undefined name mesa_frames
(F821)
490-490: Undefined name mesa_frames
(F821)
512-512: Undefined name mesa_frames
(F821)
513-513: Undefined name mesa_frames
(F821)
535-535: Undefined name mesa_frames
(F821)
536-536: Undefined name mesa_frames
(F821)
667-667: Undefined name mesa_frames
(F821)
687-687: Undefined name mesa_frames
(F821)
709-709: Undefined name mesa_frames
(F821)
747-747: Undefined name mesa_frames
(F821)
759-759: Undefined name mesa_frames
(F821)
772-772: Undefined name mesa_frames
(F821)
797-797: Undefined name mesa_frames
(F821)
801-801: Undefined name mesa_frames
(F821)
🔇 Additional comments (1)
.pre-commit-config.yaml (1)
20-20: Ensurepyupgradesupports the--py311-plusflag
The--py311-plusargument is only available in recentpyupgradereleases. Please verify that the pinned version (v3.19.1) indeed supports this flag to prevent pre-commit failures.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
- Coverage 91.68% 91.46% -0.22%
==========================================
Files 11 11
Lines 1684 1711 +27
==========================================
+ Hits 1544 1565 +21
- Misses 140 146 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Would it make sense to structure this so that runtime type checking is enabled only in development or test environments? For example, we could use a wrapper like: This way we could annotate functions with @implement_beartype, and control activation via environment variables. It would also allow more advanced users to opt-in to runtime type checking in production if they really want to i guess this will stop the over head from affecting everyone |
|
Yes @Ben-geo, I think the best thing would be to gate all runtime type-checking behind a single env var: import os
from beartype.claw import beartype_this_package
if os.getenv("MESA_FRAMES_RUNTIME_TYPECHECKING", "").lower() in ("1", "true", "yes"):
beartype_this_package()
This keeps our codebase clean, gives us type-safety in CI/dev, and preserves peak performance everywhere else. |
|
Awesome @adamamer20 I think this is the best solution. |
… link in contributing guide
for more information, see https://pre-commit.ci
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This PR adds beartype to enable runtime type checking across the codebase.
Given the extensive use of type hints already present, the integration should be straightforward and introduce minimal performance overhead. Preliminary testing on the sugarscape example showed negligible impact, though broader testing across the full codebase hasn't been performed yet.
Runtime type checking provides several benefits:
Some notes regarding this PR:
typing_extensionsand changed totypingwith the support of Python bumped up to 3.11. This is in line with the currentmesarepository.if TYPE_CHECKINGblocks for forward imports. This in fact doesn't work well with beartype. Instead, usingfrom __future__ import annotationsat the start of the file allows for the postponed evaluation of typesPerformance considerations
I ran a side-by-side comparison of
performance_plot.pywith and withoutbeartype(on Codespaces) to quantify the overhead. Below is the full picture:Key points (0 – 100 k agents)
Larger runs (100 k – 1 000 k agents)
Startup overhead at 0 agents is ~133 %, mostly due to module import and GC initialization.
For small→mid runs (10 k–90 k), expect +20–40 % on the pandas-concise path and +5–70 % on native-Polars.
A sharp spike at exactly 100 k agents (+113 % / +261 %) arises from crossing Python’s GC and list-resizing thresholds under heavy temporary-object churn.
Beyond 200 k, the relative penalty subsides to +2–30 % on concise and −7 %→+8 % on native.
Disable runtime checks with
-O.When you run Python under the
-O(optimize) flag, all@beartypechecks are skipped—so you recover essentially the same performance as the “no-beartype” build.(See the discussion in the Beartype issue tracker for details:
[FAQ] Document how to disable @beartype beartype/beartype#94)
Summary by CodeRabbit
Refactor
beartyperuntime type checking to core classes for improved type safety.Chores
typeguardwithbeartypefor runtime type checking in tests and dependencies..gitignoreand pre-commit configurations.Tests
beartypefor runtime type validation.