warn or raise ValueError on duplicated key in json/yaml config by kretes · Pull Request #6252 · Project-MONAI/MONAI · GitHub
Skip to content

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

Merged
wyli merged 12 commits into
Project-MONAI:devfrom
kretes:config-duplicate-keys
Mar 29, 2023
Merged

warn or raise ValueError on duplicated key in json/yaml config#6252
wyli merged 12 commits into
Project-MONAI:devfrom
kretes:config-duplicate-keys

Conversation

@kretes

@kretes kretes commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

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

kretes and others added 4 commits March 28, 2023 13:28
Signed-off-by: Tomasz Bartczak <kretesenator@gmail.com>
Signed-off-by: Tomasz Bartczak <kretesenator@gmail.com>
Signed-off-by: Tomasz Bartczak <kretesenator@gmail.com>
@wyli

wyli commented Mar 29, 2023

Copy link
Copy Markdown
Contributor

Signed-off-by: Tomasz Bartczak <kretesenator@gmail.com>
@kretes

kretes commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

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

wyli commented Mar 29, 2023

Copy link
Copy Markdown
Contributor

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 KumoLiu, Nic-Ma and ericspod March 29, 2023 09:00

@Nic-Ma Nic-Ma left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread monai/bundle/config_parser.py Outdated
Comment thread monai/bundle/config_parser.py Outdated
Comment thread tests/test_config_parser.py Outdated
kretes added 4 commits March 29, 2023 15:33
Signed-off-by: Tomasz Bartczak <kretesenator@gmail.com>
Signed-off-by: Tomasz Bartczak <kretesenator@gmail.com>
Signed-off-by: Tomasz Bartczak <kretesenator@gmail.com>
Signed-off-by: Tomasz Bartczak <kretesenator@gmail.com>
@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

wyli commented Mar 29, 2023

Copy link
Copy Markdown
Contributor

/build

wyli added 2 commits March 29, 2023 22:08
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli

wyli commented Mar 29, 2023

Copy link
Copy Markdown
Contributor

@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