snapm: fix "Computing diffs" progress bar flicker and at move detector progress #775
Conversation
bmr-cymru
added
enhancement
WalkthroughCompute_diff and _detect_moves now accept an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DiffEngine
participant TermControl
participant Progress
Client->>DiffEngine: compute_diff(tree_a, tree_b, options, term_control)
DiffEngine->>DiffEngine: generate main diffs (compare trees)
Note over DiffEngine: main diff phase completes
alt move-detection required
DiffEngine->>TermControl: request progress "Detecting moves"
TermControl->>Progress: create/show progress
DiffEngine->>DiffEngine: _detect_moves(diffs, tree_a, tree_b, options, term_control)
loop per candidate path
DiffEngine->>Progress: progress.update(item)
Progress->>Progress: check elapsed vs interval (FPS)
alt interval elapsed
Progress->>Client: render update
end
DiffEngine->>DiffEngine: check source/dest existence, record moved_from/moved_to
end
Progress->>Progress: progress.cancel()/end (FPS-aware finalisation)
DiffEngine->>DiffEngine: prune move-related diffs, record moves count
end
DiffEngine->>Client: return FsDiffResults
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-10-22T20:06:54.993ZApplied to files:
📚 Learning: 2025-12-10T18:28:51.721ZApplied to files:
📚 Learning: 2025-12-14T12:52:14.459ZApplied to files:
⏰ 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). (41)
🔇 Additional comments (8)
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 |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
snapm/fsdiff/engine.py (1)
1099-1112: Consider checkingtree_abefore creating the progress bar.The progress bar is created (lines 1099–1104), but if
tree_ais empty, the early return on line 1108 skipsprogress.start()on line 1112. Whilst this doesn't cause errors, it's inefficient to construct the progress object when it won't be used.🔎 Apply this diff to check tree_a earlier:
+ # Anything to do? + if not tree_a: + return diffs + term_control = term_control or TermControl() progress = ProgressFactory.get_progress( "Detecting moves", quiet=options.quiet, term_control=term_control, ) - # Anything to do? - if not tree_a: - return diffs - moves = 0 # Count of move records start_time = datetime.now() progress.start(len(tree_a))snapm/progress.py (1)
629-663: Verify FPS limiting correctly throttles updates.The FPS-based guarding logic correctly limits progress bar updates. The implementation:
- Stores state (
done,_last_message) for cancellation- Checks elapsed time against the interval
- Outputs only when sufficient time has elapsed
Note:
prefixcalculation (lines 635-639) occurs before the FPS check, meaning it's computed even when updates are skipped. This is a minor inefficiency but doesn't affect correctness.🔎 Optional optimization: move prefix calculation inside the FPS guard
message = message or "" percent = float(done) / float(self.total) n = int((self.width - 10) * percent) # Used by cancel() self.done = done # Stash last message in case we need it later. self._last_message = message + now = datetime.now() + if (now - self._last).total_seconds() * _USECS_PER_SEC >= self._interval_us: + self._last = now + if self.first_update: prefix = self.term.HIDE_CURSOR + self.term.BOL self.first_update = False else: prefix = 2 * (self.term.BOL + self.term.UP + self.term.CLEAR_EOL) if len(message) > self.budget: message = message[0 : self.budget - 3] + "..." - now = datetime.now() - if (now - self._last).total_seconds() * _USECS_PER_SEC >= self._interval_us: - self._last = now print( prefix + ( self.pbar % ( self.header, percent * 100, self.did * n, self.todo * (self.width - 10 - n), ) ) + self.term.CLEAR_EOL + message + "\n", file=self.stream, end="", ) _flush_with_broken_pipe_guard(self.stream)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapm/fsdiff/engine.py(6 hunks)snapm/progress.py(7 hunks)tests/test_progress.py(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-10T18:28:51.721Z
Learning: In the snapshotmanager/snapm codebase, `TreeWalker.walk_tree()` in `snapm/_fsdiff/treewalk.py` eagerly materializes all paths before processing (using `os.walk`) to compute the total count for progress reporting. This design is intentional and necessary because accurate progress updates require knowing the total upfront. The custom `snapm._progress` implementation is lightweight (~2000 lines with tests) and RHEL-compatible, unlike alternatives like `tqdm` (>8000 lines, not in RHEL). The eager materialization trade-off is well-justified for the UX benefit of progress reporting on large trees.
📚 Learning: 2025-12-14T12:52:14.459Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 707
File: snapm/fsdiff/engine.py:327-405
Timestamp: 2025-12-14T12:52:14.459Z
Learning: In snapm/fsdiff/engine.py, render_unified_diff intentionally uses tc.WHITE instead of tc.NORMAL to reset colors after diff lines. This avoids breaking less -R output when piping through files, and is intended to support --color=always with less -R. When reviewing changes in this file, ensure any color reset logic preserves compatibility with downstream pagers/filters; if you introduce color resets, test with --color=always and piping to less -R. If you modify reset color behavior, consider updating tests and documenting rationale.
Applied to files:
snapm/fsdiff/engine.py
🧬 Code graph analysis (1)
snapm/fsdiff/engine.py (5)
snapm/progress.py (7)
TermControl(40-277)progress(430-440)ProgressFactory(1192-1342)get_progress(1198-1269)end(453-465)end(947-960)end(1184-1189)snapm/fsdiff/treewalk.py (2)
_log_debug_fsdiff(42-44)is_file(257-265)snapm/fsdiff/tree.py (1)
_log_debug_fsdiff(32-34)snapm/fsdiff/changes.py (2)
_log_debug_fsdiff(29-31)detect_changes(376-525)snapm/fsdiff/difftypes.py (1)
DiffType(14-23)
⏰ 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). (34)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
🔇 Additional comments (10)
snapm/fsdiff/engine.py (3)
813-815: Documentation updated correctly.The docstring accurately reflects the new
term_controlparameter.
1024-1024: Documentation updated correctly.The signature and docstring accurately reflect the new
term_controlparameter.Also applies to: 1039-1041
1114-1177: Well-structured move detection with progress feedback.The loop provides per-item progress updates and includes appropriate exception handling for keyboard interrupts and system exits.
tests/test_progress.py (2)
15-18: LGTM! Imports align with FPS limiting changes.The addition of
timeand the import ofDEFAULT_FPS(renamed fromDEFAULT_THROBBER_FPS) correctly support the new FPS-based rate limiting tests.
255-256: LGTM! Sleep calls correctly honour FPS limiting.The
time.sleep(1 / DEFAULT_FPS)calls ensure sufficient time elapses between progress updates for the FPS limiting logic to take effect. This is necessary for reliable test behaviour.Also applies to: 268-269, 283-284, 325-326, 338-339, 353-354
snapm/progress.py (5)
34-34: LGTM! Constant rename broadens applicability.Renaming
DEFAULT_THROBBER_FPStoDEFAULT_FPSis appropriate since the constant is now used for bothProgressandThrobberclasses.
585-589: LGTM! FPS tracking attributes properly initialised.The new attributes correctly support FPS-based rate limiting:
fpsand_interval_usdefine the update interval_lasttracks the last update time (initialised in_do_start)_last_messageanddonesupport cancellation behaviour
614-614: LGTM! Initial timing state ensures first update displays.Setting
_lasttonow - intervalguarantees the firstprogress()call will immediately display, which is correct behaviour.
877-877: LGTM! ThrobberBase updated to use renamed constant.Correctly uses
DEFAULT_FPSinstead of the oldDEFAULT_THROBBER_FPS.
773-798: Flag inconsistency: SimpleProgress lacks FPS limiting mentioned in summary.The AI-generated summary states: "In SimpleProgress._do_start and SimpleProgress._do_progress, mirrors timing/state enhancements and implements the same guarded output behavior."
However, the code shows no FPS limiting changes in
SimpleProgress. The class still outputs on every_do_progress()call without rate limiting. IfSimpleProgressis also subject to high-frequency updates causing terminal flooding, it should receive similar FPS limiting logic.Please verify whether
SimpleProgressshould also implement FPS-based rate limiting. If high-frequencyprogress()calls occur withSimpleProgress, it may exhibit similar terminal flooding behaviour.
bmr-cymru
force-pushed
the
bmr-flicker-fix-and-move-progress
branch
from
0ef074a to
963a416
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snapm/progress.py (2)
629-664: Flush could be moved inside the conditional block for efficiency.The
_flush_with_broken_pipe_guard(self.stream)call on line 664 executes on everyprogress()call, even when no output is rendered due to FPS throttling. Moving it inside the conditional would avoid unnecessary flush syscalls.That said, this is a minor optimisation and the current behaviour is functionally correct.
🔎 Optional diff:
if (now - self._last).total_seconds() * _USECS_PER_SEC >= self._interval_us: self._last = now print( prefix + ( self.pbar % ( self.header, percent * 100, self.did * n, self.todo * (self.width - 10 - n), ) ) + self.term.CLEAR_EOL + message + "\n", file=self.stream, end="", ) - _flush_with_broken_pipe_guard(self.stream) + _flush_with_broken_pipe_guard(self.stream)
1345-1356: Consider addingDEFAULT_FPSto__all__.
DEFAULT_FPSis imported by the test suite and represents a public constant that external consumers might wish to reference (e.g., for coordinating timing in custom progress implementations). Adding it to__all__would make the public API explicit.🔎 Suggested addition:
__all__ = [ + "DEFAULT_FPS", "NullProgress", "NullThrobber", "Progress", "ProgressBase", "ProgressFactory", "SimpleProgress", "SimpleThrobber", "TermControl", "ThrobberBase", "Throbber", ]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
snapm/fsdiff/engine.py(5 hunks)snapm/fsdiff/tree.py(1 hunks)snapm/progress.py(7 hunks)tests/test_progress.py(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-10T18:28:51.721Z
Learning: In the snapshotmanager/snapm codebase, `TreeWalker.walk_tree()` in `snapm/_fsdiff/treewalk.py` eagerly materializes all paths before processing (using `os.walk`) to compute the total count for progress reporting. This design is intentional and necessary because accurate progress updates require knowing the total upfront. The custom `snapm._progress` implementation is lightweight (~2000 lines with tests) and RHEL-compatible, unlike alternatives like `tqdm` (>8000 lines, not in RHEL). The eager materialization trade-off is well-justified for the UX benefit of progress reporting on large trees.
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/fsdiff/engine.py
📚 Learning: 2025-12-10T18:28:51.721Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-10T18:28:51.721Z
Learning: In the snapshotmanager/snapm codebase, `TreeWalker.walk_tree()` in `snapm/_fsdiff/treewalk.py` eagerly materializes all paths before processing (using `os.walk`) to compute the total count for progress reporting. This design is intentional and necessary because accurate progress updates require knowing the total upfront. The custom `snapm._progress` implementation is lightweight (~2000 lines with tests) and RHEL-compatible, unlike alternatives like `tqdm` (>8000 lines, not in RHEL). The eager materialization trade-off is well-justified for the UX benefit of progress reporting on large trees.
Applied to files:
snapm/fsdiff/engine.py
📚 Learning: 2025-12-14T12:52:14.459Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 707
File: snapm/fsdiff/engine.py:327-405
Timestamp: 2025-12-14T12:52:14.459Z
Learning: In snapm/fsdiff/engine.py, render_unified_diff intentionally uses tc.WHITE instead of tc.NORMAL to reset colors after diff lines. This avoids breaking less -R output when piping through files, and is intended to support --color=always with less -R. When reviewing changes in this file, ensure any color reset logic preserves compatibility with downstream pagers/filters; if you introduce color resets, test with --color=always and piping to less -R. If you modify reset color behavior, consider updating tests and documenting rationale.
Applied to files:
snapm/fsdiff/engine.py
🧬 Code graph analysis (1)
snapm/fsdiff/engine.py (3)
snapm/progress.py (7)
TermControl(40-277)progress(430-440)ProgressFactory(1192-1342)get_progress(1198-1269)end(453-465)end(947-960)end(1184-1189)snapm/fsdiff/changes.py (2)
_log_debug_fsdiff(29-31)detect_changes(376-525)snapm/fsdiff/difftypes.py (1)
DiffType(14-23)
⏰ 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). (41)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (16)
snapm/fsdiff/tree.py (1)
181-181: LGTM!Good catch on the typo fix in the docstring.
snapm/fsdiff/engine.py (7)
813-815: LGTM!Docstring correctly documents the new
term_controlparameter forcompute_diff.
997-1001: LGTM!The
term_controlis now correctly passed to_detect_moves, ensuring consistent progress reporting between the diff computation and move detection phases.
1026-1046: LGTM!The updated signature and docstring for
_detect_movescorrectly document the newterm_controlparameter.
1101-1114: LGTM!The early return guard for empty
tree_ais appropriate, and the progress lifecycle is correctly initialised with the source tree size as the total.
1116-1172: LGTM!The move detection loop correctly reports progress for each source path and properly tracks the move count. The logic for selecting candidates and validating moves is well-structured.
1174-1179: LGTM!Exception handling is consistent with the pattern used in
compute_diff, ensuring proper progress bar cleanup on interruption.
1185-1187: LGTM!The progress message now correctly reports the
movescount rather thanlen(diffs), addressing the issue flagged in the previous review.tests/test_progress.py (3)
15-18: LGTM!The import changes correctly reference the renamed
DEFAULT_FPSconstant and add thetimemodule needed for the FPS-aware test delays.
255-284: LGTM!The
time.sleep(1 / DEFAULT_FPS)calls correctly account for the new FPS-limiting behaviour, ensuring sufficient time elapses between progress updates for the renders to occur.
325-354: LGTM!The FPS-aware delays in
test_lifecycle_no_clearmirror those intest_lifecycle, maintaining test consistency.snapm/progress.py (5)
34-34: LGTM!The constant rename from
DEFAULT_THROBBER_FPStoDEFAULT_FPScorrectly reflects that it's now used by bothProgressandThrobberclasses.
585-589: LGTM!The new FPS-related instance variables are correctly initialised. The interval calculation of
round((1.0 / self.fps) * _USECS_PER_SEC)yields 100ms at 10 FPS, which should effectively eliminate terminal flicker from rapid updates.
614-614: LGTM!Initialising
_lastto one interval in the past ensures the firstprogress()call renders immediately, providing good user feedback at startup.
705-719: LGTM!The
_do_cancelmethod correctly forces a final progress update before ending, ensuring the last state is visible. The typo flagged in the previous review ("erasue" → "erasure") has been fixed on line 716.
877-877: LGTM!
ThrobberBasecorrectly references the renamedDEFAULT_FPSconstant.
Eagerly updating the progress bar on every Progress.progress() call
hammers the terminal and causes flicker when working with large total:
use the same mechanism as Throbber to limit the FPS to something
reasonable.
The compute_diffs() progress (which is normally very quick but takes ~30s
on bigger trees with 600k total paths and 400k paths in the tree dicts)
is flickery as heck. Here's how we can support it without the problems I
noted earlier in the issue:
* Save the last status line in the class, even if we decide not to
output it due to FPS limiting
* On cancel (e.g. KeyboardInterrupt) re-draw the last status - that
way the user knows what path we got stuck on if something hangs.
Resolves: #685
Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
On large trees there's a noticeable pause after Comparing trees for '...' due to move detection: add a separate progress bar for that with "Detecting moves" / "Checking moves for '...'". Resolves: #774 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #776 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-flicker-fix-and-move-progress
branch
from
963a416 to
97c645f
Compare
Resolves: #685
Resolves: #774
Resolves: #776
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.