Project-MONAI / MONAI

AI Toolkit for Healthcare Imaging
https://monai.io/
Apache License 2.0
5.7k stars 1.04k forks source link

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

Open borisfom opened 3 weeks ago

borisfom commented 3 weeks ago

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.

Nic-Ma commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Implemented in https://github.com/Project-MONAI/MONAI/pull/7990

ericspod commented 3 weeks ago

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 commented 3 weeks ago

@ericspod : yes it's compatible with YAML, I have added unit test to https://github.com/Project-MONAI/MONAI/pull/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 commented 3 weeks ago

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 commented 3 weeks ago

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.