treewide: use ContextManagerMixin#592
Conversation
WalkthroughRaised the minimum anyio dependency to >=4.10.0 across templates and multiple driver packages. Converted several classes from AbstractContextManager/AbstractAsyncContextManager to AnyIO mixins and replaced explicit enter/exit/aenter/aexit implementations with generator-based contextmanager/asynccontextmanager methods. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Lease as Lease (Async & Sync mixins)
participant Resource as LeasedResource
Note right of Lease: New generator-based async context manager
User->>Lease: async with Lease(...)
activate Lease
Lease->>Resource: acquire / start
Lease-->>User: yield Lease
User->>Lease: exit
Lease->>Resource: release / teardown
deactivate Lease
Note over Lease: Sync use bridged via __contextmanager__
User->>Lease: with Lease(...)
activate Lease
Lease->>Resource: acquire (via portal.wrap_async_context_manager)
Lease-->>User: yield Lease
User->>Lease: exit
Lease->>Resource: release
deactivate Lease
sequenceDiagram
autonumber
actor Caller
participant Client as DbusNetworkClient (ContextManagerMixin)
participant Adapter as DbusAdapter
Note right of Client: generator-based __contextmanager__ yields Adapter
Caller->>Client: with DbusNetworkClient(...)
activate Client
Client->>Adapter: Adapter.__enter__()
Client-->>Caller: yield Adapter
Caller->>Client: exit
Client->>Adapter: Adapter.__exit__()
deactivate Client
sequenceDiagram
autonumber
actor Exporter
participant Session as Session (ContextManagerMixin)
participant Root as RootDevice
participant Log as LoggingHandler
Note right of Session: __contextmanager__ does setup -> yield -> teardown
Exporter->>Session: with Session(...)
activate Session
Session->>Log: add handler
Session->>Root: reset()
Session-->>Exporter: yield Session
Exporter->>Session: exit
Session->>Root: close()
Session->>Log: remove handler
deactivate Session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (13)
🧰 Additional context used🧬 Code graph analysis (2)packages/jumpstarter/jumpstarter/exporter/session.py (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Make use of https://anyio.readthedocs.io/en/stable/contextmanagers.html, which has the added benefit of more robust error handling. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/jumpstarter-driver-http/pyproject.toml (1)
35-40: Remove duplicate pytest-asyncio entry; keep a single, modern versionThere are two entries for pytest-asyncio (one with a placeholder 0.0.0). Keep one line (≥0.24.0 or newer) to match pytest 8.x, and drop the placeholder.
Rationale:
- pytest-asyncio 0.24.0+ targets pytest ≥8.2; newer 0.26.0/1.0.0+ are fine with pytest 8.3.x. (pytest-asyncio.readthedocs.io)
Apply:
dev = [ "pytest-cov>=6.0.0", "pytest>=8.3.3", - "pytest-asyncio>=0.0.0", - "pytest-asyncio>=0.24.0", + "pytest-asyncio>=0.24.0", ]Optional: consider using AnyIO’s built-in pytest plugin instead of pytest-asyncio if you want cross-backend tests (no extra package needed). (anyio.readthedocs.io)
packages/jumpstarter-driver-tftp/pyproject.toml (1)
16-23: Pick a single async testing plugin; drop unnecessary pytest-anyio and placeholdersThe dev group mixes pytest-anyio and pytest-asyncio, plus placeholder versions (0.0.0). Use one plugin consistently:
- If your tests are asyncio-only, keep pytest-asyncio (≥0.24.0).
- If you prefer AnyIO’s plugin, you don’t need pytest-anyio at all; it’s obsolete (the plugin ships with AnyIO).
References:
- pytest-asyncio 0.24.0+ supports pytest ≥8.2. (pytest-asyncio.readthedocs.io)
- “pytest-anyio” explicitly says you don’t need it; the plugin is built into AnyIO. (pypi.org)
- AnyIO docs describe the built-in pytest plugin. (anyio.readthedocs.io)
Example (keep pytest-asyncio):
dev = [ "pytest>=8.3.2", "pytest-cov>=6.0.0", - "pytest-anyio>=0.0.0", - "pytest-asyncio>=0.0.0", + "pytest-asyncio>=0.24.0", "jumpstarter-testing", ]Alternative (use AnyIO’s plugin, no extra package):
dev = [ "pytest>=8.3.2", "pytest-cov>=6.0.0", - "pytest-anyio>=0.0.0", - "pytest-asyncio>=0.0.0", "jumpstarter-testing", ]packages/jumpstarter-driver-probe-rs/pyproject.toml (1)
11-14: Invalid version specifier: click>=8.1.7.2 (no such release); fix to a valid versionClick uses standard three-part versions (e.g., 8.1.7, 8.1.8, 8.2.1). There is no 8.1.7.2 on PyPI; this pin will never resolve. Change to a valid version, e.g., >=8.1.7 (or consider >=8.2.1).
References: Click release history and latest versions on PyPI. (click.palletsprojects.com, pypi.org, data.safetycli.com)
Apply:
dependencies = [ "anyio>=4.10.0", - "click>=8.1.7.2", + "click>=8.1.7", "jumpstarter", "jumpstarter-driver-opendal", ]
🧹 Nitpick comments (1)
packages/jumpstarter-driver-shell/pyproject.toml (1)
9-9: Dependency bump to anyio>=4.10.0 — approved.Minor nit: most other drivers list dependencies as a multi-line array; keep as-is if the repo accepts both styles, otherwise consider aligning for consistency in a follow-up.
-dependencies = ["anyio>=4.10.0", "jumpstarter"] +dependencies = [ + "anyio>=4.10.0", + "jumpstarter", +]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
__templates__/driver/pyproject.toml.tmpl(1 hunks)packages/jumpstarter-driver-energenie/pyproject.toml(1 hunks)packages/jumpstarter-driver-flashers/pyproject.toml(1 hunks)packages/jumpstarter-driver-http-power/pyproject.toml(1 hunks)packages/jumpstarter-driver-http/pyproject.toml(1 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(2 hunks)packages/jumpstarter-driver-probe-rs/pyproject.toml(1 hunks)packages/jumpstarter-driver-shell/pyproject.toml(1 hunks)packages/jumpstarter-driver-tasmota/pyproject.toml(1 hunks)packages/jumpstarter-driver-tftp/pyproject.toml(1 hunks)packages/jumpstarter-driver-yepkit/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/client/lease.py(3 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Applied to files:
packages/jumpstarter-driver-tftp/pyproject.toml
🧬 Code Graph Analysis (3)
packages/jumpstarter/jumpstarter/exporter/session.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (2)
Driver(56-269)reset(89-91)packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py (2)
reset(120-121)reset(183-184)
packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/dbus.py (1)
DbusAdapter(12-34)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
client(26-27)client(160-161)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
Lease(112-192)DeleteLease(383-389)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
🔇 Additional comments (11)
__templates__/driver/pyproject.toml.tmpl (1)
12-12: anyio>=4.10.0 is the correct floor for ContextManagerMixin usage — LGTMThe bump aligns with the new ContextManagerMixin/AsyncContextManagerMixin pattern introduced in AnyIO 4.10.0 and documented in AnyIO’s “Context manager mix-in classes.”
See: AnyIO 4.10.0 release notes and docs. (newreleases.io, anyio.readthedocs.io)packages/jumpstarter-driver-http/pyproject.toml (1)
10-15: anyio floor bump is correct for ctx-mixin refactor — LGTMThe refactor to AnyIO’s ContextManagerMixin requires ≥4.10.0; this pin is appropriate.
Refs: AnyIO 4.10.0 release + docs on mixins. (newreleases.io, anyio.readthedocs.io)packages/jumpstarter-driver-yepkit/pyproject.toml (1)
10-14: anyio>=4.10.0 bump matches new context-manager mixins — LGTMNo further changes needed here.
Refs for mixins: AnyIO 4.10.0 release + docs. (newreleases.io, anyio.readthedocs.io)packages/jumpstarter-driver-tftp/pyproject.toml (1)
10-14: anyio floor bump appropriate — LGTMConsistent with repo-wide migration to AnyIO’s context manager mixins.
Refs: AnyIO 4.10.0 release + docs. (newreleases.io, anyio.readthedocs.io)packages/jumpstarter-driver-probe-rs/pyproject.toml (1)
10-14: anyio min-version update is correct for mixin-based context managers — LGTMAligned with AnyIO 4.10.0 features.
Refs: AnyIO 4.10.0 release + mixin docs. (newreleases.io, anyio.readthedocs.io)packages/jumpstarter-driver-energenie/pyproject.toml (1)
12-12: AnyIO minimum raised to 4.10.0 — looks good.Consistent with the repository-wide bump and the mixin-based context management.
packages/jumpstarter-driver-iscsi/pyproject.toml (1)
9-9: AnyIO ≥4.10.0 requirement — approved; no legacy pins found– Searched for any
*constraints*.txtorrequirements*.txtfiles; none exist in the repository
– No older AnyIO versions are pinned via constraint or requirements filespackages/jumpstarter-driver-http-power/pyproject.toml (1)
12-12: Raise to anyio>=4.10.0 — approved.Matches the rest of the drivers; no other changes needed here.
packages/jumpstarter/jumpstarter/exporter/session.py (1)
35-36: Adopting ContextManagerMixin: LGTM.The class now cleanly derives enter/exit from the mixin. No issues spotted.
packages/jumpstarter/jumpstarter/client/lease.py (2)
30-30: Migration to ContextManagerMixin/AsyncContextManagerMixin: LGTM.The shape matches AnyIO’s mixins and keeps sync/async parity.
137-141: Sync bridge via portal.wrap_async_context_manager: LGTM.Yields Self and mirrors async behavior. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
33-44: Teardown may be skipped on exceptions; ensure unregister always runs and the gRPC channel is closedWith contextlib.asynccontextmanager, code after yield will not run if an exception propagates unless it’s in a finally block. Currently, an error inside the async with Exporter block will bypass Unregister. Also, the controller channel from channel_factory is never closed, which can leak connections if the factory returns a new channel each time (as suggested by the config snippet).
Refactor to:
- Wrap cleanup in try/finally so Unregister runs even on error.
- Use async with on the channel (assuming the factory returns an async context-manageable channel) to guarantee closure.
- Don’t let teardown errors mask the original exception; log and continue.
- Set registered back to False after attempting to unregister.
- Fix the return type to AsyncGenerator[Self, None] to satisfy type checkers.
@asynccontextmanager -async def __asynccontextmanager__(self) -> AsyncGenerator[Self]: - yield self - if self.registered: - controller = jumpstarter_pb2_grpc.ControllerServiceStub(await self.channel_factory()) - logger.info("Unregistering exporter with controller") - await controller.Unregister( - jumpstarter_pb2.UnregisterRequest( - reason="TODO", - ) - ) +async def __asynccontextmanager__(self) -> AsyncGenerator[Self, None]: + try: + yield self + finally: + if self.registered: + logger.info("Unregistering exporter with controller") + try: + async with await self.channel_factory() as channel: + controller = jumpstarter_pb2_grpc.ControllerServiceStub(channel) + await controller.Unregister( + jumpstarter_pb2.UnregisterRequest( + reason="shutdown", + ) + ) + except Exception: + logger.warning( + "Exporter teardown: Unregister failed; ignoring since we're exiting.", + exc_info=True, + ) + finally: + self.registered = False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
packages/jumpstarter/jumpstarter/config/exporter.py (1)
channel_factory(163-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
25-31: Adoption of AsyncContextManagerMixin looks good; dataclass usage is consistentSubclassing AsyncContextManagerMixin and providing the asynccontextmanager hook is aligned with AnyIO’s pattern. Field defaults and kw_only usage are consistent.
|
oopps confict, can you rebase @NickCao ? sorry about that |
NickCao
force-pushed
the
anyio-ctx-mixin
branch
from
2abd8d3 to
12cc998
Compare
3 participants
Summary by CodeRabbit
Chores
Refactor