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

simplify the configparser usage to get parsed attributes easily #5804

Closed
wyli opened this issue Jan 4, 2023 · 7 comments
Closed

simplify the configparser usage to get parsed attributes easily #5804

wyli opened this issue Jan 4, 2023 · 7 comments

Comments

@wyli
Copy link
Contributor

wyli commented Jan 4, 2023

Is your feature request related to a problem? Please describe.

the current usage looks tedious (https://github.com/Project-MONAI/research-contributions/blob/07da7ad36a0b218556a427f3bc5292a772c30f60/auto3dseg/algorithm_templates/segresnet2d/scripts/infer.py#L34-L50)

parser = ConfigParser()
parser.read_config(config_file_)
parser.update(pairs=_args)

data_file_base_dir = parser.get_parsed_content("data_file_base_dir")
data_list_file_path = parser.get_parsed_content("data_list_file_path")
self.fast = parser.get_parsed_content("infer")["fast"]
self.num_adjacent_slices = parser.get_parsed_content("num_adjacent_slices")
self.num_sw_batch_size = parser.get_parsed_content("num_sw_batch_size")
self.output_classes = parser.get_parsed_content("output_classes")
self.overlap_ratio = parser.get_parsed_content("overlap_ratio")
self.patch_size_valid = parser.get_parsed_content("patch_size_valid")
softmax = parser.get_parsed_content("softmax")

ckpt_name = parser.get_parsed_content("infer")["ckpt_name"]
data_list_key = parser.get_parsed_content("infer")["data_list_key"]
output_path = parser.get_parsed_content("infer")["ouptut_path"]

this is a feature request of simplifying the APIs to support more user-friendly calls
to get the parsed config as attributes directly:

cfg = ConfigParser()
cfg.read_config(config_file_)
cfg.update(pairs=_args)

data_file_base_dir = cfg.data_file_base_dir
data_list_file_path = cfg.data_list_file_path
self.fast = cfg.infer.fast
...

cc @Project-MONAI/core-reviewers

@Shadow-Devil
Copy link
Contributor

Hi, I would like to work on this issue.
Is this the only place where this simplification should be done or
should I also look out for other places with similar parser.get_parsed_content() usage?

@Shadow-Devil
Copy link
Contributor

While searching through the files, I think I found a typo: https://github.com/Project-MONAI/research-contributions/blob/07da7ad36a0b218556a427f3bc5292a772c30f60/auto3dseg/algorithm_templates/dints/scripts/infer.py#L48
It should be output_path not ouptut_path, right?

@Shadow-Devil
Copy link
Contributor

I think I got the issue wrong,
it's not about replacing the code inside research-contributions,
but to simplify the API so that the code works as described.
But I still want to work on it.

@Shadow-Devil
Copy link
Contributor

Shadow-Devil commented Jan 4, 2023

My first intuition was to implement it the following way:

    def __getattr__(self, key):
        """
        Get the config by id.

        Args:
            key: id to specify the expected position. See also :py:meth:`__getitem__`.

        """
        return self[key]

Now the implementation just forwards the call to __getitem__() so we don't need to duplicate the code.
From the python documentation: Called when the default attribute access fails...

So the method won't be called if a defined method/property is tried to be accessed (e.g. config or get_parsed_content()).

One problem with this implementation is, that it only works one layer deep, so this doesn't work:

from monai.bundle.config_parser import ConfigParser
x = ConfigParser({'a':{'b':"c"}, 1:2})
x.a.b  # AttributeError: 'dict' object has no attribute 'b'

@wyli
Copy link
Contributor Author

wyli commented Jan 4, 2023

sounds great, I'm assigning this to you , it's ok if the first PR only works for the shallow structure...I haven't thought in detail about the nested case but we can always follow up and improve it.

(also thanks for reporting the typo!)

cc @Nic-Ma

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jan 5, 2023

Sounds good for the initial PR idea, I think we can think about the nested case together later.
But personally I feel the nested case is "must-have", otherwise, the user experience may be strange.

Thanks.

Shadow-Devil added a commit to Shadow-Devil/MONAI that referenced this issue Jan 5, 2023
wyli pushed a commit that referenced this issue Jan 6, 2023
…5813)

part of #5804.

### Description

This adds a simple attribute access for ConfigParser so

```python
from monai.bundle import ConfigParser
parser = ConfigParser({"a":"b"})
a = parser.get_parsed_content("a")
```
can be rewritten to
```python
from monai.bundle import ConfigParser
parser = ConfigParser({"a":"b"})
a = parser.a
```
This only works for variables/methods that are not included in
ConfigParser so e.g. `parser.config` would still get the whole config.
This PR only supports shallow attribute accesses, since the returned
value will be a `dict` where you need to use `["key"]` to get the
content.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [x] New tests added to cover the changes.
- [x] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.

Signed-off-by: Felix Schnabel <[email protected]>
@Shadow-Devil Shadow-Devil removed their assignment Jan 21, 2023
@wyli wyli closed this as completed Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants