Conversation
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
📝 WalkthroughWalkthroughAdds a Penguin workspace package with packaging config, a runtime check module for Penguin availability, project-level workspace integration, and a unit test that conditionally runs based on Penguin’s installation status. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runtime
participant is_penguin_installed.py as is_penguin_installed
participant Penguin as penguin.config_types
Note over Runtime,is_penguin_installed.py: Import-time availability check
Runtime->>is_penguin_installed.py: import is_penguin_installed
is_penguin_installed.py->>Penguin: try import penguin.config_types
alt import succeeds
is_penguin_installed.py-->>Runtime: INSTALLED=True
Note right of is_penguin_installed.py: prints "PENGUIN INSTALLED=True"
else import fails
is_penguin_installed.py-->>Runtime: INSTALLED=False (on exception)
Note right of is_penguin_installed.py: prints "PENGUIN INSTALLED=False"
end
sequenceDiagram
autonumber
actor Pytest
participant Test as tests/unit/environments/test_penguin.py
participant Penguin as penguin
Pytest->>Test: import test module
Test->>Penguin: try import penguin
alt penguin available
Test-->>Pytest: PENGUIN_INSTALLED=True
Pytest->>Test: run test_penguin_stub_module
Test-->>Pytest: print status with penguin module
else penguin missing
Test-->>Pytest: PENGUIN_INSTALLED=False
Pytest-->>Pytest: skip test (pytest.mark.skipif)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/unit/environments/test_penguin.py (1)
24-26: Stub test implementation noted.This is a placeholder test that only verifies the module can be imported. As indicated by the PR title ("Add Penguin stub"), this is expected behavior for the initial integration. Consider adding meaningful assertions in follow-up PRs.
Would you like me to open a new issue to track adding comprehensive Penguin test coverage?
3rdparty/Penguin-workspace/is_penguin_installed.py (2)
15-15: Remove unnecessary noqa directive.The
# noqa: F401directive is flagged as unused by static analysis. If F401 is not enabled in the ruff configuration, this directive can be removed.Apply this diff if F401 is not in your ruff config:
- from penguin import config_types # noqa: F401 + from penguin import config_typesBased on static analysis hints.
21-21: Reconsider print statement at module import.Printing at module import time is unconventional for library code and will emit output every time this module is imported, which could be unexpected in production environments or when running tests.
Consider one of the following alternatives:
- Use logging instead:
logging.debug(f"PENGUIN {INSTALLED=}")- Remove the print statement entirely and let consumers check the
INSTALLEDflag programmatically- Only print in debug/development mode via an environment variable check
3rdparty/Penguin-workspace/setup.py (1)
37-47: Add error handling for missing or malformed Penguin configuration.The code assumes
Penguin/pyproject.tomlexists with a specific structure (lines 28-32, now removed in suggested fix above). If this file is missing or malformed, the setup will fail with an unclear error. Consider adding try/except with informative error messages.Even after removing the unused code, ensure the setup gracefully handles cases where the Penguin directory exists but is incomplete.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
3rdparty/Penguin-workspace/is_penguin_installed.py(1 hunks)3rdparty/Penguin-workspace/pyproject.toml(1 hunks)3rdparty/Penguin-workspace/setup.py(1 hunks)pyproject.toml(3 hunks)tests/unit/environments/test_penguin.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.py: Follow the Google Python Style Guide for all Python code
Target Python 3.12+ for all Python code in NeMo-RL
Indent Python code with 4 spaces; do not use tabs
Python filenames should be snake_case (e.g., some_file.py)
Class names should be PascalCase
Function and method names should be snake_case
Local variable names should be snake_case; if starting with a number, prefix with k (e.g., k_99th_percentile)
Global variables should be UPPER_SNAKE_CASE and prefixed with G_ (e.g., G_MY_GLOBAL)
Constants should be UPPER_SNAKE_CASE
Avoid shadowing variables declared in an outer scope
Initialize all externally visible members of a class in the constructor
For public interfaces used outside a file, prefer docstrings over comments
Use comments mainly for code within a function or interfaces local to a file
Commented-out code must include a nearby comment explaining usage and why it is commented out; otherwise remove before merging
Use Google-style docstrings for classes and functions (Sphinx-parseable)
Avoid using reflection when functionality can be easily achieved without it
Limit except clauses to the smallest specific set of exceptions possible
For duck-typing via try/except, keep the try body minimal and use else for main logic
Add the NVIDIA copyright header (with current year) at the top of all Python files, excluding tests/ and test-only scripts
Files:
3rdparty/Penguin-workspace/setup.py3rdparty/Penguin-workspace/is_penguin_installed.pytests/unit/environments/test_penguin.py
🪛 Ruff (0.13.3)
3rdparty/Penguin-workspace/is_penguin_installed.py
15-15: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
18-18: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (2)
- GitHub Check: Post submodule check comment / Comment on PR
- GitHub Check: Post automodel integration comment / Comment on PR
🔇 Additional comments (7)
pyproject.toml (3)
103-105: LGTM! Penguin dependency group added correctly.The penguin optional dependency is properly configured within the mcore group, aligning with the workspace integration pattern.
152-152: LGTM! Workspace source configured correctly.The penguin workspace source is properly declared, consistent with other workspace dependencies in the project.
175-175: LGTM! Workspace member registered correctly.The Penguin-workspace is properly added to the workspace members list, following the established pattern for other workspace modules.
tests/unit/environments/test_penguin.py (2)
1-13: LGTM! Copyright header is correct.The NVIDIA copyright header follows the required format.
16-21: LGTM! Import check pattern is appropriate.The try/except pattern for detecting Penguin availability is a reasonable approach for optional dependencies.
3rdparty/Penguin-workspace/setup.py (2)
39-39: Verify version configuration consistency.The version is hardcoded to
"0.0.0"here, but3rdparty/Penguin-workspace/pyproject.tomlline 10 declaresversionas dynamic. Ensure this is intentional for stub/development purposes, or align the configuration.This relates to the comment on
3rdparty/Penguin-workspace/pyproject.tomlabout dynamic configuration strategy.
1-13: LGTM! Copyright header is correct.The NVIDIA copyright header follows the required format.
parthchadha
left a comment
There was a problem hiding this comment.
LGTM, @terrykong to review as well.
Signed-off-by: Brian Yu <bxyu@nvidia.com>
terrykong
left a comment
There was a problem hiding this comment.
@bxyu-nvidia can you run uv lock and commit the lock file? even though there are no dependencies, we still need the uv.lock to reflect penguin is a dependency (albeit empty)
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com>
Signed-off-by: Brian Yu <bxyu@nvidia.com> Signed-off-by: yuanhangs <yuanhangs@nvidia.com>
What does this PR do ?
Add a one line overview of what this PR aims to accomplish.
Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information
Summary by CodeRabbit