Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/python-package.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ jobs:
run: |
python -m pip install --upgrade pip
python -m pip install poetry
cd python
poetry lock
Comment on lines +35 to +36
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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).

poetry install --with dev
- name: Run tests
env:
Expand All @@ -53,6 +55,8 @@ jobs:
run: |
python -m pip install --upgrade pip
python -m pip install poetry
cd python
poetry lock
poetry install --with dev
- name: Run tests in container
env:
Expand Down
5 changes: 4 additions & 1 deletion python/bucketbase/backup_multi_bucket.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import contextlib
import logging
import sys
from pathlib import Path, PurePosixPath
from typing import BinaryIO, Iterable

from exceptiongroup import ExceptionGroup
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amaximciuc
Do this rather with try...except

if sys.version_info < (3, 11):
from exceptiongroup import ExceptionGroup

from streamerate import slist, sset
from typing_extensions import override

Expand Down
35 changes: 34 additions & 1 deletion python/poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion python/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ requests = ">=2.25.1"
streamerate = ">=1.2.1"
pyxtension = ">=1.17.1"
StrEnum = ">=0.4.0"
filelock= ">3.20.0"
filelock = ">=3.20.0"
minio = { version = ">=7.2.4,<=8.0.0", optional = true }
multiminio = { version = ">=1.0.2", optional = true }

Expand All @@ -29,6 +29,7 @@ pyarrow = ">=21.0.0"
twine = ">=6.1.0"
pre-commit = ">=4.3.0"
mypy = "^1.18.2"
psutil = "^7.1.3"

[tool.black]
line-length = 160
Expand Down
5 changes: 4 additions & 1 deletion python/tests/test_backup_multi_bucket.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# mypy: disable-error-code="no-untyped-def"
import gc
import os
import sys
import tempfile
import threading
from io import BytesIO
Expand All @@ -9,7 +10,9 @@
from unittest import TestCase

import psutil
from exceptiongroup import ExceptionGroup
if sys.version_info < (3, 11):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use try..except instead

from exceptiongroup import ExceptionGroup
Comment on lines +13 to +16
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 ExceptionGroup

Option 2 (version check – most explicit):

import sys

if sys.version_info < (3, 11):
    from exceptiongroup import ExceptionGroup

Based 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.


from minio import Minio

from bucketbase import MemoryBucket, MinioBucket, ShallowListing
Expand Down
Loading