https://github.com/stitchfix/hamilton logo
Join Slack
Powered by
I've been looking at `parameterize_extract_columns...
# contribute
i
I've been looking at 
parameterize_extract_columns
and saw that it requires 
ParameterizedExtract
objects. Suggestion: have it accept some sort of named/ordered fields (e.g. list of dicts, or list of tuples) then wrap them internally in 
ParameterizedExtract
. It saves an import and is a tiny bit more elegant imho. wdyt?
e
Yeah, so I’m trying to dig up the decision records (there’s an issue somewhere), but we did it so it would be a little more self-documenting — I think we found the whole 
Tuple[str, …]
, 
Dict[str, ParametrizedDependency]
to be cumbersome — lots of tuples. That said, our solution si, as you pointed out, quite cumbersome as well 😆 And it adds extra surface area. So, we’re going to want to keep it backwards compatible, but taking a step back — what do you think the ideal interface would be? Lists of tuples might be nice, but maybe theres a room for a cleaner way of presenting it…
i
I believe I saw hamilton decorators that do work like what you mentioned (i.e. no need to import a model), can't remember which though... Perhaps pair each decorator with an associated model that can always be accessed via a 
.model
property (I don't know if that's even possible), e.g. pseudocode
Copy code
from hamilton.decorators import parameterize, subdag

...
@parameterize(
    # the top level will always contain a {*}collection of the associated model type
    parameterize.model(
        # these will be optional (or mandatory) kwargs, and hence hinted
        output_name=...,
        replace_dict=...,
        docstring=...,
    ),
    ...
)
def foo(...): ...

@subdag(
    subdag.model(
        subdag_param_1=...,
        ...
    )
)
def bar(...): ...
BTW, regarding the example of 
parameterize_extract_columns
- shouldn't there be a step that renames the df columns so that extraction can pick them up? According to the 
extract_columns
docs, when the output column doesn't exist it either errors out or fills it with a default value. There's a different behavior here that's "hidden" by the 
reassign_columns
parameter which is "active" by default. I think the docs should be amended to explain this behavior explicitly (what happens when it's on and off).
Copy code
@parameterize_extract_columns(
    ParameterizedExtract(
        ("outseries1a", "outseries2a"),
        {"input1": source("inseries1a"), "input2": source("inseries1b"), "input3": value(10)},
    ),
    ParameterizedExtract(
        ("outseries1b", "outseries2b"),
        {"input1": source("inseries2a"), "input2": source("inseries2b"), "input3": value(100)},
    ),
)
def fn(input1: pd.Series, input2: pd.Series, input3: float) -> pd.DataFrame:    
    return pd.concat([input1 * input2 * input3, input1 + input2 + input3], axis=1)
    # How we magically get a series named e.g. `outseries1a` out of the above is unclear from the docstring.
e
Interesting — so yeah, most of them don’t have one as the parameters are more self-documenting. 
@parameterize_extract_columns
is a bit messy though — there’s a lot to spec out. Having a 
.model
is a clever idea, but I guess why have that if we can just have the parameters?
I think this one is just particularly weird — it’s hard to specify. And yeah, I think it’s going based on position but I’m not sure, will look (and we should add a test for this if it’s supporetd)
SO, circling back, I think it’s not crazy to just add a list of tuples as an alternate approach — it could be a list of 
Union[ParameterizedExtract, Tuple]
, and we can create the 
ParameterizeExtract
under the hood. Should be an easy change, happy with a PR if you want!
Open in Slack
PreviousNext
Powered by