Switch from manual to pydantic validation of config and LighterSystem #135
Conversation
WalkthroughThe changes involve updates to type annotations across multiple files, primarily replacing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ConfigSchema
participant LighterSystem
User->>CLI: Run command
CLI->>ConfigSchema: Validate configuration
ConfigSchema-->>CLI: Return validated config
CLI->>LighterSystem: Initialize with config
LighterSystem-->>CLI: Confirm initialization
CLI-->>User: Display success message
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@ibro45 Can you fix the style checks? |
|
The PR is still under construction - don't like several things and am taking some time to think out of the box. Honestly, this whole validation thing is shit. I wish it would be purely Monai Bundle stuff, enforced with Right now, I'm considering introducing a This crossed my mind yesterday. I still have to look into whether it's possible and if it just ends up complicating it. But this could potentially solve a lot of problems rn - maybe it would even open up a way to nicely re-design how data loaders are defined (not clear to me how rn, but food for thought). Let me know what you think. |
…el_dump() and remove SubscriptableBaseModel
Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.9.5 to 3.10.2. - [Release notes](https://github.com/aio-libs/aiohttp/releases) - [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst) - [Commits](aio-libs/aiohttp@v3.9.5...v3.10.2) --- updated-dependencies: - dependency-name: aiohttp dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ibrahim Hadzic <[email protected]>
Bumps [zipp](https://github.com/jaraco/zipp) from 3.19.0 to 3.19.1. - [Release notes](https://github.com/jaraco/zipp/releases) - [Changelog](https://github.com/jaraco/zipp/blob/main/NEWS.rst) - [Commits](jaraco/zipp@v3.19.0...v3.19.1) --- updated-dependencies: - dependency-name: zipp dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ibrahim Hadzic <[email protected]>
Bumps [certifi](https://github.com/certifi/python-certifi) from 2024.2.2 to 2024.7.4. - [Commits](certifi/python-certifi@2024.02.02...2024.07.04) --- updated-dependencies: - dependency-name: certifi dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ibrahim Hadzic <[email protected]>
…Forbid extra fields pydantic.
dosubot
bot
added
the
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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 (10)
- lighter/callbacks/utils.py (2 hunks)
- lighter/callbacks/writer/base.py (3 hunks)
- lighter/callbacks/writer/file.py (4 hunks)
- lighter/system.py (8 hunks)
- lighter/utils/logging.py (1 hunks)
- lighter/utils/misc.py (1 hunks)
- lighter/utils/patches.py (1 hunks)
- lighter/utils/runner.py (4 hunks)
- lighter/utils/schema.py (1 hunks)
- pyproject.toml (1 hunks)
Files skipped from review due to trivial changes (4)
- lighter/callbacks/utils.py
- lighter/callbacks/writer/base.py
- lighter/utils/logging.py
- lighter/utils/misc.py
Additional comments not posted (35)
lighter/utils/patches.py (8)
18-20: LGTM!The
__init__method is correctly implemented.The code changes are approved.
22-34: LGTM!The
__setitem__method is correctly implemented and ensures unique internal keys.The code changes are approved.
36-47: LGTM!The
__getitem__method is correctly implemented.The code changes are approved.
49-57: LGTM!The
__delitem__method is correctly implemented.The code changes are approved.
59-70: LGTM!The
__contains__method is correctly implemented.The code changes are approved.
72-79: LGTM!The
keysmethod is correctly implemented.The code changes are approved.
81-88: LGTM!The
itemsmethod is correctly implemented.The code changes are approved.
90-97: LGTM!The
valuesmethod is correctly implemented.The code changes are approved.
lighter/callbacks/writer/file.py (3)
9-9: LGTM!The import statement for
Tensorfromtorchis correctly added.The code changes are approved.
41-41: LGTM!The type annotation for the
writemethod is correctly updated to useTensorinstead oftorch.Tensor.The code changes are approved.
82-82: LGTM!The type annotation for the
write_itk_imagefunction is correctly updated to useTensorinstead oftorch.Tensor.The code changes are approved.
lighter/utils/runner.py (6)
12-12: LGTM!The import statement for
ArgsConfigSchemaandConfigSchemais correctly added.The code changes are approved.
17-18: LGTM!The
clifunction is correctly updated to dynamically generate commands based on the schema.The code changes are approved.
35-45: LGTM!The
parse_configfunction is correctly updated to use the schema for configuration validation.The code changes are approved.
49-63: LGTM!The
runfunction is correctly updated to accept multiple methods and validate them against the schema.The code changes are approved.
Line range hint
66-86: LGTM!The
runfunction is correctly updated to use theparse_configfunction for configuration parsing.The code changes are approved.
90-97: LGTM!The
runfunction is correctly updated to execute the specified methods of theTrainerorTuner.The code changes are approved.
lighter/utils/schema.py (9)
17-39: LGTM!The
ArgsConfigSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
42-48: LGTM!The
ConfigSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
54-58: LGTM!The
DatasetSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
61-65: LGTM!The
SamplerSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
68-72: LGTM!The
CollateFnSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
75-85: LGTM!The
MetricsSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
88-92: LGTM!The
ModeSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
95-98: LGTM!The
DataSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
101-105: LGTM!The
PostprocessingSchemaclass is well-structured and effectively uses Pydantic's validation.The code changes are approved.
pyproject.toml (2)
47-47: LGTM!The addition of
numpywith the version constraint< 2.0.0is appropriate.The code changes are approved.
49-49: LGTM!The addition of
pydanticwith the version constraint^2.8.2is appropriate.The code changes are approved.
lighter/system.py (7)
100-106: LGTM!The
__init__method is well-structured and effectively uses schema validation.The code changes are approved.
Line range hint
115-134: LGTM!The
forwardmethod is well-structured and uses theTensortype appropriately.The code changes are approved.
136-147: LGTM!The
configure_optimizersmethod is well-structured and ensures that the optimizer and scheduler are correctly configured.The code changes are approved.
149-175: LGTM!The
setupmethod is well-structured and ensures that the dataloaders and step methods are correctly set up based on the training stage.The code changes are approved.
177-217: LGTM!The
_base_dataloadermethod is well-structured and ensures that the dataloader is correctly instantiated for a given mode. The use of a collate function to handle corrupted examples is appropriate.The code changes are approved.
Line range hint
218-309: LGTM!The
_base_stepmethod is well-structured and ensures that the base step is correctly performed for all modes. The inclusion of postprocessing and validation of the batch data is appropriate.The code changes are approved.
Line range hint
311-329: LGTM!The
_log_statsmethod is well-structured and ensures that the loss, metrics, and optimizer statistics are correctly logged.The code changes are approved.
|
@ibro45 Could you add some tests for the config validation? I think tests similar to what MONAI does would be very useful moving ahead. P.S: Cursor writes amazing tests if you reference a source tests file and select a target file to be tested in the code embeds |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lighter/utils/schema.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- lighter/utils/schema.py
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
tests/unit/test_schema.py (1)
8-22: LGTM! Consider adding more assertions to validate the structure and contents of the parsed configuration.The test function is well-structured and follows the Arrange-Act-Assert pattern. The use of the
@pytest.mark.parametrizedecorator is effective for testing multiple configuration files. The function has a clear and concise docstring that explains the purpose of the test.To further improve the test, consider adding more assertions to validate the structure and contents of the parsed configuration. For example, you could assert the presence of specific keys or the types of values associated with those keys.
# Example assertions assert "model" in parsed_config.config assert isinstance(parsed_config.config["model"], dict) assert "optimizer" in parsed_config.config assert isinstance(parsed_config.config["optimizer"], dict)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- tests/configs/test1.yaml (1 hunks)
- tests/unit/test_schema.py (1 hunks)
Additional context used
yamllint
tests/configs/test1.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (2)
tests/configs/test1.yaml (2)
1-1: Verify the usage of the_requires_section.The
_requires_section is used to import theosmodule, which is a non-standard YAML feature. This could be a project-specific convention.Run the following script to verify the usage of the
_requires_section:
2-16: Verify the usage of the defined variables and arguments.The file defines variables in the
varssection and arguments for different commands in theargssection.Run the following script to verify the usage of the defined variables and arguments:
Verification successful
Variables and Arguments Not Used in Python Codebase
The variables
some_varand argumentssome_argdefined in the YAML file are not found in the Python codebase. This suggests they are currently unused in the Python files. Please verify if they are intended to be used elsewhere or if they need to be integrated into the code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the defined variables and arguments in the codebase. # Test: Search for the usage of the defined variables. Expect: Occurrences in the codebase. rg --type python 'some_var' # Test: Search for the usage of the defined arguments. Expect: Occurrences in the codebase. rg --type python 'some_arg'Length of output: 117
Script:
#!/bin/bash # Description: Verify the usage of the defined variables and arguments in the codebase. # Test: Search for the usage of the defined variables. Expect: Occurrences in the codebase. rg --type py 'some_var' # Test: Search for the usage of the defined arguments. Expect: Occurrences in the codebase. rg --type py 'some_arg'Length of output: 47
Tools
yamllint
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
| lr_find: | ||
| some_arg: "value" | ||
| scale_batch_size: | ||
| some_arg: "value" |
There was a problem hiding this comment.
Add a newline at the end of the file.
The yamllint tool reported an error indicating a missing newline at the end of the file.
Add a newline at the end of the file to fix the yamllint error.
Tools
yamllint
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/unit/test_schema.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_schema.py
Description
Switched to Pydantic for structure validation. Not ecstatic about this solution (especially Pydantic error messages), but it is better than the current solution. I hope to figure out something better in the future. Resulted in a nice cleanup of runner.py and system.py.
Implemented PatchedModuleDict. Cleans up system from the ugly
"_" + metricworkaround.Added support for multi-command runs, e.g.
lighter fit test --config=.... Convenient when you want things to be logged under the same run, not sure if there are additional benefits.Backwards compatible, any previous config should work with this.
Reorganized the order of LighterSystem's methods
Related Issue
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.make codestyle.Summary by CodeRabbit
New Features
PatchedModuleDictclass to enhance module management and prevent key conflicts.Bug Fixes
Chores
numpyandpydanticfor better compatibility.Refactor
Tests