Template generator #3938
Open
Template generator #3938
+142
−44
Conversation
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
klakhi
requested review from
Mayankm96,
hhansen-bdai and
kellyguo11
as code owners
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds non-interactive mode to the template generator CLI, allowing automated project scaffolding without user prompts. Users can now pass configuration via command-line arguments (--non-interactive, --project-name, --project-path, --task-type, --workflow, --rl-library, --rl-algorithm).
Key changes:
tools/template/cli.py: Added argparse for command-line argument parsing and conditional logic to use provided arguments in non-interactive mode or fall back to interactive promptsisaaclab.shandisaaclab.bat: Modified to detect and handle the--non-interactiveflagCONTRIBUTORS.md: Added contributor name
Issues found:
- Both shell scripts pass
--non-interactivetwice when the flag is detected (once explicitly, once in$@/!allArgs!). While argparse handles this gracefully, it's redundant and should be fixed by removing the explicit flag from the conditional command.
Confidence Score: 3/5
- Safe to merge after fixing the duplicate flag issue in shell scripts
- The core Python implementation is solid with proper validation and error handling. However, both shell scripts have a logic bug where
--non-interactiveis passed twice. While this doesn't break functionality (argparse handles duplicates), it's redundant and indicates the conditional logic is incorrect. The fix is simple - remove the explicit--non-interactivefrom the command when the condition is true. - Pay attention to
isaaclab.shandisaaclab.bat- both need the duplicate flag issue fixed
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| isaaclab.sh | 2/5 | Added non-interactive mode support for template generator, but passes --non-interactive flag twice |
| isaaclab.bat | 2/5 | Added non-interactive mode support with argument quoting, but passes --non-interactive flag twice in both -n and --new handlers |
| tools/template/cli.py | 4/5 | Implemented comprehensive non-interactive mode with argparse flags and validation, enabling CI/CD automation |
Sequence Diagram
sequenceDiagram
participant User
participant Shell as isaaclab.sh/bat
participant CLI as cli.py
participant ArgParser as argparse
participant Generator as generator.py
User->>Shell: ./isaaclab.sh --new --non-interactive --project-name test
Shell->>Shell: Detect --non-interactive flag
Shell->>Shell: Build allArgs including --non-interactive
alt non-interactive mode detected
Shell->>CLI: python cli.py --non-interactive [allArgs with --non-interactive]
Note over Shell,CLI: Bug: --non-interactive passed twice
else interactive mode
Shell->>CLI: python cli.py [allArgs]
end
CLI->>ArgParser: Parse arguments
ArgParser->>ArgParser: Handle --non-interactive, --project-name, etc.
ArgParser-->>CLI: Return parsed args
alt non-interactive mode
CLI->>CLI: Use command-line argument values
CLI->>CLI: Validate required parameters
alt validation fails
CLI->>User: Raise SystemExit with error
end
else interactive mode
CLI->>User: Prompt for task type, project path, etc.
User-->>CLI: Provide inputs
end
CLI->>CLI: Build specification dict
CLI->>Generator: generate(specification)
Generator->>User: Create project files
4 files reviewed, 3 comments
Comment on lines
+711
to
+712
| if [[ " $* " == *" --non-interactive "* ]]; then | ||
| ${python_exe} ${ISAACLAB_PATH}/tools/template/cli.py --non-interactive $@ |
There was a problem hiding this comment.
logic: --non-interactive flag is passed twice when already present in $@
Suggested change
| if [[ " $* " == *" --non-interactive "* ]]; then | |
| ${python_exe} ${ISAACLAB_PATH}/tools/template/cli.py --non-interactive $@ | |
| ${python_exe} ${ISAACLAB_PATH}/tools/template/cli.py $@ |
Comment on lines
+580
to
+581
| if "!isNonInteractive!"=="1" ( | ||
| call !python_exe! tools\template\cli.py --non-interactive !allArgs! |
There was a problem hiding this comment.
logic: --non-interactive flag is passed twice when already present in !allArgs!
Suggested change
| if "!isNonInteractive!"=="1" ( | |
| call !python_exe! tools\template\cli.py --non-interactive !allArgs! | |
| call !python_exe! tools\template\cli.py !allArgs! |
Comment on lines
+607
to
+608
| if "!isNonInteractive!"=="1" ( | ||
| call !python_exe! tools\template\cli.py --non-interactive !allArgs! |
There was a problem hiding this comment.
logic: --non-interactive flag is passed twice when already present in !allArgs!
Suggested change
| if "!isNonInteractive!"=="1" ( | |
| call !python_exe! tools\template\cli.py --non-interactive !allArgs! | |
| call !python_exe! tools\template\cli.py !allArgs! |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds non-interactive mode support to the template generator CLI, enabling automation through command-line arguments.
Key changes:
- Added
argparseto parse command-line arguments (--non-interactive,--task-type,--project-path,--project-name,--workflow,--rl-library,--rl-algorithm) - Implemented conditional logic to bypass interactive prompts when
--non-interactiveflag is set - Added validation for required arguments in non-interactive mode with appropriate error messages
- Supports comma-separated values and "all" keyword for multi-select options
Issues found:
- Critical: RL algorithm selection logic reuses the same
--rl_algorithmvalue for all selected RL libraries, which causes incorrect validation since each library has different supported algorithms - The
get_algorithms_per_rl_libraryfunction signature was changed to acceptboolinstead of relying on length checks (line 302-303)
Confidence Score: 2/5
- This PR has a critical logical flaw in multi-library RL algorithm selection that will cause runtime errors
- The RL algorithm selection logic (lines 307-318) reuses the same
--rl_algorithmargument value across all selected RL libraries. Since different libraries support different algorithms with potentially different naming conventions, this will cause incorrect validation failures or selection of unsupported algorithms. The duplicate--non-interactiveflag issues in shell scripts were already caught in previous comments. - tools/template/cli.py requires immediate attention for the RL algorithm selection logic
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| tools/template/cli.py | 2/5 | Added non-interactive mode with argparse for CLI automation. Found critical issue: RL algorithm selection reuses same value for all libraries. |
Sequence Diagram
sequenceDiagram
participant User
participant CLI as cli.py
participant Parser as argparse
participant Handler as CLIHandler
participant Generator as generator.py
User->>CLI: Run with args
CLI->>Parser: Parse arguments
Parser-->>CLI: args object
alt non-interactive mode
CLI->>CLI: Validate task_type
CLI->>CLI: Validate project_path
CLI->>CLI: Validate project_name
CLI->>CLI: Parse workflow from string
CLI->>CLI: Parse rl_library from string
CLI->>CLI: Parse rl_algorithm from string
Note over CLI: Issue: Same rl_algorithm<br/>used for all libraries
else interactive mode
CLI->>Handler: input_select/input_checkbox
Handler-->>CLI: User selections
end
CLI->>Generator: get_algorithms_per_rl_library()
Generator-->>CLI: Supported algorithms
CLI->>CLI: Build specification dict
CLI->>Generator: generate(specification)
Generator->>Generator: Create project files
Generator-->>User: Project generated
1 file reviewed, 2 comments
Comment on lines
+307
to
+318
| if args.non_interactive: | ||
| if args.rl_algorithm is not None: | ||
| provided_algorithms = [item.strip().lower() for item in args.rl_algorithm.split(",") if item.strip()] | ||
| if "all" in provided_algorithms: | ||
| selected_algorithms = [item.lower() for item in algorithms] | ||
| else: | ||
| valid_algorithms = [item for item in provided_algorithms if item in [a.lower() for a in algorithms]] | ||
| if not valid_algorithms: | ||
| raise SystemExit(f"No valid --rl_algorithm provided for library '{rl_library}'") | ||
| selected_algorithms = valid_algorithms | ||
| else: | ||
| selected_algorithms = [item.lower() for item in algorithms] |
There was a problem hiding this comment.
logic: Same --rl_algorithm value is reused for all RL libraries
When multiple RL libraries are selected, the same --rl_algorithm value is used for all of them. Each library has different supported algorithms, so this causes invalid algorithm selection.
Example: User passes --rl-library=rl_games,skrl --rl-algorithm=PPO. The code validates PPO against rl_games algorithms (valid), but then reuses the same PPO validation for skrl algorithms, which may fail if skrl has different algorithm names.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
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