Closed eimrek closed 7 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
23c784a
) 99.90% compared to head (34bb0de
) 99.90%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I think that not capturing the output of rsync is OK as the best solution among those you describe, but happy to hear if @sphuber also agrees or not
@sphuber This should be good now, give it a quick check if you have time.
To summarize the discussion @eimrek and myself had: ideally the progress of rsync also goes through the logging system as then it can be fully controlled by the caller, e.g. aiida-core
. However, the current progress reporting mechanism in aiida-core
doesn't even go through the logging, so it doens't make sense to invest the time now to do it here. So for now, we will go simply with letting rsync's progress go uncaptured straight to stdout
Hi,
this PR contains two changes:
1) remove the logger argument from BackupManager, and instead only use the logger object of this library.
This change is motivated by the fact that in aiida-core, the default logging level is REPORT (23) and the logger.info messages of this library are not outputted, which give information about the progress and should probably be shown by default. After these changes, the logger instead should be controlled in the aiida-core via something like
Although, perhaps there is a better way to do it, similarly to the other loggers (as is done in aiida.common.log.py).
In principle, I can also revert to using the logger argument, but then i probably should also create a custom log level called REPORT in this library, if i want to log consistently with aiida-core.
2) Enable rsync progress indication (with
--info=progress2
) and output the stdout directly to the user, without capturing itIf i call
subprocess.run(rsync_args, capture_output=False)
, the stdout of rsync is passed directly from the subprocess to the user, bypassing any logging system. Although this has drawbacks (e.g. it won't show up in logs, e.g. when redirecting to a file), it allows to show the updating progress indicator to the user running the CLI sees. The output is captured and not shown, however, if the logging level is above INFO. The motivation behind why I just bypass capturing the output is that if the stdout is captured, it's not possible (i think; or at least not without writing a lot of code) to output a in-place updating progress indicator via the logging system.As this has the drawback, I think it would be good to discuss and get opinions (@giovannipizzi @sphuber).
Two possible alternatives would be