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

Enforce kwargs only for most methods and classes #8327

Open
johnzielke opened this issue Feb 4, 2025 · 0 comments
Open

Enforce kwargs only for most methods and classes #8327

johnzielke opened this issue Feb 4, 2025 · 0 comments

Comments

@johnzielke
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Often, when refactoring models and code, parameters become obsolete, or new ones are added. As MONAI tries to maintain backward compatibility, any arguments that can be used as positional arguments make changes like these often very difficult.
IMO most networks, transforms etc will and should be called with keyword arguments to help readability. And in the case of monai configs, they are called with kwargs anyway.
E.g.

We want to remove save_mem from the following class:

class GroupNorm3D(nn.GroupNorm):
   
    def __init__(
        self,
        num_groups: int,
        num_channels: int,
        eps: float = 1e-5,
        affine: bool = True,
        save_mem: bool = True,
        norm_float16: bool = False,
        print_info: bool = False,

    ):

Since there is a public interface, users could call this using GroupNorm3D(10,4,args.eps,args.affine,args.args.save_mem)
When we now remove the argument save_mem, we have no way of determining whether the user actually wanted to call the class with the old save_mem parameter or the norm_float16 parameter at the now 5th position.
If we use kw-only args, this would not be a problem and the deprecate_arg decorator could be applied without issue.
Example:

class GroupNorm3D(nn.GroupNorm):
   
    def __init__(
        self,
        num_groups: int,
        num_channels: int,
        *,
        eps: float = 1e-5,
        affine: bool = True,
        save_mem: bool = True,
        norm_float16: bool = False,
        print_info: bool = False,

    ):

or even not allowing any positional arguments, which I think would be best for most classes, but not for utility functions.

class GroupNorm3D(nn.GroupNorm):
   
    def __init__(
        self,
        *,
        num_groups: int,
        num_channels: int,
        eps: float = 1e-5,
        affine: bool = True,
        save_mem: bool = True,
        norm_float16: bool = False,
        print_info: bool = False,

    ):

I think MONAI should consider making all new (and eventually existing) networks, transforms, etc, that do not have an obvious need for positional arguments to only use kwargs-only. This would force readable calls and an easier-to-maintain code base.
This should also apply to any special arguments for utility functions, etc, that will not always be used or whose position is not important on the call.

This could be either enforced during review or with flake8/ruff rules.

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

1 participant