From ba5a5b33f95133538dc8080ce67d028a97725259 Mon Sep 17 00:00:00 2001 From: Terry Kong Date: Wed, 17 Sep 2025 07:21:01 +0000 Subject: [PATCH 1/3] chore: add coderabbit configuration and coding guidelines for coderabbit Signed-off-by: Terry Kong --- .coderabbit-current.yaml | 457 +++++++++++++++++++++++++++++++++++++++ .coderabbit.yaml | 101 +++++++++ CODING_GUIDELINES.md | 295 +++++++++++++++++++++++++ 3 files changed, 853 insertions(+) create mode 100644 .coderabbit-current.yaml create mode 100644 .coderabbit.yaml create mode 100644 CODING_GUIDELINES.md diff --git a/.coderabbit-current.yaml b/.coderabbit-current.yaml new file mode 100644 index 0000000000..f29c0fd099 --- /dev/null +++ b/.coderabbit-current.yaml @@ -0,0 +1,457 @@ + # Set the language for reviews by using the corresponding ISO language code. + # Default: "en-US" + language: en-US + # Set the tone of reviews and chat. + # Default: "" + tone_instructions: '' # not in base + # Enable early-access features. + # Default: false + early_access: false # not in base + # Enable free tier features for users not on a paid plan. + # Default: true + enable_free_tier: true # not in base + tone_instructions: '' + early_access: false + enable_free_tier: true + # Settings related to reviews. + # Default: {} + reviews: + profile: chill + # Set the profile for reviews. Assertive profile yields more feedback, that may be considered nitpicky. + # Options: chill, assertive + # Default: "chill" + request_changes_workflow: false # not in base + # Approve the review once CodeRabbit’s comments are resolved and no pre-merge checks are in an error state. + # Default: false + high_level_summary: true # not in base + # Generate a high level summary of the changes in the PR/MR description. + # Default: true + high_level_summary_placeholder: '@coderabbitai summary' # not in base + # Placeholder in the PR/MR description that gets replaced with the high level summary. + # Default: "@coderabbitai summary" + high_level_summary_in_walkthrough: false # not in base + # Include the high level summary in the walkthrough comment. + # Default: false + auto_title_placeholder: '@coderabbitai' + # Add this keyword in the PR/MR title to auto-generate the title. + # Default: "@coderabbitai" + auto_title_instructions: '' + # Auto Title Instructions - Custom instructions for auto-generating the PR/MR title. + # Default: "" + review_status: true + # Post review details on each review. Additionally, post a review status when a review is skipped in certain cases. + # Default: true + commit_status: true + # Set the commit status to 'pending' when the review is in progress and 'success' when it is complete. + # Default: true + fail_commit_status: false + # Set the commit status to 'failure' when the PR cannot be reviewed by CodeRabbit for any reason. + # Default: false + collapse_walkthrough: false + # Generate walkthrough in a markdown collapsible section. + # Default: false + changed_files_summary: true # not in base + # Generate a summary of the changed files in the walkthrough. + # Default: true + sequence_diagrams: true # not in base + # Generate sequence diagrams in the walkthrough. + # Default: true + estimate_code_review_effort: true # not in base + # Estimate the code review effort in the walkthrough. + # Default: true + assess_linked_issues: true + # Generate an assessment of how well the changes address the linked issues in the walkthrough. + # Default: true + related_issues: true + # Include possibly related issues in the walkthrough. + # Default: true + related_prs: true + # Related PRs - Include possibly related pull requests in the walkthrough. + # Default: true + suggested_labels: true + # Suggest labels based on the changes in the pull request in the walkthrough. + # Default: true + auto_apply_labels: false # not in base + # Automatically apply the suggested labels to the PR/MR. + # Default: false + suggested_reviewers: true + # Suggest reviewers based on the changes in the pull request in the walkthrough. + # Default: true + auto_assign_reviewers: false # not in base + # Automatically assign suggested reviewers to the pull request + # Default: false + poem: true + # Generate a poem in the walkthrough comment. + # Default: true + labeling_instructions: [] # not in base + # Labeling Instructions - Provide guidelines for suggesting labels for the PR/MR. + # Default: [] + path_filters: [] # not in base + # Specify file patterns to include or exclude in a review using glob patterns. + # Default: [] + path_instructions: [] # not in base + # Path Instructions - Provide specific additional guidelines for code review based on file paths. + # Default: [] + abort_on_close: true # not in base + # Abort the in-progress review if the pull request is closed or merged. + # Default: true + disable_cache: false # not in base + # Disable caching of code and dependencies. + # Default: false + auto_review: + # Configuration for auto review + # Default: {} + enabled: true # not in base + # Automatic Review - Automatic code review + # Default: true + auto_incremental_review: true + # Automatic Incremental Review - Automatic incremental code review on each push + # Default: true + ignore_title_keywords: [] # not in base + # Ignore reviewing if the title of the pull request contains any of these keywords. + # Default: [] + labels: [] # not in base + # List of labels to control which PRs/MRs to review. + # Default: [] + drafts: false + # Review draft PRs/MRs. + # Default: false + base_branches: [] + # Base branches (other than the default branch) to review. Accepts regex patterns. + # Default: [] + ignore_usernames: [] # not in base + # Ignore reviewing pull requests by these usernames. + # Default: [] + finishing_touches: # not in base + # Configuration for finishing touches + # Default: {} + docstrings: + # Docstrings - Allow CodeRabbit to generate docstrings for PRs/MRs. + # Default: true + enabled: true + unit_tests: + # Unit Tests - Allow CodeRabbit to generate unit tests for PRs/MRs. + # Default: true + enabled: true + pre_merge_checks: # not in base + # Configuration for pre merge checks + # Default: {} + docstrings: + # Mode - Determines how strictly the docstring coverage check is enforced. + # Default: "warning" + mode: warning + # Percentage threshold for docstring coverage check. + # Default: 80 + threshold: 80 + title: + # Mode - Determines how strictly the title check is enforced. + # Default: "warning" + mode: warning + # Requirements - Requirements for the pull request title. + # Default: "" + requirements: '' + description: + # Mode - Determines how strictly the description check is enforced. + # Default: "warning" + mode: warning + issue_assessment: + # Mode - Determines how strictly the issue assessment check is enforced. + # Default: "warning" + mode: warning + custom_checks: [] + # Custom Pre-merge Checks - Add unique checks to enforce your team's standards before merging a pull request. + # Default: [] + tools: # not in base + # Tools that provide additional context to code reviews. + # Default: {} + ast-grep: + # List of rules directories. + # Default: [] + rule_dirs: [] + # List of utils directories. + # Default: [] + util_dirs: [] + # Use ast-grep essentials package. + # Default: true + essential_rules: true + # Predefined packages to be used. + # Default: [] + packages: [] + shellcheck: + # Enable ShellCheck - Enable ShellCheck integration. + # Default: true + enabled: true + ruff: + # Enable Ruff - Enable Ruff integration. + # Default: true + enabled: true + markdownlint: + # Enable markdownlint - Enable markdownlint integration. + # Default: true + enabled: true + github-checks: + # Enable GitHub Checks - Enable GitHub Checks integration. + # Default: true + enabled: true + # Time in milliseconds to wait for all GitHub Checks to conclude. + # Default: 90000 + timeout_ms: 90000 + languagetool: + # Enable LanguageTool - Enable LanguageTool integration. + # Default: true + enabled: true + # IDs of rules to be enabled. + # Default: [] + enabled_rules: [] + # IDs of rules to be disabled. + # Default: [] + disabled_rules: [] + # IDs of categories to be enabled. + # Default: [] + enabled_categories: [] + # IDs of categories to be disabled. + # Default: [] + disabled_categories: [] + # Only the rules and categories whose IDs are specified are enabled. + # Default: false + enabled_only: false + # Level of strictness for LanguageTool. + # Default: "default" + level: default + biome: + # Enable Biome - Enable Biome integration. + # Default: true + enabled: true + hadolint: + # Enable Hadolint - Enable Hadolint integration. + # Default: true + enabled: true + swiftlint: + # Enable SwiftLint - Enable SwiftLint integration. + # Default: true + enabled: true + phpstan: + # Enable PHPStan - Enable PHPStan integration. + # Default: true + enabled: true + # Level - Specify the rule level to run. + # Default: "default" + level: default + phpmd: + # Enable PHPMD - Enable PHPMD integration. + # Default: true + enabled: true + phpcs: + # Enable PHP CodeSniffer - Enable PHP CodeSniffer integration. + # Default: true + enabled: true + golangci-lint: + # Enable golangci-lint - Enable golangci-lint integration. + # Default: true + enabled: true + yamllint: + # Enable YAMLlint - Enable YAMLlint integration. + # Default: true + enabled: true + gitleaks: + # Enable Gitleaks - Enable Gitleaks integration. + # Default: true + enabled: true + checkov: + # Enable Checkov - Enable Checkov integration. + # Default: true + enabled: true + detekt: + # Enable detekt - Enable detekt integration. + # Default: true + enabled: true + eslint: + # Enable ESLint - Enable ESLint integration. + # Default: true + enabled: true + flake8: + # Enable Flake8 - Enable Flake8 integration. + # Default: true + enabled: true + rubocop: + # Enable RuboCop - Enable RuboCop integration. + # Default: true + enabled: true + buf: + # Enable Buf - Enable Buf integration. + # Default: true + enabled: true + regal: + # Enable Regal - Enable Regal integration. + # Default: true + enabled: true + actionlint: + # Enable actionlint - Enable actionlint integration. + # Default: true + enabled: true + pmd: + # Enable PMD - Enable PMD integration. + # Default: true + enabled: true + cppcheck: + # Enable Cppcheck - Enable Cppcheck integration. + # Default: true + enabled: true + semgrep: + # Enable Semgrep - Enable Semgrep integration. + # Default: true + enabled: true + circleci: + # Enable CircleCI - Enable CircleCI integration. + # Default: true + enabled: true + clippy: + # Enable Clippy - Enable Clippy integration. + # Default: true + enabled: true + sqlfluff: + # Enable SQLFluff - Enable SQLFluff integration. + # Default: true + enabled: true + prismaLint: + # Enable Prisma Schema linting + # Default: true + enabled: true + pylint: + # Enable Pylint - Enable Pylint integration. + # Default: true + enabled: true + oxc: + # Enable Oxlint - Enable Oxlint integration. + # Default: true + enabled: true + shopifyThemeCheck: + # Enable Shopify Theme Check + # Default: true + enabled: true + luacheck: + # Enable Lua code linting - Enable Luacheck integration. + # Default: true + enabled: true + brakeman: + # Enable Brakeman - Enable Brakeman integration. + # Default: true + enabled: true + dotenvLint: + # Enable dotenv-linter - Enable dotenv-linter integration. + # Default: true + enabled: true + htmlhint: + # Enable HTMLHint - Enable HTMLHint integration. + # Default: true + enabled: true + checkmake: + # Enable checkmake - Enable checkmake integration. + # Default: true + enabled: true + osvScanner: + # Enable OSV Scanner - Enable OSV Scanner integration. + # Default: true + enabled: true + # Configuration for chat + # Default: {} + chat: # not in base + art: true + # Generate art in response to chat messages. + # Default: true + auto_reply: true + # Enable the bot to reply automatically without requiring the user to tag it. + # Default: true + integrations: + # Configuration for integrations + # Default: {} + jira: + # Jira - Enable the Jira integration for opening issues, etc. + # Default: "auto" + usage: auto + linear: + # Linear - Enable the Linear integration for opening issues, etc. + # Default: "auto" + usage: auto +knowledge_base: + # Configuration for knowledge base + # Default: {} + opt_out: false # not in base + # Opt Out - Disable all knowledge base features that require data retention. + # Default: false + web_search: + # Configuration for web search + # Default: {} + enabled: true # not in base + # Web Search - Enable the web search integration. + # Default: true + code_guidelines: + enabled: true + # CodeRabbit will analyse and learn from your organization's code guidelines. + # Default: {} + filePatterns: [] + # File Patterns - Specify files for your coding guideline documents. + # Default: [] + learnings: + # Configuration for learnings + # Default: {} + scope: auto # not in base + # Learnings - Specify the scope of learnings to use for the knowledge base. + # Default: "auto" + issues: + # Configuration for issues + # Default: {} + scope: auto # not in base + # Issues - Specify the scope of git platform issues to use for the knowledge base. + # Default: "auto" + jira: + # Configuration for jira + # Default: {} + usage: auto # not in base + # Jira - Enable the Jira knowledge base integration. + # Default: "auto" + project_keys: [] # not in base + # Jira Project Keys - Specify the Jira project keys to use for the knowledge base. + # Default: [] + linear: + # Configuration for linear + # Default: {} + usage: auto # not in base + # Linear - Enable the Linear knowledge base integration. + # Default: "auto" + team_keys: [] # not in base + # Linear Team Keys - Specify the Linear team keys to use for the knowledge base. + # Default: [] + pull_requests: + # Configuration for pull requests + # Default: {} + scope: auto # not in base + # Pull Requests - Specify the scope of pull requests to use for the knowledge base. + # Default: "auto" + mcp: + # Configuration for mcp + # Default: {} + usage: auto # not in base + # MCP - Enable the MCP knowledge base integration. + # Default: "auto" + disabled_servers: [] # not in base + # MCP Disabled Servers - Specify MCP server labels to disable. + # Default: [] +code_generation: # not in base + # Configuration for code generation + # Default: {} + docstrings: + # Settings related to the generation of docstrings. + # Default: {"path_instructions":[]} + language: en-US # not in base + # Set the language for docstrings by using the corresponding ISO language code. + # Default: "en-US" + path_instructions: [] # not in base + # Path Instructions - Provide additional guidelines for docstring generation based on file paths. + # Default: [] + unit_tests: + # Settings related to the generation of unit tests. + # Default: {"path_instructions":[]} + path_instructions: [] # not in base + # Unit Test Generation - Provide additional guidelines for unit test generation based on file paths. + # Default: [] \ No newline at end of file diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000000..e596a5d26f --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,101 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +# https://docs.coderabbit.ai/getting-started/configure-coderabbit/ +# Validator https://docs.coderabbit.ai/configuration/yaml-validator#yaml-validator +# In PR, comment "@coderabbitai configuration" to get the full config including defaults +# Set the language for reviews by using the corresponding ISO language code. +# Default: "en-US" +language: "en-US" +# Settings related to reviews. +# Default: {} +reviews: + # Set the profile for reviews. Assertive profile yields more feedback, that may be considered nitpicky. + # Options: chill, assertive + # Default: "chill" + profile: chill + # Add this keyword in the PR/MR title to auto-generate the title. + # Default: "@coderabbitai" + auto_title_placeholder: '@coderabbitai title' + # Auto Title Instructions - Custom instructions for auto-generating the PR/MR title. + # Default: "" + auto_title_instructions: 'Format: ": ". Category must be one of: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert, cp. The category must be followed by a colon. Title should be concise (<= 80 chars). Example: "feat: Add logit_bias support".' # current: '' + # Set the commit status to 'pending' when the review is in progress and 'success' when it is complete. + # Default: true + commit_status: false + # Generate walkthrough in a markdown collapsible section. + # Default: false + collapse_walkthrough: true + # Generate an assessment of how well the changes address the linked issues in the walkthrough. + # Default: true + assess_linked_issues: true + # Include possibly related issues in the walkthrough. + # Default: true + related_issues: true + # Related PRs - Include possibly related pull requests in the walkthrough. + # Default: true + related_prs: true + # Suggest labels based on the changes in the pull request in the walkthrough. + # Default: true + suggested_labels: true + # Suggest reviewers based on the changes in the pull request in the walkthrough. + # Default: true + suggested_reviewers: true + # Generate a poem in the walkthrough comment. + # Default: true + poem: false # current: true + # Post review details on each review. Additionally, post a review status when a review is skipped in certain cases. + # Default: true + review_status: false # current: true + # Configuration for pre merge checks + # Default: {} + pre_merge_checks: + # Custom Pre-merge Checks - Add unique checks to enforce your team's standards before merging a pull request. Each check must have a unique name (up to 50 characters) and clear instructions (up to 10000 characters). Use these to automatically verify coding, security, documentation, or business rules and maintain code quality. + # Default: [] + custom_checks: + - name: "Test Results for Major Changes" + mode: "warning" # or "error" to block merges + instructions: | + If this PR contains major changes (such as new features, breaking changes, or significant refactoring), verify that the PR description includes test results or testing information. + If a change could affect numerics or convergence, the PR description should include information demonstrating that there is no regression. + If a change could affect performance, the PR description should include before-and-after performance numbers, as well as the configuration and context in which they apply. + Pass if test results are documented or if the changes are minor. + auto_review: + # Configuration for auto review + # Default: {} + # Automatic Incremental Review - Automatic incremental code review on each push + # Default: true + auto_incremental_review: false # current: true + # Review draft PRs/MRs. + # Default: false + drafts: false + # Base branches (other than the default branch) to review. Accepts regex patterns. Use '.*' to match all branches. + # Default: [] + base_branches: ["main", "r[0-9].*"] # current: [] +# Configuration for knowledge base +# Default: {} +knowledge_base: + code_guidelines: + # CodeRabbit will analyse and learn from your organization's code guidelines, which you can mention in the file patterns section. These guidelines will then be used to conduct thorough code reviews. + # Default: {} + enabled: true + # Enabled - Enable CodeRabbit to enforce your organization's coding standards during reviews. + # Default: true + filePatterns: # current: [] + # File Patterns - Specify files for your coding guideline documents in this section. CodeRabbit will scan these files to understand your team's standards and apply them during code reviews. Multiple files supported. File names are case-sensitive. Common files like: (**/.cursorrules, .github/copilot-instructions.md, .github/instructions/*.instructions.md, **/CLAUDE.md, **/GEMINI.md, **/.cursor/rules/*, **/.windsurfrules, **/.clinerules/*, **/.rules/*, **/AGENT.md, **/AGENTS.md) are included by default. + # Default: [] + - "**/CODING_GUIDELINES.md" + - "**/.cursor/rules/*" diff --git a/CODING_GUIDELINES.md b/CODING_GUIDELINES.md new file mode 100644 index 0000000000..e129a794fa --- /dev/null +++ b/CODING_GUIDELINES.md @@ -0,0 +1,295 @@ + +# NeMo-RL Coding Guidelines + +Note: This repository is Python-first. Prefer the Python guidelines in this document. + +## Style Guides We Follow + +- Python: [Google Python Style Guide](https://google.github.io/styleguide/pyguide.html) +- Shell: [Google Shell Style Guide](https://google.github.io/styleguide/shellguide.html) + +## uv Guidelines + +#### Use uv run instead of python + +Use `uv run` to execute scripts, rather than activating a virtual environment and calling `python` directly. + +Don't: + +```bash +source .venv/bin/activate +python examples/run_grpo_math.py +``` + +Do: + +```bash +uv run examples/run_grpo_math.py +``` + +Exception: `Dockerfile.ngc_pytorch` is exempt from this rule. + +## Python Coding Guidelines +#### Python Standard +1. The code developed for NeMo RL should conform to Python 3.12+. + +#### Indentation +1. Indent code with 4 spaces. Do not use tabs. + +#### Imports +1. Always maintain the namespace when importing, even if only one class or function from a module is used. + +For example instead of: + +```python +from package.subpackage.foo import SomeClass +SomeClass() +``` +or +```python +import package +package.subpackage.foo.SomeClass() +``` + +Do: + +```python +from package.subpackage import foo +foo.SomeClass() +``` + +#### Naming + +##### Identifier Format +1. Files +- snake_case: `some_file.py` + +2. Classes +- PascalCase: `class SomeClass` + +3. Functions and Methods +- snake_case: `def my_awesome_function():` + +4. Local Variables +- snake_case: `my_variable = ...` +- prefix `k` for variable names that start with a number: `k_99th_percentile = ...` + +5. Global Variables +- upper snake_case and prefix `G`: `G_MY_GLOBAL = ...` + +6. Constants +- upper snake_case: `MY_CONSTANT = ...` + +##### Identifier Guidelines +1. Avoid shadowing variables declared in an outer scope. +2. Initialize all externally visible memberes of a class in the constructor. + +#### Comments + +1. For interfaces that may be used outside a file, prefer docstrings over comments. +2. Comments should be reserved for code within a function, or interfaces that are local to a file. + +#### Docstring Syntax +##### Classes and Functions +Use the [Google style](https://google.github.io/styleguide/pyguide.html), which can be parsed by Sphinx. + +##### Attributes and Variables +Attributes and variables can be documented inline. Attribute docstrings will be rendered under the docstring for the class. For example: +```python +class MyClass: + """ + Description of the class. + """ + def __init__(self): + self.x = 5 + """<type>: Description of 'x'""" + +y = 2 +"""<type>: Description of 'y'""" +``` + +#### Avoid Reflection +Avoid using reflection when functionality can be easily achieved without reflection. + +For example, instead of: + +```python +def make_complex(*args): + x, y = args + return dict(**locals()) +``` + +Do: + +```python +def make_complex(x, y): + return {'x': x, 'y': y} +``` + +#### Error Handling +1. When using try-except blocks, limit the except to the smallest set of errors possible. + +For example, instead of: + +```python +try: + open(path, "r").read() +except: + print("Failed to open file") +``` + +Do: + +```python +try: + open(path, "r").read() +except FileNotFoundError: + print("Failed to open file") +``` + + +2. When using try-except blocks to handle multiple possible variable types (i.e. duck-typing), keep the body of the try as small as possible, using the else block to implement the logic. + +For example, instead of: + +```python +try: + f.seek(0) + f.read() +except AttributeError: + ... # Not a file-like object, do something else +``` + +Do: + +```python +try: + f.seek # Do not call to minimize chance of unrelated failure +except AttributeError: + ... # Not a file-like object, do something else +else: + f.seek(0) + f.read() +``` + +## Doc Guidelines + +#### Ensure docs/index.md is up to date + +When a new markdown doc is added under `docs/**/*.md` or a markdown file is renamed, ensure that `docs/index.md` is updated and the document appears in the most appropriate section. + +## Tests + +### Coverage and Ray Actors + +- For any source file under `nemo_rl/*.py` that defines a class or function decorated with `@ray.remote`, add a coverage pragma because these run in separate Ray processes and are not reliably tracked by coverage. +- Place `# pragma: no cover` on the `class` or `def` line (and on any remote functions), for example: + +```python +import ray + +@ray.remote # pragma: no cover +class RolloutActor: + def run(self) -> None: + ... + +@ray.remote # pragma: no cover +def remote_eval(batch): + ... +``` + +### Nightly Tests for New Model Support + +When adding support for a new model, add a corresponding nightly test consisting of: + +1) Recipe YAML under `examples/configs/recipes/` +- Place the YAML in the appropriate domain subdirectory (e.g., `examples/configs/recipes/llm/` or `examples/configs/recipes/vlm/`). +- Name it following our recipe naming rules (see below). The YAML filename should mirror the driver script name but with `.yaml`. + +2) Driver script under `tests/test_suites/` +- Create a shell script in the matching domain (e.g., `tests/test_suites/llm/` or `tests/test_suites/vlm/`). +- The script should source any common environment (e.g., `common.env`) and invoke the training entrypoint with `uv run ... --config <path-to-yaml>` as appropriate. +- Match the driver script filename to the YAML base name, with `.sh`. + +3) Add to nightly list +- Append the driver script path (relative to `tests/test_suites/`) to `tests/test_suites/nightly.txt`. + +### Recipe Naming Rules (YAML and Driver Scripts) + +Base pattern (LLM): + +``` +<algo>-<model>-<nodes>n<gpus>g-<strategy-and-params>[-modifiers][-long][.vN].(yaml|sh) +``` + +- **algo**: task or algorithm, e.g., `sft`, `dpo`, `grpo`. +- **model**: model identifier, e.g., `llama3.1-8b-instruct`, `qwen2.5-7b-instruct`. +- **nodes/gpus**: cluster allocation, e.g., `1n8g`, `4n8g`, `8n8g`. +- **strategy-and-params**: parallelism or framework detail, e.g., `fsdp2tp1`, `tp4pp2`, `megatron`, `dtensor2tp1`. +- **modifiers** (optional): short flags like `sp` (sequence packing), `actckpt` (activation checkpointing), `fp8`, `noncolocated`, `quick`. +- **-long** (optional): indicates long-running recipe. +- **.vN** (optional): version suffix (e.g., `.v2`, `.v3`) reserved for convergence-impacting changes. + +Examples (from current tree): + +``` +sft-llama3.1-8b-1n8g-fsdp2tp1-long.yaml +dpo-llama3.1-8b-instruct-4n8g-fsdp2tp4.yaml +grpo-llama3.1-8b-instruct-1n8g-megatron-fp8.yaml +grpo-qwen2.5-7b-instruct-4n8g-fsdp2tp4sp.v3.yaml +``` + +VLM pattern: + +``` +vlm_<algo>-<model>-<nodes>n<gpus>g-<strategy>[-modifiers][.vN].(yaml|sh) +``` + +Examples: + +``` +vlm_grpo-qwen2.5-vl-3b-instruct-clevr-1n2g-dtensor2tp1.v1.yaml +vlm_grpo-smolvlm2-2.2b-instruct-clevr-1n2g-dtensor2tp1.v1.sh +``` + +Known exceptions currently present: +- Deepscaler recipes encode context length in place of the cluster tuple, e.g., `grpo-deepscaler-1.5b-8K.(yaml|sh)`. These are allowed but should document the intended hardware in the script body. +- Some recipes include additional short flags in the strategy token (e.g., `fsdp2tp8sp`). Treat these as modifiers appended to the strategy. + +Directory placement: + +``` +examples/configs/recipes/ + llm/ + <name>.yaml + vlm/ + <name>.yaml + +tests/test_suites/ + llm/ + common.env + <name>.sh + vlm/ + common.env + <name>.sh + nightly.txt +``` + +## NVIDIA Copyright + +1. In NeMo-RL, add the following NVIDIA copyright header to all Python files and shell scripts and this header should include the current year. Exclude tests (e.g., files under `tests/` or test-only scripts). The header should appear at the top of the file. +```py +# Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +``` From 711bcd3c27dff3faeafca252890746a378c98128 Mon Sep 17 00:00:00 2001 From: Terry Kong <terryk@nvidia.com> Date: Wed, 17 Sep 2025 19:54:45 +0000 Subject: [PATCH 2/3] fix up coding guidelines with more details and remove unnecessary things Signed-off-by: Terry Kong <terryk@nvidia.com> --- CODING_GUIDELINES.md | 95 ++++++++++++++++++------------------- tests/test_suites/README.md | 43 +++++++++++++---- 2 files changed, 79 insertions(+), 59 deletions(-) diff --git a/CODING_GUIDELINES.md b/CODING_GUIDELINES.md index e129a794fa..3149579501 100644 --- a/CODING_GUIDELINES.md +++ b/CODING_GUIDELINES.md @@ -10,7 +10,7 @@ Note: This repository is Python-first. Prefer the Python guidelines in this docu ## uv Guidelines -#### Use uv run instead of python +### Use uv run instead of python Use `uv run` to execute scripts, rather than activating a virtual environment and calling `python` directly. @@ -30,37 +30,15 @@ uv run examples/run_grpo_math.py Exception: `Dockerfile.ngc_pytorch` is exempt from this rule. ## Python Coding Guidelines -#### Python Standard +### Python Standard 1. The code developed for NeMo RL should conform to Python 3.12+. -#### Indentation +### Indentation 1. Indent code with 4 spaces. Do not use tabs. -#### Imports -1. Always maintain the namespace when importing, even if only one class or function from a module is used. +### Naming -For example instead of: - -```python -from package.subpackage.foo import SomeClass -SomeClass() -``` -or -```python -import package -package.subpackage.foo.SomeClass() -``` - -Do: - -```python -from package.subpackage import foo -foo.SomeClass() -``` - -#### Naming - -##### Identifier Format +#### Identifier Format 1. Files - snake_case: `some_file.py` @@ -80,35 +58,21 @@ foo.SomeClass() 6. Constants - upper snake_case: `MY_CONSTANT = ...` -##### Identifier Guidelines +#### Identifier Guidelines 1. Avoid shadowing variables declared in an outer scope. 2. Initialize all externally visible memberes of a class in the constructor. -#### Comments +### Comments 1. For interfaces that may be used outside a file, prefer docstrings over comments. 2. Comments should be reserved for code within a function, or interfaces that are local to a file. +3. If a piece of code is commented out, there should be a comment around that piece of code describing it's usage and why it's commented out. Otherwise that is a debug comment and it should be removed before merging -#### Docstring Syntax -##### Classes and Functions +### Docstring Syntax +#### Classes and Functions Use the [Google style](https://google.github.io/styleguide/pyguide.html), which can be parsed by Sphinx. -##### Attributes and Variables -Attributes and variables can be documented inline. Attribute docstrings will be rendered under the docstring for the class. For example: -```python -class MyClass: - """ - Description of the class. - """ - def __init__(self): - self.x = 5 - """<type>: Description of 'x'""" - -y = 2 -"""<type>: Description of 'y'""" -``` - -#### Avoid Reflection +### Avoid Reflection Avoid using reflection when functionality can be easily achieved without reflection. For example, instead of: @@ -126,7 +90,7 @@ def make_complex(x, y): return {'x': x, 'y': y} ``` -#### Error Handling +### Error Handling 1. When using try-except blocks, limit the except to the smallest set of errors possible. For example, instead of: @@ -172,9 +136,42 @@ else: f.read() ``` +### Configuration Defaults + +- **YAML is the single source of truth for defaults.** Do not set non-`None` defaults in the code for configuration values. The loaded YAML (and any user overrides) must supply required values. +- **Access config directly and expect presence.** For required attributes, write code like `policy_cfg["precision"]` and assume it is present. Do not introduce hidden defaults deep in the code (e.g., defaulting `policy.precision` to `"bfloat16"`). +- **Express optionality via `TypedDict`.** Use `typing.NotRequired` to mark optional attributes. Optional attributes may be absent/`None`; code may check for their presence. +- **Where defaults live.** Exemplar configs under `examples/configs/*.yaml` include documented defaults. Recipe YAMLs under `examples/configs/recipes/**/*.yaml` are runnable snapshots and may omit documentation. +- **Additions must be documented.** When adding a new config key to a `TypedDict` subclass, document the key’s purpose, valid values/types, and recommended default (if applicable), and reflect the default in the exemplar YAMLs under `examples/configs/*.yaml`. +- **Rationale.** Centralizing defaults in YAML avoids surprising behavior and makes value provenance clear. + +Forbidden patterns: + +```python +# Hidden default in code +precision = policy_cfg.get("precision", "bfloat16") + +# Function parameter defaulting a config value +def build_policy(policy_cfg, precision: str = "bfloat16"): + ... +``` + +Preferred patterns: + +```python +# Required attribute: expect it to come from YAML or user override +precision: str = policy_cfg["precision"] + +# Optional attribute: check for presence +if "milestones" in scheduler_cfg: + configure_milestones(scheduler_cfg["milestones"]) +``` + +See also: [TypedDict and Configuration Defaults](docs/design-docs/design-and-philosophy.md#typeddict-and-configuration-defaults). + ## Doc Guidelines -#### Ensure docs/index.md is up to date +### Ensure docs/index.md is up to date When a new markdown doc is added under `docs/**/*.md` or a markdown file is renamed, ensure that `docs/index.md` is updated and the document appears in the most appropriate section. diff --git a/tests/test_suites/README.md b/tests/test_suites/README.md index b262bc4075..e13b330c05 100644 --- a/tests/test_suites/README.md +++ b/tests/test_suites/README.md @@ -2,20 +2,43 @@ ## Naming -Each test is named: +Base pattern (LLM): + +``` +<algo>-<model>-<nodes>n<gpus>g-<strategy-and-params>[-modifiers][-long][.vN].sh +``` + +VLM pattern: + ``` -<algo>-<model>-#n#g-<parallelism>-<opt:long><opt:v$N>.sh +vlm_<algo>-<model>-<nodes>n<gpus>g-<strategy>[-modifiers][.vN].sh ``` +- **algo**: task or algorithm, e.g., `sft`, `dpo`, `grpo`. +- **model**: model identifier, e.g., `llama3.1-8b-instruct`, `qwen2.5-7b-instruct`. +- **nodes/gpus**: cluster allocation, e.g., `1n8g`, `4n8g`, `8n8g`. +- **strategy-and-params**: parallelism or framework detail, e.g., `fsdp2tp1`, `tp4pp2`, `megatron`, `dtensor2tp1`. +- **modifiers** (optional): short flags like `sp` (sequence packing), `actckpt` (activation checkpointing), `fp8`, `noncolocated`, `quick`. +- **-long** (optional): indicates long-running recipe. +- **.vN** (optional): version suffix (e.g., `.v2`, `.v3`) reserved for convergence-impacting changes. Use when the recipe's convergence behavior changes (dataset, loss, convergence bug fix). Pure performance changes do not require a version bump. + Examples: -* sft-llama3.2-1b-1n8g-fsdp2tp1.sh -* grpo-qwen2-1.5B-instruct-4n8g-fsdp2tp2.sh -* grpo-qwen2-1.5B-instruct-4n8g-fsdp2tp2-long.sh -* grpo-qwen2-1.5B-instruct-4n8g-fsdp2tp2-long.v2.sh - * The final verison suffix (starts with `.v2`, `.v3`, ...), is reserved for cases contributors believe the recipe's - convergence has changed due to their commit. Versioning signals that this recipe should not be compared to its - predecessor due to a change in convergence behavior. Examples of this change include: changing dataset, changing loss, - convergence bug fix. Changes affecting performance do not need a version change. + +``` +sft-llama3.1-8b-1n8g-fsdp2tp1-long.sh +dpo-llama3.1-8b-instruct-4n8g-fsdp2tp4.sh +grpo-llama3.1-8b-instruct-1n8g-megatron-fp8.sh +grpo-qwen2.5-7b-instruct-4n8g-fsdp2tp4sp.v3.sh +``` + +Known exceptions currently present: +- Deepscaler recipes encode context length in place of the cluster tuple, e.g., `grpo-deepscaler-1.5b-8K.sh`. These are allowed but should document the intended hardware in the script body. +- Some recipes include additional short flags in the strategy token (e.g., `fsdp2tp8sp`). Treat these as modifiers appended to the strategy. + +Directory placement and naming parity: +- Place driver scripts under `tests/test_suites/llm/` or `tests/test_suites/vlm/`. +- The script filename should mirror the YAML recipe filename under `examples/configs/recipes/**` but with a `.sh` suffix. +- Add the relative script path to `tests/test_suites/nightly.txt` for nightly execution. ## Running manually From b4934d90b44827276c37c653035067b74d34597f Mon Sep 17 00:00:00 2001 From: Terry Kong <terryk@nvidia.com> Date: Wed, 17 Sep 2025 19:58:46 +0000 Subject: [PATCH 3/3] remove not needed Signed-off-by: Terry Kong <terryk@nvidia.com> --- .coderabbit-current.yaml | 457 --------------------------------------- 1 file changed, 457 deletions(-) delete mode 100644 .coderabbit-current.yaml diff --git a/.coderabbit-current.yaml b/.coderabbit-current.yaml deleted file mode 100644 index f29c0fd099..0000000000 --- a/.coderabbit-current.yaml +++ /dev/null @@ -1,457 +0,0 @@ - # Set the language for reviews by using the corresponding ISO language code. - # Default: "en-US" - language: en-US - # Set the tone of reviews and chat. - # Default: "" - tone_instructions: '' # not in base - # Enable early-access features. - # Default: false - early_access: false # not in base - # Enable free tier features for users not on a paid plan. - # Default: true - enable_free_tier: true # not in base - tone_instructions: '' - early_access: false - enable_free_tier: true - # Settings related to reviews. - # Default: {} - reviews: - profile: chill - # Set the profile for reviews. Assertive profile yields more feedback, that may be considered nitpicky. - # Options: chill, assertive - # Default: "chill" - request_changes_workflow: false # not in base - # Approve the review once CodeRabbit’s comments are resolved and no pre-merge checks are in an error state. - # Default: false - high_level_summary: true # not in base - # Generate a high level summary of the changes in the PR/MR description. - # Default: true - high_level_summary_placeholder: '@coderabbitai summary' # not in base - # Placeholder in the PR/MR description that gets replaced with the high level summary. - # Default: "@coderabbitai summary" - high_level_summary_in_walkthrough: false # not in base - # Include the high level summary in the walkthrough comment. - # Default: false - auto_title_placeholder: '@coderabbitai' - # Add this keyword in the PR/MR title to auto-generate the title. - # Default: "@coderabbitai" - auto_title_instructions: '' - # Auto Title Instructions - Custom instructions for auto-generating the PR/MR title. - # Default: "" - review_status: true - # Post review details on each review. Additionally, post a review status when a review is skipped in certain cases. - # Default: true - commit_status: true - # Set the commit status to 'pending' when the review is in progress and 'success' when it is complete. - # Default: true - fail_commit_status: false - # Set the commit status to 'failure' when the PR cannot be reviewed by CodeRabbit for any reason. - # Default: false - collapse_walkthrough: false - # Generate walkthrough in a markdown collapsible section. - # Default: false - changed_files_summary: true # not in base - # Generate a summary of the changed files in the walkthrough. - # Default: true - sequence_diagrams: true # not in base - # Generate sequence diagrams in the walkthrough. - # Default: true - estimate_code_review_effort: true # not in base - # Estimate the code review effort in the walkthrough. - # Default: true - assess_linked_issues: true - # Generate an assessment of how well the changes address the linked issues in the walkthrough. - # Default: true - related_issues: true - # Include possibly related issues in the walkthrough. - # Default: true - related_prs: true - # Related PRs - Include possibly related pull requests in the walkthrough. - # Default: true - suggested_labels: true - # Suggest labels based on the changes in the pull request in the walkthrough. - # Default: true - auto_apply_labels: false # not in base - # Automatically apply the suggested labels to the PR/MR. - # Default: false - suggested_reviewers: true - # Suggest reviewers based on the changes in the pull request in the walkthrough. - # Default: true - auto_assign_reviewers: false # not in base - # Automatically assign suggested reviewers to the pull request - # Default: false - poem: true - # Generate a poem in the walkthrough comment. - # Default: true - labeling_instructions: [] # not in base - # Labeling Instructions - Provide guidelines for suggesting labels for the PR/MR. - # Default: [] - path_filters: [] # not in base - # Specify file patterns to include or exclude in a review using glob patterns. - # Default: [] - path_instructions: [] # not in base - # Path Instructions - Provide specific additional guidelines for code review based on file paths. - # Default: [] - abort_on_close: true # not in base - # Abort the in-progress review if the pull request is closed or merged. - # Default: true - disable_cache: false # not in base - # Disable caching of code and dependencies. - # Default: false - auto_review: - # Configuration for auto review - # Default: {} - enabled: true # not in base - # Automatic Review - Automatic code review - # Default: true - auto_incremental_review: true - # Automatic Incremental Review - Automatic incremental code review on each push - # Default: true - ignore_title_keywords: [] # not in base - # Ignore reviewing if the title of the pull request contains any of these keywords. - # Default: [] - labels: [] # not in base - # List of labels to control which PRs/MRs to review. - # Default: [] - drafts: false - # Review draft PRs/MRs. - # Default: false - base_branches: [] - # Base branches (other than the default branch) to review. Accepts regex patterns. - # Default: [] - ignore_usernames: [] # not in base - # Ignore reviewing pull requests by these usernames. - # Default: [] - finishing_touches: # not in base - # Configuration for finishing touches - # Default: {} - docstrings: - # Docstrings - Allow CodeRabbit to generate docstrings for PRs/MRs. - # Default: true - enabled: true - unit_tests: - # Unit Tests - Allow CodeRabbit to generate unit tests for PRs/MRs. - # Default: true - enabled: true - pre_merge_checks: # not in base - # Configuration for pre merge checks - # Default: {} - docstrings: - # Mode - Determines how strictly the docstring coverage check is enforced. - # Default: "warning" - mode: warning - # Percentage threshold for docstring coverage check. - # Default: 80 - threshold: 80 - title: - # Mode - Determines how strictly the title check is enforced. - # Default: "warning" - mode: warning - # Requirements - Requirements for the pull request title. - # Default: "" - requirements: '' - description: - # Mode - Determines how strictly the description check is enforced. - # Default: "warning" - mode: warning - issue_assessment: - # Mode - Determines how strictly the issue assessment check is enforced. - # Default: "warning" - mode: warning - custom_checks: [] - # Custom Pre-merge Checks - Add unique checks to enforce your team's standards before merging a pull request. - # Default: [] - tools: # not in base - # Tools that provide additional context to code reviews. - # Default: {} - ast-grep: - # List of rules directories. - # Default: [] - rule_dirs: [] - # List of utils directories. - # Default: [] - util_dirs: [] - # Use ast-grep essentials package. - # Default: true - essential_rules: true - # Predefined packages to be used. - # Default: [] - packages: [] - shellcheck: - # Enable ShellCheck - Enable ShellCheck integration. - # Default: true - enabled: true - ruff: - # Enable Ruff - Enable Ruff integration. - # Default: true - enabled: true - markdownlint: - # Enable markdownlint - Enable markdownlint integration. - # Default: true - enabled: true - github-checks: - # Enable GitHub Checks - Enable GitHub Checks integration. - # Default: true - enabled: true - # Time in milliseconds to wait for all GitHub Checks to conclude. - # Default: 90000 - timeout_ms: 90000 - languagetool: - # Enable LanguageTool - Enable LanguageTool integration. - # Default: true - enabled: true - # IDs of rules to be enabled. - # Default: [] - enabled_rules: [] - # IDs of rules to be disabled. - # Default: [] - disabled_rules: [] - # IDs of categories to be enabled. - # Default: [] - enabled_categories: [] - # IDs of categories to be disabled. - # Default: [] - disabled_categories: [] - # Only the rules and categories whose IDs are specified are enabled. - # Default: false - enabled_only: false - # Level of strictness for LanguageTool. - # Default: "default" - level: default - biome: - # Enable Biome - Enable Biome integration. - # Default: true - enabled: true - hadolint: - # Enable Hadolint - Enable Hadolint integration. - # Default: true - enabled: true - swiftlint: - # Enable SwiftLint - Enable SwiftLint integration. - # Default: true - enabled: true - phpstan: - # Enable PHPStan - Enable PHPStan integration. - # Default: true - enabled: true - # Level - Specify the rule level to run. - # Default: "default" - level: default - phpmd: - # Enable PHPMD - Enable PHPMD integration. - # Default: true - enabled: true - phpcs: - # Enable PHP CodeSniffer - Enable PHP CodeSniffer integration. - # Default: true - enabled: true - golangci-lint: - # Enable golangci-lint - Enable golangci-lint integration. - # Default: true - enabled: true - yamllint: - # Enable YAMLlint - Enable YAMLlint integration. - # Default: true - enabled: true - gitleaks: - # Enable Gitleaks - Enable Gitleaks integration. - # Default: true - enabled: true - checkov: - # Enable Checkov - Enable Checkov integration. - # Default: true - enabled: true - detekt: - # Enable detekt - Enable detekt integration. - # Default: true - enabled: true - eslint: - # Enable ESLint - Enable ESLint integration. - # Default: true - enabled: true - flake8: - # Enable Flake8 - Enable Flake8 integration. - # Default: true - enabled: true - rubocop: - # Enable RuboCop - Enable RuboCop integration. - # Default: true - enabled: true - buf: - # Enable Buf - Enable Buf integration. - # Default: true - enabled: true - regal: - # Enable Regal - Enable Regal integration. - # Default: true - enabled: true - actionlint: - # Enable actionlint - Enable actionlint integration. - # Default: true - enabled: true - pmd: - # Enable PMD - Enable PMD integration. - # Default: true - enabled: true - cppcheck: - # Enable Cppcheck - Enable Cppcheck integration. - # Default: true - enabled: true - semgrep: - # Enable Semgrep - Enable Semgrep integration. - # Default: true - enabled: true - circleci: - # Enable CircleCI - Enable CircleCI integration. - # Default: true - enabled: true - clippy: - # Enable Clippy - Enable Clippy integration. - # Default: true - enabled: true - sqlfluff: - # Enable SQLFluff - Enable SQLFluff integration. - # Default: true - enabled: true - prismaLint: - # Enable Prisma Schema linting - # Default: true - enabled: true - pylint: - # Enable Pylint - Enable Pylint integration. - # Default: true - enabled: true - oxc: - # Enable Oxlint - Enable Oxlint integration. - # Default: true - enabled: true - shopifyThemeCheck: - # Enable Shopify Theme Check - # Default: true - enabled: true - luacheck: - # Enable Lua code linting - Enable Luacheck integration. - # Default: true - enabled: true - brakeman: - # Enable Brakeman - Enable Brakeman integration. - # Default: true - enabled: true - dotenvLint: - # Enable dotenv-linter - Enable dotenv-linter integration. - # Default: true - enabled: true - htmlhint: - # Enable HTMLHint - Enable HTMLHint integration. - # Default: true - enabled: true - checkmake: - # Enable checkmake - Enable checkmake integration. - # Default: true - enabled: true - osvScanner: - # Enable OSV Scanner - Enable OSV Scanner integration. - # Default: true - enabled: true - # Configuration for chat - # Default: {} - chat: # not in base - art: true - # Generate art in response to chat messages. - # Default: true - auto_reply: true - # Enable the bot to reply automatically without requiring the user to tag it. - # Default: true - integrations: - # Configuration for integrations - # Default: {} - jira: - # Jira - Enable the Jira integration for opening issues, etc. - # Default: "auto" - usage: auto - linear: - # Linear - Enable the Linear integration for opening issues, etc. - # Default: "auto" - usage: auto -knowledge_base: - # Configuration for knowledge base - # Default: {} - opt_out: false # not in base - # Opt Out - Disable all knowledge base features that require data retention. - # Default: false - web_search: - # Configuration for web search - # Default: {} - enabled: true # not in base - # Web Search - Enable the web search integration. - # Default: true - code_guidelines: - enabled: true - # CodeRabbit will analyse and learn from your organization's code guidelines. - # Default: {} - filePatterns: [] - # File Patterns - Specify files for your coding guideline documents. - # Default: [] - learnings: - # Configuration for learnings - # Default: {} - scope: auto # not in base - # Learnings - Specify the scope of learnings to use for the knowledge base. - # Default: "auto" - issues: - # Configuration for issues - # Default: {} - scope: auto # not in base - # Issues - Specify the scope of git platform issues to use for the knowledge base. - # Default: "auto" - jira: - # Configuration for jira - # Default: {} - usage: auto # not in base - # Jira - Enable the Jira knowledge base integration. - # Default: "auto" - project_keys: [] # not in base - # Jira Project Keys - Specify the Jira project keys to use for the knowledge base. - # Default: [] - linear: - # Configuration for linear - # Default: {} - usage: auto # not in base - # Linear - Enable the Linear knowledge base integration. - # Default: "auto" - team_keys: [] # not in base - # Linear Team Keys - Specify the Linear team keys to use for the knowledge base. - # Default: [] - pull_requests: - # Configuration for pull requests - # Default: {} - scope: auto # not in base - # Pull Requests - Specify the scope of pull requests to use for the knowledge base. - # Default: "auto" - mcp: - # Configuration for mcp - # Default: {} - usage: auto # not in base - # MCP - Enable the MCP knowledge base integration. - # Default: "auto" - disabled_servers: [] # not in base - # MCP Disabled Servers - Specify MCP server labels to disable. - # Default: [] -code_generation: # not in base - # Configuration for code generation - # Default: {} - docstrings: - # Settings related to the generation of docstrings. - # Default: {"path_instructions":[]} - language: en-US # not in base - # Set the language for docstrings by using the corresponding ISO language code. - # Default: "en-US" - path_instructions: [] # not in base - # Path Instructions - Provide additional guidelines for docstring generation based on file paths. - # Default: [] - unit_tests: - # Settings related to the generation of unit tests. - # Default: {"path_instructions":[]} - path_instructions: [] # not in base - # Unit Test Generation - Provide additional guidelines for unit test generation based on file paths. - # Default: [] \ No newline at end of file