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

5852 fixes partial callable config parser #5854

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

wyli
Copy link
Contributor

@wyli wyli commented Jan 14, 2023

Signed-off-by: Wenqi Li [email protected]

Fixes #5852

Description

  1. the following partial instantiate doesn't work
config = {"import statements": "$import math", 
          "calc": {"_target_": "math.isclose", "a": 0.001, "b": 0.001}}
print(ConfigParser(config).calc())

with an error message:

Component to instantiate must represent a valid class or function, but got math.isclose.
Traceback (most recent call last):
  File "test.py", line 4, in <module>
    print(ConfigParser(config).calc())
TypeError: isclose() missing required argument 'a' (pos 1)

because math.isclose is a builtin type but not a function:

import inspect
inspect.isfunction(math.isclose)  # False
inspect.isbuiltin(math.isclose)  # True

the partial should support callable including builtin functions

  1. also this PR supports _target_ of reference object's methods such as partial init:
"forward": {"_target_": "$@model().forward", "x": "$torch.rand(1, 3, 256, 256)"}

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • 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.

@wyli
Copy link
Contributor Author

wyli commented Jan 14, 2023

/build

@wyli wyli changed the title 5852 fixes partial callable 5852 fixes partial callable config parser Jan 14, 2023
monai/bundle/config_item.py Outdated Show resolved Hide resolved
wyli added 4 commits January 17, 2023 00:23
Signed-off-by: Wenqi Li <[email protected]>
- update test cases

Signed-off-by: Wenqi Li <[email protected]>
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli force-pushed the 5852-callable-partial branch from 88babdd to ac26db9 Compare January 17, 2023 00:25
@wyli
Copy link
Contributor Author

wyli commented Jan 17, 2023

/build

@wyli wyli enabled auto-merge (squash) January 17, 2023 00:42
@ibro45
Copy link
Contributor

ibro45 commented Jan 17, 2023

https://github.com/wyli/MONAI/blob/ac26db9ba5672c45cb904c5ad426383846f95a58/tests/test_config_parser.py#L253

This doesn't cover the following case where the model is an instantiated object and not a partial function. Picked EfficientNetBN for no particular reason. I've changed $@model().forward to @model.forward as I believe that that would be the expected format.

configs = {
            "fwd": {"_target_": "@model.forward", "x": "$torch.rand(1, 3, 256, 256)"},
            "model": {"_target_": "monai.networks.nets.EfficientNetBN",  "model_name": "efficientnet-b0", "pretrained": False, "spatial_dims": 2},
        }

@wyli wyli merged commit 734d4ff into Project-MONAI:dev Jan 17, 2023
@wyli wyli deleted the 5852-callable-partial branch January 17, 2023 07:19
@wyli
Copy link
Contributor Author

wyli commented Jan 17, 2023

thanks @ibro45, in that case we still need the $ so that .forward is treated as a python dot-notation to access the instance method:

"fwd": {"_target_": "[email protected]", "inputs": "$torch.rand(1, 3, 256, 256)"},

also because an instance of EfficientNetBN is also a callable, "@model" works as well:

configs = {
    "fwd": {"_target_": "@model", "inputs": "$torch.rand(1, 3, 256, 256)"},
    "model": {
        "_target_": "monai.networks.nets.EfficientNetBN",
        "model_name": "efficientnet-b0",
        "pretrained": False,
        "spatial_dims": 2,
    },
}
print(ConfigParser(configs).fwd())

Please feel free to make a PR to add more test cases if you are interested :)

@ibro45
Copy link
Contributor

ibro45 commented Jan 17, 2023

@wyli when you parse the config it returns a partial function that you then manually call here print(ConfigParser(configs).fwd())

Reproduce

config.yaml:

fwd1:
    _target_: "[email protected]"
    inputs: "$torch.zeros(1, 3, 256, 256)"

fwd2: "[email protected](inputs=torch.zeros(1, 3, 256, 256))"

model:
    _target_: "monai.networks.nets.EfficientNetBN"
    model_name: "efficientnet-b0"
    pretrained: True
    spatial_dims: 2

Run fwd1:
python -m monai.bundle run fwd1 --config_file config.yaml
Returns a partial function that didn't run.

Run fwd2:
python -m monai.bundle run fwd2 --config_file config.yaml
Returns the output tensor of the model.

Do you agree that the fwd1 and fwd2 should return the same output, or is it just that I don't understand how its meant to work?

Reasoning

I want to call a method using _target_ and pass the arguments to it through CLI. Let's say:
python -m monai.bundle run fwd1 --config_file config.yaml --fwd1#inputs "$torch.rand(1,3,512,512)". That isn't possible using the format in fwd2 unless you rewrite it something like this:

fwd2:
    _method_: "[email protected](@fwd#inputs)
    inputs: "$torch.zeros(1, 3, 256, 256)"

This allows you to pass the arguments from CLI at the expense of making it very cluttered.

_partial_

I might just be used to Hydra's _partial_, but I think it makes sense. If you want a function, method, or even class init to be partial, we could have a _partial_ flag. Otherwise, it will just be instantiated/called.

Let me know if I should open an issue for this.

@surajpaib
Copy link
Contributor

This seems like a helpful feature to me as well! Having the ability to both call a function through instantiate and create a partial function.

I would've expected the default config to call the function because I mostly worked with hydra and am used to that being their default behavior.

@wyli
Copy link
Contributor Author

wyli commented Jan 17, 2023

I think _target_ is mainly to create the object instance, to get the concrete result, we can create an "infer" command:

configs = {
    "fwd": {"_target_": "@model", "inputs": "$torch.rand(1, 3, 256, 256)"},
    "model": {
        "_target_": "monai.networks.nets.EfficientNetBN",
        "model_name": "efficientnet-b0",
        "pretrained": False,
        "spatial_dims": 2,
    },
    "infer": "$@fwd()",
}
print(ConfigParser(configs).infer)

cc @Nic-Ma @ericspod

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.

Parse callables (functions, methods) similarly to objects
4 participants