fix BackupMultiBucket's need for ExceptionGroup #161
Conversation
amaximciuc
commented
Nov 9, 2025
amaximciuc
commented
- make BackupMultiBucket import from exceptiongroup only on py<3.11
- add psutil as a dev dependency in poetry.lock
…d psutil as a dev dependency in poetry.lock
|
Warning Rate limit exceeded@asuiu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 49 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdated runtime import for ExceptionGroup, added two methods and a signature/variable change in BackupMultiBucket, adjusted FileLock release ordering, bumped package version and dev deps (added psutil), and modified CI to run Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Importer as Module import
participant Sys as sys
participant Backport as exceptiongroup
participant Builtin as built-in ExceptionGroup
rect `#F1FBFF`
Note over Importer,Sys: On import, probe for ExceptionGroup
end
Importer->>Sys: attempt to reference ExceptionGroup
alt NameError / missing built-in
Importer->>Backport: import ExceptionGroup from backport
Note right of Backport: runtime fallback
else built-in present
Importer->>Builtin: use built-in ExceptionGroup
Note right of Builtin: no external import
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the exceptiongroup import to be conditional based on Python version, since ExceptionGroup is built into Python 3.11+. It also adds psutil as an explicit dev dependency to support memory leak testing.
- Conditional import of
exceptiongroupfor Python < 3.11 in both production and test code - Addition of
psutilversion 7.1.3 as a dev dependency in pyproject.toml
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/tests/test_backup_multi_bucket.py | Added conditional import of exceptiongroup for Python < 3.11 |
| python/bucketbase/backup_multi_bucket.py | Added conditional import of exceptiongroup for Python < 3.11 |
| python/pyproject.toml | Added psutil version 7.1.3 as a dev dependency |
| python/poetry.lock | Updated lock file with psutil 7.1.3 package details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from pathlib import Path, PurePosixPath | ||
| from typing import BinaryIO, Iterable | ||
|
|
||
| from exceptiongroup import ExceptionGroup |
|
|
||
| import psutil | ||
| from exceptiongroup import ExceptionGroup | ||
| if sys.version_info < (3, 11): |
+ fix the lock deletion on Py 3.12
![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
♻️ Duplicate comments (2)
.github/workflows/python-package.yml (1)
58-59: Same issue here – missing--no-updateflag.Same problem as the earlier job. This will cause dependency version drift in CI.
Apply this diff:
cd python - poetry lock + poetry lock --no-update poetry install --with devpython/bucketbase/backup_multi_bucket.py (1)
6-9: Same suggestion: simplify the ExceptionGroup availability check.The
ExceptionGroup.__class__check is unnecessarily verbose. Consider the simpler alternatives suggested in the test file.Option 1 (cleaner):
try: - _ = ExceptionGroup.__class__ # pylint: disable=used-before-assignment + ExceptionGroup # pylint: disable=used-before-assignment except NameError: from exceptiongroup import ExceptionGroupOption 2 (most explicit):
import sys if sys.version_info < (3, 11): from exceptiongroup import ExceptionGroupBased on learnings: The try-except approach requested in previous review has been successfully implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
python/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/python-package.yml(2 hunks)python/bucketbase/backup_multi_bucket.py(1 hunks)python/bucketbase/file_lock.py(1 hunks)python/pyproject.toml(3 hunks)python/tests/test_backup_multi_bucket.py(1 hunks)
⏰ 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). (4)
- GitHub Check: build (macos-latest, 3.10)
- GitHub Check: build (macos-latest, 3.11)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.14)
🔇 Additional comments (3)
python/bucketbase/file_lock.py (1)
22-30: LGTM – proper resource cleanup ordering.Moving
super()._release()to the top ensures the parent class releases the lock before attempting platform-specific cleanup. This is the correct order for resource teardown.python/pyproject.toml (2)
16-16: Filelock constraint change now includes version 3.20.0.The constraint was loosened from
>3.20.0to>=3.20.0. This now allows filelock version 3.20.0 exactly, which was previously excluded. Make sure version 3.20.0 doesn't have any known issues you were trying to avoid.
32-32: The review comment is incorrect—psutil 7.1.3 exists.The PyPI data confirms that psutil 7.1.3 is the current latest stable release. The dependency specification in pyproject.toml is valid and will resolve without issues.
Likely an incorrect or invalid review comment.
| cd python | ||
| poetry lock |
There was a problem hiding this comment.
Missing --no-update flag as stated in the PR description.
The PR description explicitly mentions running poetry lock --no-update, but the workflow just runs poetry lock. Without the --no-update flag, Poetry will upgrade all dependencies to their latest compatible versions, which could introduce unexpected changes in CI. This is probably not what you want for reproducible builds.
Apply this diff:
cd python
- poetry lock
+ poetry lock --no-update
poetry install --with dev📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cd python | |
| poetry lock | |
| cd python | |
| poetry lock --no-update |
🤖 Prompt for AI Agents
.github/workflows/python-package.yml around lines 35 to 36: the workflow
currently runs "poetry lock" but the PR description requires "poetry lock
--no-update"; change the command to include the --no-update flag so Poetry
generates a lockfile without upgrading dependencies (i.e., replace the existing
poetry lock invocation with poetry lock --no-update).
| try: | ||
| _ = ExceptionGroup.__class__ | ||
| except NameError: | ||
| from exceptiongroup import ExceptionGroup |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
More idiomatic approach: simplify the ExceptionGroup check.
The current pattern _ = ExceptionGroup.__class__ is functional but overly complex. Just checking ExceptionGroup directly would raise NameError if it doesn't exist. Even better would be checking Python version explicitly.
Option 1 (simpler try-except):
-try:
- _ = ExceptionGroup.__class__
-except NameError:
+try:
+ ExceptionGroup
+except NameError:
from exceptiongroup import ExceptionGroupOption 2 (version check – most explicit):
import sys
if sys.version_info < (3, 11):
from exceptiongroup import ExceptionGroupBased on learnings: Previous review requested try-except approach, which has been implemented.
🤖 Prompt for AI Agents
In python/tests/test_backup_multi_bucket.py around lines 13 to 16, the current
check `_ = ExceptionGroup.__class__` is awkward; replace it with a simple
try/except that references ExceptionGroup directly (try: ExceptionGroup except
NameError: from exceptiongroup import ExceptionGroup) or, alternatively, perform
an explicit version check (import sys and if sys.version_info < (3, 11): from
exceptiongroup import ExceptionGroup) — pick one approach and update the file to
use that clearer, idiomatic pattern.
