Skip to content

Conversation

@Alex-Burmak
Copy link
Owner

@Alex-Burmak Alex-Burmak commented May 8, 2025

Summary by Sourcery

Add support for Python 3.13 and update project dependencies and configurations

New Features:

  • Add Python 3.13 support to the project

Bug Fixes:

  • Fix thread termination handling in pipeline run method
  • Update file handling in lock manager
  • Improve Docker Compose command detection

Enhancements:

  • Update project dependencies to be compatible with Python 3.13
  • Modify GitHub workflow to include Python 3.13 testing
  • Update various code snippets for Python 3.13 compatibility

Chores:

  • Remove support for older Python versions (3.7 and 3.8)
  • Update ClickHouse version matrix in CI workflow
  • Adjust import and code structure for Python 3.13

Summary by CodeRabbit

  • New Features

    • Expanded test matrix to include Python 3.13 and updated ClickHouse versions for improved compatibility.
  • Bug Fixes

    • Improved exception handling in pipeline execution to address known issues with thread termination errors.
  • Chores

    • Updated dependency version constraints and compatibility for Python 3.9+.
    • Refined development and runtime dependencies for better support of newer Python versions.
    • Updated configuration files and minimum Python version requirements.
    • Improved Docker Compose command detection in integration tests.
  • Refactor

    • Simplified resource management in lock handling.
    • Minor code style and import order adjustments throughout the codebase and tests.

@sourcery-ai
Copy link

sourcery-ai bot commented May 8, 2025

Reviewer's Guide

This pull request implements Python 3.13 support by updating dependencies with conditional logic (requirements.txt), adapting the CI workflow for Python 3.13 testing (.github/workflows/workflow.yml), and introducing a workaround for a stopit library issue in the async pipeline (ch_backup/storage/async_pipeline/pipelines.py). It also modernizes Docker Compose usage in tests (tests/integration/modules/compose.py) and applies various code and configuration refinements, such as flock usage improvements and style updates.

File-Level Changes

Change Details Files
Updated dependencies for Python 3.13 compatibility.
  • Modified version constraints for requests, boto3, and botocore, introducing Python 3.13 specific conditions.
  • Added legacy-cgi>=2.6 as a dependency for Python 3.13 and newer.
  • Added packaging as a new dependency.
  • Adjusted setuptools version condition for Python 3.12 and newer.
  • Removed specific version pinning for dataclasses and typing_extensions.
requirements.txt
Integrated Python 3.13 into the CI testing pipeline.
  • Added Python 3.13 to the GitHub Actions test matrix.
  • Removed Python 3.7 and 3.8 from the test matrix.
  • Updated various ClickHouse versions used in the test matrix.
.github/workflows/workflow.yml
Added error handling for an issue in an async pipeline dependency.
  • Implemented a try-except block in the run function to catch and log a specific ValueError ("Invalid thread ID") from the stopit library, preventing it from halting execution.
ch_backup/storage/async_pipeline/pipelines.py
Improved Docker Compose command flexibility in integration tests.
  • Modified test utility to dynamically use docker-compose or docker compose based on system availability.
tests/integration/modules/compose.py
Applied various code style updates, configuration adjustments, and minor refactorings.
  • Refactored flock usage in LockManager to operate on file objects directly instead of file descriptors.
  • Adopted parenthesized context managers for opening multiple files in unit tests.
  • Standardized import statement order in several test modules.
  • Updated Makefile codespell targets to specify source and test directories and markdown files.
  • Simplified .isort.cfg by removing src_paths and known_first_party settings.
  • Set python_version = 3.9 in mypy.ini.
  • Applied minor syntactic updates such as adding trailing commas in function signatures and calls.
ch_backup/logic/lock_manager.py
tests/unit/test_pipeline.py
tests/unit/test_backup_tables.py
Makefile
tests/unit/test_access.py
tests/unit/test_clickhouse_encryption.py
tests/unit/test_schema.py
tests/unit/test_upload_part_observer.py
tests/unit/test_disks.py
.isort.cfg
mypy.ini
ch_backup/storage/async_pipeline/base_pipeline/exec_pool.py
ch_backup/storage/async_pipeline/pipeline_builder.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link

coderabbitai bot commented May 8, 2025

Walkthrough

This update refines Python version compatibility and dependency management across configuration, requirements, and workflow files. It adjusts import sources for version parsing, improves test and Makefile scripts, and streamlines code style. Exception handling is enhanced in a pipeline runner, and resource management is simplified in a lock manager. Minor formatting and import order changes are made throughout tests.

Changes

File(s) Change Summary
.github/workflows/workflow.yml Updated test matrix: removed Python 3.7/3.8 with ClickHouse "latest", updated ClickHouse versions for Python 3.10, added Python 3.10/3.13 entries, rest unchanged.
.isort.cfg, mypy.ini, .pylintrc, debian/control Updated/simplified config: set Python version to 3.9, removed unnecessary settings, updated minimum Python version.
Makefile codespell and fix-codespell-errors now explicitly target source, tests, and Markdown files.
requirements.txt, requirements-dev.txt Refined version constraints and Python-version-specific dependencies for compatibility; added packaging, adjusted/removes some conditional dependencies.
ch_backup/clickhouse/control.py, tests/integration/modules/clickhouse.py, tests/integration/modules/utils.py Changed version parsing import from pkg_resources to packaging.version.
ch_backup/logic/lock_manager.py Removed _fd attribute from LockManager, now uses file object _file for resource management.
ch_backup/storage/async_pipeline/base_pipeline/exec_pool.py, ch_backup/storage/async_pipeline/pipeline_builder.py Added trailing commas to function signatures/calls for style consistency.
ch_backup/storage/async_pipeline/pipelines.py Added exception handling for "Invalid thread ID" in pipeline runner, with logging.
tests/integration/modules/compose.py Docker Compose command now dynamically uses docker-compose or docker compose based on system availability.
tests/integration/features/backup_replicated.feature Fixed YAML block indentation in test scenario.
tests/integration/modules/zookeeper.py, tests/integration/steps/ch_backup.py, tests/integration/steps/clickhouse.py, tests/integration/steps/common.py, tests/integration/steps/s3.py, tests/integration/steps/zookeeper.py Added blank lines for code style/PEP8 compliance.
tests/unit/test_access.py, tests/unit/test_cli.py, tests/unit/test_clickhouse_encryption.py, tests/unit/test_control.py, tests/unit/test_disks.py, tests/unit/test_rate_limiter.py, tests/unit/test_retry_exponetial.py, tests/unit/test_schema.py, tests/unit/test_upload_part_observer.py Reordered import statements for style/consistency.
tests/unit/test_backup_tables.py, tests/unit/test_pipeline.py Refactored context manager syntax for combining multiple managers in with statements.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant PipelineRunner
    participant Logger

    User->>PipelineRunner: run(pipeline)
    PipelineRunner->>PipelineRunner: Create iterator from pipeline
    alt Normal Execution
        PipelineRunner->>PipelineRunner: Exhaust iterator
    else ValueError: "Invalid thread ID"
        PipelineRunner->>Logger: Log warning with stack trace
        PipelineRunner-->>User: Suppress exception
    else Other ValueError
        PipelineRunner-->>User: Raise exception
    end
Loading

Poem

🐇
A hop, a skip, a version bump,
Python grows, old configs slump.
Compose detects, exceptions caught,
Imports tidied, bugs are fought.
Blank lines bloom, dependencies align—
This rabbit’s code is looking fine!

Tip

⚡️ Faster reviews with caching
  • CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c0c9f6 and ac92594.

📒 Files selected for processing (2)
  • requirements-dev.txt (1 hunks)
  • requirements.txt (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • requirements-dev.txt
  • requirements.txt
⏰ Context from checks skipped due to timeout of 90000ms (11)
  • GitHub Check: test (Python 3.12, ClickHouse latest)
  • GitHub Check: test (Python 3.13, ClickHouse latest)
  • GitHub Check: test (Python 3.10, ClickHouse latest)
  • GitHub Check: test (Python 3.10, ClickHouse 24.3.18.7)
  • GitHub Check: test (Python 3.11, ClickHouse latest)
  • GitHub Check: test (Python 3.10, ClickHouse 25.3.3.42)
  • GitHub Check: test (Python 3.10, ClickHouse 22.8.21.38)
  • GitHub Check: test (Python 3.10, ClickHouse 23.8.16.40)
  • GitHub Check: test (Python 3.10, ClickHouse 23.3.22.3)
  • GitHub Check: test (Python 3.10, ClickHouse 24.8.14.39)
  • GitHub Check: test (Python 3.9, ClickHouse latest)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 8, 2025

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Alex-Burmak - I've reviewed your changes - here's some feedback:

  • The legacy-cgi dependency, added for 'boto' compatibility, may indicate reliance on the original boto library; consider planning its upgrade or replacement if so.
  • Update mypy.ini's python_version from 3.9 to a more recent version (e.g., 3.13) to improve static type checking accuracy for newer Python features.
  • Verify isort's first-party module detection after .isort.cfg changes, possibly by migrating its configuration to pyproject.toml.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

with open(original_file_path, "rb") as orig_fobj, open(
backward_file_name, "rb"
) as res_fobj:
with (
Copy link

Choose a reason for hiding this comment

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

suggestion (testing): Consider adding a test for the 'Invalid thread ID' ValueError handling in pipelines.run.

Mock pypeln.utils.exhaust_iterator (or the relevant internal function) to raise the 'Invalid thread ID' ValueError, then verify that pipelines.run logs a warning and suppresses the exception.

@Alex-Burmak
Copy link
Owner Author

@CodiumAI-Agent /review

@QodoAI-Agent
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Flock usage

The call to flock(self._file, LOCK_SH) may be incorrect since flock expects a file descriptor (int), not a file object. Verify that flock is imported correctly and consider using self._file.fileno().

if not os.path.exists(self._process_lockfile_path):
    with open(self._process_lockfile_path, "w+", encoding="utf-8"):
        pass
self._file = open(self._process_lockfile_path, "r", encoding="utf-8")
flock(self._file, LOCK_SH)
Broad exception catch

Catching all ValueError instances may mask unrelated errors. Narrow the except clause or re-raise only when the message matches the known "Invalid thread ID".

try:
    itr = iter(pipeline)
    exhaust_iterator(itr)
except ValueError as e:
    # The library "stopit" (dependency of "pypeln") as of the version 1.1.2, incorrectly handle the case when
    # timeout exception is sent to already terminated thread. It's incorrectly interpreted as an internal error.
    # https://github.com/glenfant/stopit/blob/dda4bd181d1d29ab1fb22314dc9bde0e3c931abc/src/stopit/threadstop.py#L37
    if "Invalid thread ID" in str(e):
        logging.warning(
            "Thread ID error due to incorrect handling of thread termination in stopit library, skipping",
            exc_info=True,
        )
    else:
        raise
Unused kwargs

The submit method signature now accepts **kwargs, but the implementation shown does not forward them. Ensure that these keyword arguments are passed through to the underlying execution call.

    job_id: str,
    func: Callable,
    *args: Any,
    callback: Optional[Callable] = None,
    **kwargs: Any,
) -> None:

@Alex-Burmak
Copy link
Owner Author

@CodiumAI-Agent /improve

@QodoAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Use fileno for flock call

The flock call expects a file descriptor integer, not a file object. Use the file’s
fileno() method to pass the correct type.

ch_backup/logic/lock_manager.py [112]

-flock(self._file, LOCK_SH)
+flock(self._file.fileno(), LOCK_SH)
Suggestion importance[1-10]: 9

__

Why: Passing the file object to flock will raise a TypeError at runtime; using self._file.fileno() ensures the correct integer descriptor is used for locking.

High
Security
Use argument list for subprocess

Building a shell string is prone to injection and quoting issues. Construct a list
of arguments and call subprocess with shell=False for better safety and reliability.

tests/integration/modules/compose.py [191-194]

-shell_command = f"{docker_compose} --file {_config_path(conf)}"
+args = [docker_compose, "--file", _config_path(conf)]
 if project_name:
-    shell_command += f" -p {project_name}"
-shell_command += f" {command}"
+    args.extend(["-p", project_name])
+args.append(command)
+subprocess.run(args, check=True)
Suggestion importance[1-10]: 6

__

Why: Building the Docker Compose command as a list avoids shell injection and quoting pitfalls, although it requires adjusting the actual subprocess call in the surrounding context.

Low
General
Guard and close file safely

Ensure _file exists and is open before closing to prevent attribute or state errors.
Add a guard checking for the attribute and its closed state.

ch_backup/logic/lock_manager.py [96-97]

-if self._lock_conf.get("flock"):
+if self._lock_conf.get("flock") and getattr(self, "_file", None) and not self._file.closed:
     self._file.close()
Suggestion importance[1-10]: 5

__

Why: Adding a check for the existence and open state of _file prevents attribute errors or attempting to close an already closed file, improving robustness.

Low

@Alex-Burmak
Copy link
Owner Author

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link

Title

Python 3.13 support


User description

Summary by Sourcery

Add support for Python 3.13 and update project dependencies and configurations

New Features:

  • Add Python 3.13 support to the project

Bug Fixes:

  • Fix thread termination handling in pipeline run method
  • Update file handling in lock manager
  • Improve Docker Compose command detection

Enhancements:

  • Update project dependencies to be compatible with Python 3.13
  • Modify GitHub workflow to include Python 3.13 testing
  • Update various code snippets for Python 3.13 compatibility

Chores:

  • Remove support for older Python versions (3.7 and 3.8)
  • Update ClickHouse version matrix in CI workflow
  • Adjust import and code structure for Python 3.13

Summary by CodeRabbit

  • New Features

    • Expanded test matrix to include Python 3.13 and updated ClickHouse versions for improved compatibility.
  • Bug Fixes

    • Improved exception handling in pipeline execution to address known issues with thread termination errors.
  • Chores

    • Updated dependency version constraints and compatibility for Python 3.9+.
    • Refined development and runtime dependencies for better support of newer Python versions.
    • Updated configuration files and minimum Python version requirements.
    • Improved Docker Compose command detection in integration tests.
  • Refactor

    • Simplified resource management in lock handling.
    • Minor code style and import order adjustments throughout the codebase and tests.

PR Type

Enhancement, Bug fix


Description

  • Add Python 3.13 support and CI tests

  • Update dependency version constraints

  • Replace pkg_resources.parse_version usage

  • Fix file lock close and pipeline error


Changes walkthrough 📝

Relevant files
Enhancement
4 files
control.py
Use packaging.parse_version instead of pkg_resources         
+1/-1     
clickhouse.py
Replace pkg_resources.parse_version import                             
+1/-1     
compose.py
Detect docker-compose binary with shutil.which                     
+7/-2     
utils.py
Replace pkg_resources.parse_version import                             
+1/-1     
Bug fix
2 files
lock_manager.py
Close lock file instead of os.close descriptor                     
+2/-4     
pipelines.py
Add pipeline run error handling for stopit                             
+15/-2   
Formatting
20 files
exec_pool.py
Add trailing comma to **kwargs signature                                 
+1/-1     
pipeline_builder.py
Append comma after stages unpacking                                           
+1/-1     
zookeeper.py
Remove unused blank line import cleanup                                   
+1/-0     
ch_backup.py
Add blank line before BackupManager import                             
+1/-0     
clickhouse.py
Add blank line before BackupManager import                             
+1/-0     
common.py
Add blank line before get_version import                                 
+1/-0     
s3.py
Add blank line before modules imports                                       
+1/-0     
zookeeper.py
Add blank line before step imports                                             
+1/-0     
test_access.py
Move parametrize import to bottom                                               
+1/-2     
test_backup_tables.py
Convert patch context to with tuple style                               
+3/-2     
test_cli.py
Remove and re-add parametrize import                                         
+1/-2     
test_clickhouse_encryption.py
Move utils import to bottom of file                                           
+1/-2     
test_control.py
Move parametrize import below tested module                           
+1/-2     
test_disks.py
Move parametrize import to end                                                     
+1/-1     
test_pipeline.py
Use with tuple for file opens                                                       
+4/-3     
test_rate_limiter.py
Swap import of TimeMocker                                                               
+1/-1     
test_retry_exponetial.py
Swap import of TimeMocker                                                               
+1/-1     
test_schema.py
Move parametrize import below tested code                               
+1/-2     
test_upload_part_observer.py
Move parametrize import to bottom                                               
+1/-2     
backup_replicated.feature
Fix indentation in scenario outline                                           
+2/-2     
Configuration changes
6 files
workflow.yml
Add Python 3.13 to CI matrix, remove 3.7/3.8                         
+6/-6     
.isort.cfg
Remove custom src_paths and known_first_party                       
+0/-2     
.pylintrc
Bump minimum Python version to 3.9                                             
+1/-1     
Makefile
Include paths in codespell commands                                           
+2/-2     
control
Update Build-Depends to Python >= 3.9                                       
+1/-1     
mypy.ini
Set mypy python_version to 3.9                                                     
+1/-0     
Dependencies
2 files
requirements-dev.txt
Simplify dev dependencies and version markers                       
+17/-15 
requirements.txt
Update runtime dependencies for Python 3.13                           
+7/-6     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @Alex-Burmak Alex-Burmak force-pushed the main branch 7 times, most recently from 6c1cd87 to 6b13808 Compare May 18, 2025 15:14
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants