Refactor cli and runner. Implement reserved config keys. Add feature to pass args to Trainer's methods. #124
Conversation
WalkthroughThe recent changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Runner
participant ConfigParser
participant TrainerTuner
User->>CLI: lighter fit --config config.yaml
CLI->>Runner: cli() function call
Runner->>ConfigParser: parse_config(config.yaml)
ConfigParser-->>Runner: parsed config
Runner->>ConfigParser: validate_config(parsed config)
ConfigParser-->>Runner: validation success
Runner->>TrainerTuner: run(trainer_method, parsed config)
TrainerTuner-->>Runner: training result
Runner-->>CLI: output result
CLI-->>User: result displayed
Poem
Warning Review ran into problemsProblems (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
dosubot
bot
added
the
size:L
![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
Outside diff range and nitpick comments (3)
tests/integration/test_cifar.py (1)
21-21: Add a descriptive docstring to the test function.It's good practice to include a brief description of what the test does, which can be helpful for future maintenance or for other developers who might work on this test.
docs/basics/quickstart.md (1)
Line range hint
1-1: Address markdown lint issues.- Trailing spaces - Multiple consecutive blank lines - Headings should be surrounded by blank lines - Fenced code blocks should be surrounded by blank lines - Code block stylePlease fix the markdown lint issues as per the hints provided by the static analysis tool.
Also applies to: 2-2, 17-17, 19-19, 92-92, 15-15, 135-135, 16-16, 24-24, 38-38, 84-84, 11-11, 30-30, 93-93
Tools
Markdownlint
135-135: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank linesdocs/index.md (1)
Line range hint
7-7: Address markdown lint issues.- Multiple top-level headings in the same document - Fenced code blocks should be surrounded by blank lines - Images should have alternate text (alt text) - Code block stylePlease fix the markdown lint issues as per the hints provided by the static analysis tool.
Also applies to: 18-18, 10-10, 54-54
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- docs/basics/projects.md (1 hunks)
- docs/basics/quickstart.md (1 hunks)
- docs/index.md (1 hunks)
- lighter/utils/logging.py (1 hunks)
- lighter/utils/runner.py (1 hunks)
- pyproject.toml (1 hunks)
- tests/integration/test_cifar.py (2 hunks)
Files skipped from review due to trivial changes (2)
- lighter/utils/logging.py
- pyproject.toml
Additional context used
LanguageTool
docs/basics/quickstart.md
[uncategorized] ~10-~10: If this is a compound adjective that modifies the following noun, use a hyphen. (EN_COMPOUND_ADJECTIVE_INTERNAL)
Context: ...on pip install project-lighterFor bleeding edge version, runpython pip install proj...
[duplication] ~24-~24: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...nents: - Trainer - LighterSystem ### Trainer Trainer contains all the information about runn...
[duplication] ~38-~38: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...nformation see here ### LighterSystem LighterSystem encapsulates all parts of a deep learni...
[uncategorized] ~41-~41: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...riterion, etc.) This provides powerful extensibility as training experiments for classificat...docs/basics/projects.md
[uncategorized] ~9-~9: It seems likely that a singular genitive (’s) apostrophe is missing. (AI_HYDRA_LEO_APOSTROPHE_S_XS)
Context: ...] Customization per your imagination! Lets start by looking at each of these one b...
[uncategorized] ~14-~14: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...dataset If you are reproducing another study you often start with a pre-defined conf...
[style] ~96-~96: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 2135 characters long) (EN_EXCESSIVE_EXCLAMATION)
Context: ...ed to do is add it to the lighter config! But wait, how will Lighter know where y...
Markdownlint
docs/basics/quickstart.md
1-1: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
2-2: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
17-17: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
19-19: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
92-92: Expected: 0 or 2; Actual: 6 (MD009, no-trailing-spaces)
Trailing spaces
15-15: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
135-135: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
1-1: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
16-16: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
24-24: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
38-38: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
84-84: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
11-11: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
30-30: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
93-93: Expected: fenced; Actual: indented (MD046, code-block-style)
Code block styledocs/index.md
7-7: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
18-18: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
10-10: null (MD045, no-alt-text)
Images should have alternate text (alt text)
54-54: Expected: fenced; Actual: indented (MD046, code-block-style)
Code block styledocs/basics/projects.md
12-12: Expected: h2; Actual: h3 (MD001, heading-increment)
Heading levels should only increment by one level at a time
8-8: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
96-96: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
135-135: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
142-142: Expected: 0 or 2; Actual: 3 (MD009, no-trailing-spaces)
Trailing spaces
95-95: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
103-103: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
136-136: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
104-104: Expected: fenced; Actual: indented (MD046, code-block-style)
Code block style
Additional comments not posted (5)
tests/integration/test_cifar.py (1)
5-5: The updates to use the newrunfunction and theconfigparameter are correctly implemented.Also applies to: 11-11, 23-25
docs/basics/quickstart.md (1)
132-132: The update to the command-line argument is correctly reflected in the documentation.docs/index.md (1)
102-102: The update to the command-line argument is correctly reflected in the documentation.lighter/utils/runner.py (1)
16-19: The implementation of the CLI interface and therunfunction are well-structured and align with the PR objectives.Also applies to: 72-111
docs/basics/projects.md (1)
139-139: Update CLI command to match new syntax.The change from
--config_fileto--configaligns with the PR's objective to simplify command syntax.
lighter/utils/runner.py
Outdated
| config = kwargs.pop("config", None) | ||
| if config is None: | ||
| raise ValueError("'--config' not specified. Exiting.") |
There was a problem hiding this comment.
Improve error handling and validation.
- raise ValueError("'--config' not specified. Exiting.")
+ raise ValueError("'--config' not specified. Please provide a valid configuration file.")
- raise ValueError(f"Invalid top-level config keys: {list(invalid_keys)}. Allowed keys: {list(CONFIG_STRUCTURE.keys())}")
+ raise ValueError(f"Invalid top-level config keys: {invalid_keys}. Allowed keys are: {CONFIG_STRUCTURE.keys()}")
- raise ValueError(f"Invalid method names in 'args': {invalid_keys}. Allowed methods are: {TRAINER_METHOD_NAMES}")
+ raise ValueError(f"Invalid method names in 'args': {invalid_keys}. Allowed methods are: {TRAINER_METHOD_NAMES}")Consider rephrasing the error messages for better clarity and consistency.
Also applies to: 63-63, 69-69
Committable suggestion was skipped due to low confidence.
|
https://data.safetycli.com/v/70612/97c/ Some kind of jinja2 vulnerability that the maintainers disagree with is preventing the checks from running @surajpaib |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
Files selected for processing (2)
- lighter/utils/runner.py (1 hunks)
- pyproject.toml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
Additional comments not posted (5)
lighter/utils/runner.py (5)
16-20: Refactor the CLI definition to use thefirelibrary.This change simplifies the CLI definition by dynamically creating commands based on the
TRAINER_METHOD_NAMES. It's a clean and scalable approach.
33-35: Ensure a configuration file is specified.The addition of a check to ensure that a configuration file is specified before proceeding is a good practice to avoid runtime errors.
97-99: Validate Trainer method existence.The validation that checks if the
trainerobject has the method specified is crucial to prevent runtime errors. This ensures that only valid methods are called, aligning with the robustness of the system.
105-105: Handle special keys in configuration.Removing the 'meta' key from the configuration before saving hyperparameters is a good practice to clean up internal metadata that should not be exposed or logged.
111-111: Execute the Trainer method dynamically.Using
getattrto dynamically call a method on thetrainerobject based on the method name provided is a flexible approach that supports the extensibility of the Trainer class.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lighter/utils/runner.py (1 hunks)
Additional comments not posted (3)
lighter/utils/runner.py (3)
16-19: The implementation of the CLI using thefirelibrary is clean and effective.
33-41: Ensure the mandatory 'config' parameter is well-documented in user guides or help sections, as this is a significant change.
72-111: The sequence of operations in therunfunction is well-structured and ensures robust setup before method execution.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lighter/utils/runner.py (1 hunks)
Additional comments not posted (5)
lighter/utils/runner.py (5)
3-5: Imports added forpartial,fire, andConfigParserare appropriate for the new functionalities introduced.
9-9: Import ofLighterSystemis necessary for the system configuration handling.
12-13: TheCONFIG_STRUCTUREandTRAINER_METHOD_NAMESare well-defined and align with the new configuration validation logic.
16-19: Thecli()function effectively leverages thefirelibrary to map trainer methods to command line arguments, which simplifies the CLI interface.
44-58: Thevalidate_configfunction correctly ensures that only valid keys and trainer methods are used, which is crucial for maintaining configuration integrity.
| def run(method: str, **kwargs: Any): | ||
| """Run the trainer method. | ||
|
|
||
| Args: | ||
| method (str): name of the Trainer method to run. ["fit", "validate", "test", "predict", "tune"]. | ||
| **kwargs (Any): keyword arguments passed to the `monai.bundle.run` function. | ||
| method (str): name of the trainer method to run. | ||
| **kwargs (Any): keyword arguments that include 'config' and specific config overrides passed to `parse_config()`. | ||
| """ | ||
| # Sets the random seed to `PL_GLOBAL_SEED` env variable. If not specified, it picks a random seed. | ||
| seed_everything() | ||
|
|
||
| # Parse the config file(s). | ||
| # Parse and validate the config. | ||
| parser = parse_config(**kwargs) | ||
| validate_config(parser) | ||
|
|
||
| # Get trainer and system | ||
| trainer = parser.get_parsed_content("trainer") | ||
| # Import the project folder as a module, if specified. | ||
| project = parser.get_parsed_content("project") | ||
| if project is not None: | ||
| import_module_from_path("project", project) | ||
|
|
||
| # Get the main components from the parsed config. | ||
| system = parser.get_parsed_content("system") | ||
| trainer = parser.get_parsed_content("trainer") | ||
| trainer_method_args = parser.get_parsed_content(f"args#{method}", default={}) | ||
|
|
||
| # Checks | ||
| if not isinstance(system, LighterSystem): | ||
| raise ValueError(f"Expected 'system' to be an instance of LighterSystem, got {system.__class__.__name__}.") | ||
| if not hasattr(trainer, method): | ||
| raise ValueError(f"{trainer.__class__.__name__} has no method named '{method}'.") | ||
| if any("dataloaders" in key or "datamodule" in key for key in trainer_method_args): | ||
| raise ValueError("All dataloaders should be defined as part of the LighterSystem, not passed as method arguments.") | ||
|
|
||
| # Save the config to checkpoints under "hyper_parameters" and log it if a logger is defined. | ||
| config = parser.get() | ||
| config.pop("_meta_") | ||
| # Save the config to model checkpoints under the "hyper_parameters" key. | ||
| config.pop("_meta_") # MONAI Bundle adds this automatically, remove it. | ||
| system.save_hyperparameters(config) | ||
| # Log the config. | ||
| if trainer.logger is not None: | ||
| trainer.logger.log_hyperparams(config) | ||
|
|
||
| # Run the Trainer method. | ||
| if not hasattr(trainer, method): | ||
| raise ValueError(f"Trainer has no method named {method}.") | ||
| getattr(trainer, method)(system) | ||
| # Run the trainer method. | ||
| getattr(trainer, method)(system, **trainer_method_args) |
There was a problem hiding this comment.
The run function is well-structured and includes comprehensive checks and validations to ensure the trainer method is executed correctly. However, consider adding more detailed logging for debugging purposes.
+ if trainer.logger is not None:
+ trainer.logger.debug(f"Running method: {method} with args: {trainer_method_args}")Committable suggestion was skipped due to low confidence.
Description
Related Issue
#99
#119 (comment)
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.make codestyle.Summary by CodeRabbit
Documentation
--config_fileto--config.Refactor
fire.Chores
lightercommand to streamline command execution.Tests