Fix tqdm progress bar rendering in GitHub Actions logs - #6949
Fix tqdm progress bar rendering in GitHub Actions logs#6949mataylor-nvidia wants to merge 2 commits into
Conversation
The "Prefilling reset buffer" tqdm bar renders as garbled block characters in GitHub Actions logs because the log viewer does not support carriage-return-based line overwriting. Add disable=not sys.stderr.isatty() so the bar is suppressed when running in CI. Replace the missing terminal output with logging.info calls at the start and on completion so CI logs still show meaningful progress.
|
Too many files changed for review (3000 files, 100 file limit). |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The TTY-aware tqdm suppression is a minimal, non-breaking fix, but two maintainability issues should be corrected before merge: the logger assignment interrupts the import block, and the changelog uses incorrect Sphinx roles that may produce unresolved references.
- Design and architecture: Applying
disable=not sys.stderr.isatty()attqdmconstruction preserves the existing prefill control flow and keepsprogress.update()valid in both interactive and non-interactive environments. No architectural concern was found with this approach. - API: No public signatures, defaults, return types, or exported symbols change. Interactive behavior remains intact, while non-TTY output changes only in presentation, so no deprecation or public API documentation updates are required.
- Implementation: Move
_log = logging.getLogger(__name__)below the complete import block to preserve PEP 8 import ordering and avoid E402/isort failures. In the changelog, referenceisaaclab_tasks.core.lift.mdp.eventswith:mod:rather than:func:, and render the third-partytqdmname as a literal instead of an unresolved:class:target.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| ^^^^^ | ||
|
|
||
| * Fixed the "Prefilling reset buffer" progress bar in | ||
| :func:`~isaaclab_tasks.core.lift.mdp.events` rendering as garbled block |
There was a problem hiding this comment.
🟡 Warning · Implementation — Incorrect Sphinx roles in changelog fragment
isaaclab_tasks.core.lift.mdp.events is a module, so :func: will not resolve once the fragment is compiled into CHANGELOG.rst; likewise :class:tqdm targets an external package with no such documented class. Use `:mod:` for the module and plain literal markup (tqdm``) for the third-party library to avoid broken references in the generated changelog.
| import warp as wp | ||
| from tqdm import tqdm | ||
|
|
||
| _log = logging.getLogger(__name__) |
There was a problem hiding this comment.
🟡 Warning · Implementation — Logger assignment splits the import block
_log = logging.getLogger(__name__) is inserted between the third-party imports and the isaaclab imports, so module-level executable code now precedes imports. This breaks PEP 8 import ordering (E402) and leaves isort with two disjoint import sections, which can churn under the formatting hooks. Move the logger definition below the final import block.
1 participant
Description
The "Prefilling reset buffer"
tqdmprogress bar inisaaclab_tasks/core/lift/mdp/events.pyrenders as garbled Unicode blockcharacters in GitHub Actions logs. The root cause is that
tqdmusescarriage-return (
\r) overwriting to update the bar in place; GitHub's logviewer does not support
\r, so each update is emitted as a new line full of█characters.Fix: add
disable=not sys.stderr.isatty()to thetqdmcall so the baris suppressed when stderr is not a terminal (i.e. in CI). Replace the missing
output with
logging.infocalls at start and on completion so CI logs stillconfirm the buffer fill is running and when it finishes.
No behaviour change in interactive terminals —
tqdmrenders normally there.Fixes # (issue)
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there