Support for progress of running backup & restore jobs

[devmato]

First of all thank you for the excellent library. I'm using it to develop a web based centralized backup solution.
One thing I'm currently missing is the possibility to get the progress of a running backup, or restore job.

The restic binary is printing regular progress updates to stdout when running the backup command with the --json argument. Maybe this could be used to invoke a callback function, whenever a new progress update appears.

Are there any plans for something like this?

[mtlynch]

Oh, this is a cool idea!

This is not something I'm planning to implement, but I'd be open to it if someone wants to contribute.

[mtlynch]

@darkpasiu

I’d like to suggest an enhancement that would allow progress logging for long-running commands, like restic backup. Currently, with the existing approach, there’s no way to see the progress of these operations as they happen.

The idea behind my previous pull request was to enable streaming of stdout with throttled logging, which would allow for live updates on the backup progress without overwhelming the log with excessive output. This would provide users with real-time feedback, especially for long-running commands that report progress.

I like the idea, but resticpy is a library rather than a console app, so it's ugly to print directly to stdout. It should be the consuming app's decision what to do with the output and how quickly to present it to the user. For example, if I was building a web app on top of resticpy and wanted to stream progress to the user, but resticpy just dumped everything to stdout, the user wouldn't be able to see any of that in their browser.

I'm open to a suggestion where the consuming app gets to decide how to process the output and an example in the unit test or e2e tests that shows how to do it.

[darkpasiu]

@mtlynch:

Thanks for the clarification - that makes sense, and I agree that resticpy shouldn’t decide how or where output is presented.

Instead of printing or logging directly, I can rework the approach to expose a streaming interface via callbacks (e.g. on_stdout / on_stderr). This would allow the consuming application to decide how to handle, throttle, or forward progress output, whether it’s a CLI, a web app, or something else.

Throttling would live entirely in the consumer’s callback, not in resticpy itself.

Below is an example of what I have in mind.

Example API:

def backup(
    self,
    *,
    on_stdout: Callable[[str], None] | None = None,
    on_stderr: Callable[[str], None] | None = None,
    **kwargs,
):
    command = self._build_backup_command(**kwargs)
    return self._run_command(
        command,
        on_stdout=on_stdout,
        on_stderr=on_stderr,
    )

example implementation:

import subprocess

def _run_command(self, command, on_stdout=None, on_stderr=None):
    process = subprocess.Popen(
        command,
        stdout=subprocess.PIPE,
        stderr=subprocess.PIPE,
        text=True,
        bufsize=1,
    )

    if process.stdout:
        for line in process.stdout:
            if on_stdout:
                on_stdout(line.rstrip())

    if process.stderr:
        for line in process.stderr:
            if on_stderr:
                on_stderr(line.rstrip())

    return process.wait()

Example consumer usage (with throttling):

import time
import logging

logger = logging.getLogger(__name__)
last_emit = 0

def throttled_progress(line: str):
    global last_emit
    now = time.time()
    if now - last_emit > 1:
        logger.info(line)
        last_emit = now

repo.backup(on_stdout=throttled_progress)

[mtlynch]

Sure, that implementation looks good to me. I'm ready to move on to a PR, but we need to include e2e tests and unit tests, as this is a big change.

[darkpasiu]

I need a bit more time to prepare a draft of the implementation. I will try to have something ready by next week (hopefully!) :)

[darkpasiu]

@mtlynch: I wanted to get your thoughts on this approach: #212.
It’s currently a draft, but I wanted to see if you’re okay with the general direction.

[darkpasiu]

@mtlynch: PR #212 was closed. I’m wondering how to proceed with adding this feature. Was the implementation too complex, and is that why it was rejected? If you have any additional comments, please share them - I will try to adjust my code to match the desired approach.

[mtlynch]

@darkpasiu - I think we're too far apart on style and design preferences. PR #212 is pretty different in design and style from the rest of the project, and the description didn't match my guidelines for a clear PR description.

I do appreciate the work you're putting in, but I unfortunately don't have time to help match it to the project's existing conventions at the moment.

I don't see a way of proceeding with you on this work.

[darkpasiu]

Hi @mtlynch,

Thanks for your feedback - I really appreciate you taking the time to share your perspective.

I wanted to ask whether you’d be open to providing some high-level guidelines or expectations on how you’d like this change to fit into resticpy’s existing style and design. Even a brief direction on what to follow or avoid would help a lot.

If you feel you might have time to offer some feedback on a revised PR, I’d be very happy to try again and put extra effort into aligning both the implementation and the PR description with your conventions.

At the same time, I want to be respectful of your time. If that’s not feasible, I’d appreciate a candid answer so I can decide whether to proceed with a fork for my own use.

I’d really like to see this feature in the official project so more users can benefit from it, but I completely understand if that’s not possible right now.

Thanks again for your work on resticpy.

[mtlynch]

@darkpasiu - Sure, I appreciate you being open to feedback.

I think the best path forward would be to start with a PR that's smaller in scope. 350 LOC is on the medium high side, so if we're not already matching in terms of approach, it's a difficult place to start. Is there a feature / bug you'd be interested in fixing that's smaller in scope so we can calibrate on style and approach?

My high-level note is to look more closely at existing convention and try to match the style in the surrounding code and the project overall.

For example, when I read this part of the PR, the thing that sticks out to me is that the test adds comments on a lot of lines that are identical across tests but the other tests don't have comments attached. If this were the first test in the file, it's fine to err on the side of too many comments, but if it's adding to an existing file, it's more important to match existing conventions. Otherwise, after the PR is merged, the reader wonders why just one function has these comments and the rest don't. Which is correct?

Comments are fine and helpful, but in that file, I'd limit them to just the things that are unique and difficult to infer about that particular test case.

Also, the dominant convention in the project is that comments have trailing punctuation, whereas in that test, they're missing punctuation (but other parts of the PR have trailing punctuation).

Comment style isn't the most important thing, but it's just the first thing that stands out to me as inconsistent in the PR.

I also noticed that the PR uses type hints, which is something the rest of the project doesn't use. I'm not strictly against that, but it's one of those things we should take on consciously rather than just incidentally while trying to solve a different problem.

I generally try to follow Google's Python Style Guide, though I didn't document that properly until yesterday (#214).

[darkpasiu]

I’ve created a new PR #216 with a minimal set of changes, and I hope this will make the review easier.
There are no new tests in this PR either - I wanted to avoid creating another large PR, so the idea was first to get your opinion on the logging display mechanism itself. Once we agree on the approach, I’ll extend it with the required tests.

I also tried to remove typing, leaving it only in the dataclass, and I removed most of the comments as well.

Please let me know what you think about these changes - I’m open to any comments or suggestions. If something doesn’t look right, please don’t close the PR; just leave a comment and I’ll adjust the code accordingly 🙂

[mtlynch]

I’ve created a new PR #216 with a minimal set of changes, and I hope this will make the review easier.

Sorry, when I said a PR that's smaller in scope, I meant to try a different bugfix or feature. This feature affects too much of resticpy.

Also, please fix automated test failures before submitting to review. Their purpose is to save the maintainer time by catching things automatically that the maintainer would otherwise have to flag manually.