Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

warn or raise ValueError on duplicated key in json/yaml config #6252

Merged
merged 12 commits into from
Mar 29, 2023

Conversation

kretes
Copy link
Contributor

@kretes kretes commented Mar 28, 2023

Fixes #5471 .

Description

[WIP]
Monai will now raise an error whenever there is a duplicated key in any of the config json files.

TBD:

  • support similiar thing when reading from yaml

Things to discuss:

  • do we want to raise an Error or do we just want a warning message?
  • This will raise with the first duplicate encountered, not with a full list, but I think that's a fair & simple approach.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage. (I couldn't run them locally due to OOM)
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.

Documentation updated, tested make html command in the docs/ folder. I think that's a safeguard that doesn't require explicit documentation

@wyli
Copy link
Contributor

wyli commented Mar 29, 2023

Thanks, I think we can make it a warning be default, and provide an option to raise an exception. Cc @Nic-Ma

@kretes
Copy link
Contributor Author

kretes commented Mar 29, 2023

I've made it a warning by default and error only when a specific env variable is set. LMK if this is ok and then I can extend it to yaml.

@wyli
Copy link
Contributor

wyli commented Mar 29, 2023

I've made it a warning by default and error only when a specific env variable is set. LMK if this is ok and then I can extend it to yaml.

thanks, looks good to me

@wyli wyli requested review from Nic-Ma, ericspod and KumoLiu March 29, 2023 09:00
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, put some minor comments inline.

Thanks.

monai/bundle/config_parser.py Outdated Show resolved Hide resolved
monai/bundle/config_parser.py Outdated Show resolved Hide resolved
tests/test_config_parser.py Outdated Show resolved Hide resolved
kretes added 4 commits March 29, 2023 15:33
Signed-off-by: Tomasz Bartczak <[email protected]>
Signed-off-by: Tomasz Bartczak <[email protected]>
@kretes kretes changed the title will raise ValueError on duplicated key in json config warn or raise ValueError on duplicated key in json/yaml config Mar 29, 2023
@kretes kretes marked this pull request as ready for review March 29, 2023 16:08
@wyli
Copy link
Contributor

wyli commented Mar 29, 2023

/build

wyli added 2 commits March 29, 2023 22:08
Signed-off-by: Wenqi Li <[email protected]>
Signed-off-by: Wenqi Li <[email protected]>
@wyli
Copy link
Contributor

wyli commented Mar 29, 2023

/build

@wyli wyli enabled auto-merge (squash) March 29, 2023 21:44
@wyli wyli merged commit 45a398a into Project-MONAI:dev Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config component shouldn't allow duplicated arguments
3 participants