Decorator improvements #316
Conversation
This allows for inspection/visibility, and will be essential in checkpointing/storage options. We want decorators to declare these upfront, rather than relying on the internals of the decorators to break, enabling clearer error messages for more usable configuration-driven pipelines. Furthermore, we also allow for some "optional" configuration parameters. These enable us to provide a default. The use-case here is config.when -- which currently defaults to None. Allowing this from the start is not ideal, but it's what we did and users rely on it. There *is* an out, however. If someone is using the base `@config`, they were able to declare a lambda function, which yields no insight into the parameters required by the function. To bypass this, we pass None out, and the framework knows not to filter the required parameters.
elijahbenizzy
force-pushed
the
target-decorator
branch
6 times, most recently
from
95bdfc9 to
4a72db0
Compare
The `target` parameter specifies *which* nodes will be transformed. This can take 4 values: - None -- this is the old default behavior, just decorating the 'output' (I.E. sync nodes) - Ellipsis (...) -- this decorates everything - string -- this decorates *just* the specified node - Collection[string] -- this decorates all nodes in the collection
elijahbenizzy
force-pushed
the
target-decorator
branch
from
4a72db0 to
045e7db
Compare
non-final. Our new target decorator nullifies this, and to be honest, this never really did what we thought from the beginning. The nodes taht were decorated with non-final were *actually* non-final, meaning that they were never sink nodes (E.G. always depended on).
elijahbenizzy
force-pushed
the
target-decorator
branch
from
045e7db to
dcfb43c
Compare
| @@ -126,6 +127,15 @@ def validate(self, fn: Callable): | |||
| ) | |||
|
|
|||
|
|
|||
| @deprecation.deprecated( | |||
There was a problem hiding this comment.
why deprecate? Seems like this is much better for mass application?
| 2. Decorators that *turn* a function into a set of nodes (E.G. `@does`) | ||
| 3. Decorators that modify a set of nodes, turning it into another set of nodes (E.G. `@parameterize`, `@extract_columns`, and `@check_output`) |
There was a problem hiding this comment.
It doesn't sound like there's a difference between (2) & (3)?
Otherwise going between function and nodes is confusing I think -- all the decorators operate on a function.
There was a problem hiding this comment.
There is a difference. The decorators in (3) don't operate on functions, they operate on nodes.
The decorators in (2) have access to the function and use it to determine the subdag shape, the decorators in (2) don't.
There was a problem hiding this comment.
Yes, but does that explanation make sense to an end user? I don't think we've articulated the distinction...
There was a problem hiding this comment.
Yeah, TBH I think we need to work on making it clearer, and its difficult for me to do when I'm in the nodes headspace. So, let's revisit that and see how users react? Added a note about functions/nodes, but let's see how users react when we're rewriting documentation.
elijahbenizzy
force-pushed
the
target-decorator
branch
from
dcfb43c to
484678f
Compare
| :return: A collection of nodes that are not in the set of nodes to transform but are in the | ||
| subdag | ||
| """ | ||
| return [node_ for node_ in all_nodes if node_ not in nodes_to_transform] |
There was a problem hiding this comment.
sure -- could also use a set operation here.
elijahbenizzy
force-pushed
the
target-decorator
branch
from
77712ec to
95edbd5
Compare
We get at the mental model for decorators/how layering works. Its complicated, so we don't go into too much detail (the code is there to browse), but should give users enough to be effective at using layering/`target_`
This is a log nicer. See discussion here for full context: #86.
elijahbenizzy
force-pushed
the
target-decorator
branch
from
95edbd5 to
f0be6ca
Compare
A few decorator changes. Some are user-facing and some are relevant till later.
Changes
targetparameter for NodeTransformer/NodeDecoratorHow I tested this
Automated tests.
Notes
Checklist