hamilton icon indicating copy to clipboard operation
hamilton copied to clipboard

Add `config.otherwise`

Open elijahbenizzy opened this issue 3 years ago • 3 comments

Is your feature request related to a problem? Please describe. Currently config.*** has to be mutually exclusive. This is a pain -- we should have a config.otherwise or config.default to allow for default function definitions.

Describe the solution you'd like

@config.when(region='us')
def some_fn__us() -> ...:
    pass

@config.when(region='ca')
def some_fn__ca() -> ...:
    pass

@config.default
def some_fn() -> ...:
    pass    

Describe alternatives you've considered Think this is worthwhile, but we could also avoid this if we wanted.

Additional context Might be tricky to implement, as this needs some sort of global state. Currently each one resolves a node, but this one resolves only if the others have not yet resolved. TBD on how to implement, but we could pass/store a global state for decorators.

elijahbenizzy avatar Sep 12 '22 23:09 elijahbenizzy

OK, big challenge is managing global state. Currently resolve_nodes is a single function that takes in a function and gives out nodes. It does not have access to the states of any other functions. This would have to change. Options:

  1. Restrict to the same module/require that the default be last. This is a hack that allows us to, in the default, resolve all other potential candidate functions.
  2. Pass in global state of all other nodes when resolving -- this requires it to be last
  3. Pass in a pointer to the default implementation -- hard to see exactly how this would work/solve the problem.
  4. Do a two-pass fn graph creation: (1) create all the nodes and (2) filter based on the config

elijahbenizzy avatar Sep 13 '22 19:09 elijahbenizzy

I think there is a benefit to being explicit. Why not do:


@config.when_not_in(region=['us', 'ca'])
def some_fn__default() -> ...:
    pass

?

skrawcz avatar Sep 13 '22 19:09 skrawcz

I think there is a benefit to being explicit. Why not do:




@config.when_not_in(region=['us', 'ca'])

def some_fn__default() -> ...:

    pass

?

Honestly I kind of regret building when_not_in for a default. I'd rather just have it be more explicit. If you add a new region, specify exactly how it'll behave in all cases...

elijahbenizzy avatar Sep 13 '22 19:09 elijahbenizzy

Concensus is not to implement (for now) -- lots of complexity with little benefit (in clarity). Would love thoughts if you have them!

elijahbenizzy avatar Oct 28 '22 22:10 elijahbenizzy