Repository navigation
Conversation
Signed-off-by: saime428 <51110572+saime428@users.noreply.github.com>
saime428
requested review from
AntoineRichard,
StafaH,
fatimaanes,
kellyguo11,
ooctipus and
pbarejko
as code owners
November 9, 2026 16:53
Contributor
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Style consistency (2). Other reviewed categories look good.
- PR description: The title and body match the fallback, regression coverage, and changelog; reported baseline validation and the unchanged Mac test failure are explicit, with no performance claim requiring benchmarks.
- Design and architecture: Architecture looks good.
- Scope and over-engineering: No over-engineering issues.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: Implementation looks good.
- Style consistency: The fallback uses getattr for the known encoding attribute, hiding a missing attribute contrary to repository guidance; use self._console.encoding while retaining the None fallback.
- Test quality: Test quality looks good.
Minor fixes needed. Posted 2 inline findings.
Automated review; human maintainers own approval decisions.
Contributor
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Style consistency (2). Other reviewed categories look good.
- PR description: The title and body match the fallback, changelog, and encoding regressions; validation limitations and the baseline Mac failure are disclosed, with no measurable performance claim requiring benchmarks.
- Design and architecture: Architecture looks good.
- Scope and over-engineering: No over-engineering issues.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: Implementation looks good.
- Style consistency: Reflective access to the known encoding attribute hides missing attributes behind ASCII; use self._console.encoding while retaining the None fallback.
- Test quality: Test quality looks good.
Minor fixes needed. Posted 2 inline findings.
Automated review; human maintainers own approval decisions.
Contributor
 [Medium impact] The change affects console output during loading-screen startup. The PR appears safe to merge, with a non-blocking improvement needed to make the new test independent of terminal width. Findings
SummaryThe PR prevents redirected loading-screen output from crashing on characters its encoding cannot represent.
Reviews (1) · Last reviewed commit: "Fix loading-screen crashes on redirected..." · Reviewed by Greptile |
Signed-off-by: saime428 <51110572+saime428@users.noreply.github.com>
2 of 4 tasks
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When stdout uses strict cp932 encoding, the summary title
Isaac Lab · trainraisesUnicodeEncodeErrorbefore training starts. This applies the fallback suggested in #8978:_writeretries with replacement characters if the stream cannot encode the text. It keeps the characters the stream can display and leaves the stream settings and normal UTF-8 output unchanged.The regression calls
LoadingScreen.summary()with real cp932 and UTF-8 streams. It checks that the task information remains readable, encodable Japanese text survives, and the stream settings stay intact. The fix includes a package changelog entry.Fixes #8978.
#8977 filters greeting art that the console cannot encode. The title still crashes on its head (
2214b28f3332) withlogo=False, so that fix does not cover this case.Type of change
Testing
Tested on macOS ARM64 with Python 3.12.15, rich 14.3.4, pytest 9.1.1, and lazy-loader 0.5. The uv environment contains the editable core package and test/format tools; commands use
--no-syncto avoid installing the simulation stack.The cp932 regression fails with
UnicodeEncodeErroron the original source at4cfdfcfa52d7and passes with the fix. The UTF-8 control passes on both.uv run --no-sync python -m pytest source/isaaclab/test/app/test_loading_screen.py -q -k 'not test_interactive_input_shows_captured_context_before_waiting_for_answer': 15 passed, 1 deselected.COLUMNS=30 uv run --no-sync python -m pytest source/isaaclab/test/app/test_loading_screen.py -k summary_preserves -q: 2 passed. Before pinning the test width, both cases failed because the task name wrapped.uv run --no-sync python -m pytest source/isaaclab/test/app/test_remote_greetings.py -q: 4 passed.ISAACLAB_CHANGELOG_BASE_REF=origin/main uv run --no-sync isaaclab --format: passed, including repository-wide lint, changelog, and Git LFS pointer checks.git diff --check: passed.The full
test_loading_screen.pyrun has 15 passes and one failure:test_interactive_input_shows_captured_context_before_waiting_for_answerfails atassert b"Answer: NO" in outputon this Mac. The same test fails after restoring the original production file. It is unchanged in this patch and excluded only from the focused command above.A Japanese Windows training run and Docker/GPU tests were not run.
Checklist
uv run --no-sync isaaclab --format.source/isaaclab/changelog.d/.CONTRIBUTORS.mdassaime428.