Test#5659
Conversation
Greptile SummaryThis PR adds
Confidence Score: 3/5Not safe to merge — contains leftover debug instrumentation that will continuously dump thread stack traces to stderr throughout every test run. The only change is module-level faulthandler code that fires a repeating 1-second timer dumping all thread tracebacks to stderr. The timer is never cancelled, so it runs for the entire test session, making CI logs unreadable. This is clearly diagnostic scaffolding from a debugging session that was not cleaned up before opening the PR. source/isaaclab_newton/test/sim/test_newton_schemas.py — the faulthandler block at lines 8–17 needs to be removed or, at minimum, the Important Files Changed
Reviews (1): Last reviewed commit: "Test" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| faulthandler.dump_traceback_later( | ||
| 1.0, | ||
| repeat=True, | ||
| file=sys.__stderr__, | ||
| exit=False, | ||
| ) |
There was a problem hiding this comment.
faulthandler.dump_traceback_later with repeat=True and a 1-second timeout will dump full thread stack traces to stderr every second for the entire test run. Because faulthandler.cancel_dump_traceback_later() is never called — not even in the setup_sim teardown — the timer runs until the process exits, producing a continuous wall of noise in CI logs. This looks like diagnostic scaffolding added while investigating a hang, and should be removed before merging.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Automated Code Review
Summary
This PR appears to be a debugging/testing PR that makes significant changes to the CI pipeline for troubleshooting test_newton_schemas.py. These changes should not be merged to develop in their current state.
🚨 Critical Issues
1. CI Pipeline Disabled
Severity: CRITICAL | Files: .github/workflows/build.yaml
22 CI jobs are disabled by setting if: false:
- All test jobs (isaaclab-core, isaaclab-assets, isaaclab-devices, etc.)
- Docker build jobs (build-curobo-docker-image)
- cuRobo test jobs
- Training and benchmark jobs
Impact: Merging this would disable the entire CI test suite on the develop branch, allowing untested code to be merged.
Suggestion: These changes should remain on a feature branch for debugging only, or be reverted before merging.
2. Debug Code in Test File
Severity: HIGH | Files: source/isaaclab_newton/test/sim/test_newton_schemas.py
The following debugging instrumentation was added:
import faulthandler
import sys
faulthandler.enable(file=sys.__stderr__, all_threads=True)
faulthandler.dump_traceback_later(
1.0,
repeat=True,
file=sys.__stderr__,
exit=False,
)This enables periodic traceback dumps every 1 second, which is useful for debugging hangs but:
- Adds noise to test output
- May impact test performance
- Should be removed once debugging is complete
⚠️ Moderate Issues
3. Test Scope Narrowed
Severity: MEDIUM | Files: .github/workflows/build.yaml (line 592)
The Newton test job now includes include-files: "test_newton_schemas.py", which limits test execution to only this file. This significantly reduces test coverage for the isaaclab_newton package.
📋 Process Issues
| Issue | Status |
|---|---|
| PR Description | ❌ Template not filled out |
| Linked Issue | ❌ No issue referenced |
| Contribution Checklist | ❌ All items unchecked |
| PR Title |
💡 Recommendations
- Do not merge this PR to
develop- The CI disabling changes would break the repository's testing infrastructure - If debugging is needed, consider:
- Creating a draft PR
- Adding
[WIP]or[DO NOT MERGE]to the title - Using a separate branch for CI experiments
- Once debugging is complete, create a new PR with only the actual fix
- Remove faulthandler code before any merge to main branches
This is an automated review. Please address the critical issues before requesting human review.
Update (d537628): New commit adds -vv flag to pytest in tools/conftest.py for extra verbosity. This is consistent with the debugging nature of the PR and doesn't introduce new issues. Previous concerns remain unaddressed:
- ❌ CI pipeline still disabled
- ❌ Debug faulthandler code still present
- ❌ Test scope still narrowed
Update (70b2a25): New commit comments out omni.kit.telemetry extension in both apps/isaaclab.python.headless.kit and apps/isaaclab.python.kit. This appears to be another debugging/troubleshooting change (possibly to rule out telemetry as a cause of hangs). No new critical issues, but this is another change that should not be merged to develop without justification.
Previous concerns status:
- ❌ CI pipeline still disabled (22 jobs)
- ❌ Debug faulthandler code still present
- ❌ Test scope still narrowed
⚠️ New: Telemetry disabled without documented reason
1 participant
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there