chore: additional workflow template cleanup #899
Conversation
WalkthroughRemoved logging and front_end blocks and a retry setting from the workflow config template; added docstrings and switched FunctionInfo.description to use the inner function's docstring; updated the register template to import the workflow symbol suffixed with Changes
Sequence Diagram(s):Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
willkill07
added
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nat/cli/commands/workflow/templates/workflow.py.j2 (1)
1-1: Add Apache License 2.0 header.The file is missing the required Apache License 2.0 header comment at the top. This is required by the coding guidelines for all code files except .mdc files.
Add the Apache License 2.0 header at the top of the file:
+# 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. + import loggingAs per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/nat/cli/commands/workflow/templates/config.yml.j2(0 hunks)src/nat/cli/commands/workflow/templates/workflow.py.j2(1 hunks)
💤 Files with no reviewable changes (1)
- src/nat/cli/commands/workflow/templates/config.yml.j2
🧰 Additional context used
📓 Path-based instructions (2)
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/cli/commands/workflow/templates/workflow.py.j2
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/cli/commands/workflow/templates/workflow.py.j2
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/cli/commands/workflow/templates/workflow.py.j2
⏰ 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). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (2)
src/nat/cli/commands/workflow/templates/workflow.py.j2 (2)
23-33: LGTM!The added docstring clearly describes the function's purpose, arguments, and return value. It provides helpful context about the registration mechanism and follows Google-style docstring conventions.
48-50: LGTM!Using
_echo.__doc__to dynamically source the description is a good practice. It maintains the DRY principle and ensures the description always matches the function's docstring, improving maintainability.
willkill07
changed the title
Signed-off-by: Will Killian <[email protected]>
Signed-off-by: Will Killian <[email protected]>
willkill07
force-pushed
the
wkk_template-update
branch
from
75d3f22 to
9db2545
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/nat/cli/commands/workflow/templates/config.yml.j2(0 hunks)src/nat/cli/commands/workflow/templates/register.py.j2(1 hunks)src/nat/cli/commands/workflow/templates/workflow.py.j2(1 hunks)
💤 Files with no reviewable changes (1)
- src/nat/cli/commands/workflow/templates/config.yml.j2
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nat/cli/commands/workflow/templates/workflow.py.j2
🧰 Additional context used
📓 Path-based instructions (2)
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/cli/commands/workflow/templates/register.py.j2
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/cli/commands/workflow/templates/register.py.j2
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/cli/commands/workflow/templates/register.py.j2
🔇 Additional comments (1)
src/nat/cli/commands/workflow/templates/register.py.j2 (1)
4-4: Verify backward compatibility for symbol name change.
The import inregister.py.j2now references{{ python_safe_workflow_name }}_functioninstead of{{ python_safe_workflow_name }}. Confirm existing generated workflows won’t break by ensuring the old symbol is still exported or aliased, or that migration docs cover this change.Run:
#!/bin/bash # Check exported symbols in the registration template rg -n "^(def |__all__|from .* import)" src/nat/cli/commands/workflow/templates/register.py.j2
Signed-off-by: Will Killian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/nat/cli/commands/workflow/templates/register.py.j2 (1)
1-2: The Apache License 2.0 header is still missing.This concern was raised in a previous review and remains unaddressed. Per coding guidelines, all code files (including Jinja2 templates that generate code) must include the Apache License 2.0 header at the top.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nat/cli/commands/workflow/templates/register.py.j2(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/cli/commands/workflow/templates/register.py.j2
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/cli/commands/workflow/templates/register.py.j2
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/cli/commands/workflow/templates/register.py.j2
zhongxuanwang-nv
left a comment
zhongxuanwang-nv
left a comment
There was a problem hiding this comment.
Thank you Will!!
|
/merge |
Cleans up the default configuration + workflow template a little more ## By Submitting this PR I confirm: - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. ## Summary by CodeRabbit - New Features - None - Refactor - Simplified generated workflow configuration by removing redundant logging and frontend blocks and a deprecated retry option. - Aligned workflow registration naming to a consistent convention. - Documentation - Added comprehensive docstrings to the workflow and helper to improve clarity and editor hints. - Chores - Cleaned up default config output to reduce noise and simplify generated templates. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Eric Evans II (https://github.com/ericevans-nv) URL: NVIDIA#899 Signed-off-by: Yuchen Zhang <[email protected]>
Cleans up the default configuration + workflow template a little more ## By Submitting this PR I confirm: - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. ## Summary by CodeRabbit - New Features - None - Refactor - Simplified generated workflow configuration by removing redundant logging and frontend blocks and a deprecated retry option. - Aligned workflow registration naming to a consistent convention. - Documentation - Added comprehensive docstrings to the workflow and helper to improve clarity and editor hints. - Chores - Cleaned up default config output to reduce noise and simplify generated templates. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Eric Evans II (https://github.com/ericevans-nv) URL: NVIDIA#899 Signed-off-by: Yuchen Zhang <[email protected]>
Cleans up the default configuration + workflow template a little more ## By Submitting this PR I confirm: - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. ## Summary by CodeRabbit - New Features - None - Refactor - Simplified generated workflow configuration by removing redundant logging and frontend blocks and a deprecated retry option. - Aligned workflow registration naming to a consistent convention. - Documentation - Added comprehensive docstrings to the workflow and helper to improve clarity and editor hints. - Chores - Cleaned up default config output to reduce noise and simplify generated templates. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Eric Evans II (https://github.com/ericevans-nv) URL: NVIDIA#899 Signed-off-by: Yuchen Zhang <[email protected]>
3 participants
Description
Cleans up the default configuration + workflow template a little more
By Submitting this PR I confirm:
Summary by CodeRabbit