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

Adding syntax for the json config files to actually merge, not override, entries #8044

Open
borisfom opened this issue Aug 26, 2024 · 7 comments

Comments

@borisfom
Copy link
Contributor

borisfom commented Aug 26, 2024

I would like to see syntax for the json config files to actually merge, not override, entries.
Like to be able to just add a single import to the import list rather than overriding the whole list, or add a handler to the list of handlers via additional config .json.
Such an option would help a lot.

Describe the solution you'd like
I propose the following syntax:
Instead of having to override the whole list, e.g:
inference.json:

    "imports": [
        "$import numpy",                                                                            
    ],

inference_trt.json:

    "imports": [
        "$import numpy",
        "$from monai.networks import trt_compile"                                                                                                   
    ],

let's allow :
inference_trt.json:

   "+imports": [
       "$from monai.networks import trt_compile"                                                                                                   
   ],

Processing would be : let's add an extra merge pass after the 'override' step, where config entries are overridden by .update().
With '+' key prefix, both the original 'imports' and '+imports' entries will be in the dict.
Additional pass would find all the '+*' entries and perform merge of '+id" key entry into corresponding 'id' key.
Dictionaries will be update()'d, lists append()'ed, error otherwise.

borisfom added a commit to borisfom/MONAI that referenced this issue Aug 26, 2024
Signed-off-by: Boris Fomitchev <bfomitchev@nvidia.com>
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Aug 27, 2024

Sounds interesting, maybe adding a mode["override", "merge"] arg to the load_config_files() function?
https://github.com/Project-MONAI/MONAI/blob/dev/monai/bundle/config_parser.py#L406
CC @ericspod , @KumoLiu

Thanks.

@borisfom
Copy link
Contributor Author

I thing it's best the way I implemented it - we do want to be able to both override some entries and merge others, in the same config file. Most of new inference_trt.json do exactly that.

@borisfom
Copy link
Contributor Author

Implemented in #7990

@borisfom borisfom mentioned this issue Aug 27, 2024
7 tasks
@ericspod
Copy link
Member

Sounds interesting, maybe adding a mode["override", "merge"] arg to the load_config_files() function? https://github.com/Project-MONAI/MONAI/blob/dev/monai/bundle/config_parser.py#L406 CC @ericspod , @KumoLiu

Thanks.

We would want to selectively choose which keys to merge so the proposed strategy here may work for that. I don't see many places where this would be needed, honestly only the imports sections as far as I can see. If it was only that we could just do merge expressly for imports only but that would be inconsistent behaviour I feel. Also this syntax seems compatible with YAML but should be double checked.

@borisfom
Copy link
Contributor Author

borisfom commented Aug 27, 2024

@ericspod : yes it's compatible with YAML, I have added unit test to #7990 for both JSOM and YAML.
As for usage - second common use case I had in mind is adding handlers sections - this is another way to introduce trt_compile (instead of overriding network_def), and possibly for other extra processing.

@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 28, 2024

One more use case could be something like:

    "+train#random_transforms": [
            {
                "_target_": "RandSpatialCropd",
                "keys": [
                    "image",
                    "label"
                ],
                "roi_size": [
                    224,
                    224,
                    144
                ]
            }
        ]

It will extend the random_transforms list. cc @ericspod

@ericspod
Copy link
Member

One more use case could be something like:

Anything like this adding transforms could be useful, yes. We should test that the syntax adds to the random_transforms member correctly is all.

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

No branches or pull requests

6 participants
@borisfom @ericspod @Nic-Ma @KumoLiu and others