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

Update package name #7

Open
PetrochukM opened this issue Sep 7, 2020 · 3 comments
Open

Update package name #7

PetrochukM opened this issue Sep 7, 2020 · 3 comments

Comments

@PetrochukM
Copy link
Owner

PetrochukM commented Sep 7, 2020

Add this quote to the documentation:

Defaults are arguably the most important design decisions you'll ever make as a software developer.

https://blog.codinghorror.com/the-power-of-defaults/

Rename this package to Defaults, it makes more sense than HParams.

Example API:

import defaults

@defaults.register
def function(parameter: int = defaults.Default()):
    pass

class Object():
    @defaults.register
    def __init__(self, parameter: str = defaults.Default()):
        pass

defaults.set_defaults({
    function: defaults.Defaults(parameter=0),
    Object.__init__: defaults.Defaults(parameter=''),
})

This module is also redundant, the primary interface is via a dotted string path. The dotted string path is then parsed, imported, and hashed. Instead of resolving a string path, we could just accept a function object. The function object is guaranteed to exist, and we can check if it has a configurable wrapper.

We should still use _get_function_signature so that the configuration is exportable and importable into another process. We can remove _lazy_resolution because the import responsibility is on the owner.

With regard to external libraries, like _BatchNorm and BatchNorm1d, the issue will continue to persist. At the end of the day, there are two references to the same function. If one of the references is decorated and the other is not, that will cause all kinds of issues. For example, it'll cause issues with the trace. It'll also cause issues with __qualname__. The proper solution is to create a subclass with BatchNorm1d that has a separate __init__ with a separate set of kwargs.

The idea of decorating external libraries is interesting. At the end of the day, the goal of this library is to help configure a bunch of code. Instead of having an external configuration object that is passed through the entire code base, we just directly inject the parameters that we'd like to use.

@PetrochukM
Copy link
Owner Author

We could also update the name to configs. The idea of "defaults" is based on overwriting arguments, which isn't entirely how this library is intended to be used.

@PetrochukM
Copy link
Owner Author

PetrochukM commented Sep 21, 2020

We could also... instead of having a cryptic function like add_config that adds another level of abstraction. We could do something like:

import defaults
function.set_defaults(parameter=0)
Object.__init__.set_defaults(parameter='')

Or

import defaults
defaults.set_defaults(function, parameter=0)
defaults.set_defaults(Object.__init__, parameter='')

Or

import defaults
defaults.set_defaults({
    function: {
        parameter: 0
    },
    Object.__init__: {
        parameter: ''
    },
})

Or

import defaults
defaults.set_defaults(
   (function, dict(parameter=0)),
   (Object.__init__, dict(parameter='')),
)

We could also... maybe... use a generic type to add type checking for set_defaults.

@PetrochukM
Copy link
Owner Author

Or it could be named options, similar to this: https://docs.streamlit.io/en/stable/api.html#placeholders-help-and-options

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