Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

subdag modifier + tag_outputs + refactor #237

Merged
merged 18 commits into from
Dec 24, 2022
Merged

Conversation

elijahbenizzy
Copy link
Collaborator

[Short description explaining the high-level reason for the pull request]

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@elijahbenizzy elijahbenizzy changed the title Subdag modifier rebase Subdag modifier rebase for RC Nov 27, 2022
@elijahbenizzy
Copy link
Collaborator Author

This PR has turned into a massive one due to a rebase. It's replacing:

  1. Subdag modifier #199
  2. Refactors decorators to live in a separate subpackage #198

And will likely replace:
3. #227

hamilton/function_modifiers_base.py Outdated Show resolved Hide resolved
load_from=[unique_users, interactions_filtered],
)
def quarterly_user_data_US() -> reuse.MultiOutput({"unique_users_daily_US": pd.Series}):
pass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add documentation to explain what this variant is.

load_from=[unique_users, interactions_filtered],
)
def daily_user_data_CA() -> reuse.MultiOutput({"unique_users_daily_CA": pd.Series}):
pass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add docs to explain what this variant is.

decorators.md Outdated Show resolved Hide resolved
@skrawcz
Copy link
Collaborator

skrawcz commented Nov 27, 2022

Some thoughts we should sync up on:

  • What's the long term story here from a code management perspective?
  • How will manage new decorators?
  • What should people be importing?

@elijahbenizzy
Copy link
Collaborator Author

TODO prior to general release:

@elijahbenizzy elijahbenizzy mentioned this pull request Dec 15, 2022
7 tasks
@skrawcz skrawcz linked an issue Dec 20, 2022 that may be closed by this pull request
elijahbenizzy and others added 13 commits December 22, 2022 08:06
This makes it backwards compatible -- we previously had two files:

1. function_modifiers.py
2. function_modfiers_base.py

We now create a module `function_modifiers.py` which enables us to
separate out decorators, as the file is getting unwieldy and ugly.

That said, we need to maintain backwards compatibility on import.
To do this, we:

1. Create a function_modifiers subpackage
2. Put everything that was in function_modifiers.py in __init__.py in
   that package (so imports work)
3. Move function_modifiers_base.py over there
4. Create a "dummy" function_modifiers_base.py that just imports
   everything from the actual one.

Will need to test this out on sample workflows prior to publishing.
1. configuration -- if/else equivalents
2. dependencies -- utilities for specifying dependencies, generally used
   for parameterized, but this will likely be expanded. Note decorators.
3. expanders -- decorators that turn one node into many
4. function_modifiers_base -- the base classes/common utilities
5. macros -- decorators that replace a function's implementation with
   something else (think @does/@dynamic_transform)
6. metadata -- decorators that add metadata to a node (tag right now)
7. validation -- decorators that do some validation on a node

Note that this was all done to preserve pure backwards compatibility.
Care was taken that every public component of the API was placed within
the __init__.py of function_modifiers.
Previously this was in the node's name. This makes it part of
the node, allowing for:

1. No string parsing in querying the namespace
2. Other constructs to use namespace effectively
3. Potentially layered namespaces

Note we still concatenate strings to reference upstream nodes -- TBD on
exactly how we should do this, but i think the convention of '.' joining
names is fairly reasonable.
@elijahbenizzy elijahbenizzy force-pushed the subdag-modifier-rebase branch 4 times, most recently from d8b38c6 to 2035e92 Compare December 23, 2022 18:40
Solves #254

`tag_outputs` enables you to attach metadata to a function that outputs
multiple nodes, and give different tag values to different items.

This can be applied multiple times, and precedence order of the `tag`
family of nodes is in reverse application order (see docs). Note that
if we have `tag` before `tag_outputs` this is undefined behavior.
@elijahbenizzy elijahbenizzy force-pushed the subdag-modifier-rebase branch 2 times, most recently from f09f1c5 to f93785b Compare December 23, 2022 18:55
@elijahbenizzy elijahbenizzy marked this pull request as ready for review December 24, 2022 21:28
Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@elijahbenizzy elijahbenizzy force-pushed the subdag-modifier-rebase branch 2 times, most recently from f13dff4 to a78ff0c Compare December 24, 2022 22:01
@elijahbenizzy elijahbenizzy changed the title Subdag modifier rebase for RC subdag modifier + tag_outputs + refactof Dec 24, 2022
@elijahbenizzy elijahbenizzy changed the title subdag modifier + tag_outputs + refactof subdag modifier + tag_outputs + refactor Dec 24, 2022
@elijahbenizzy elijahbenizzy merged commit 8fac22e into main Dec 24, 2022
@elijahbenizzy elijahbenizzy deleted the subdag-modifier-rebase branch December 24, 2022 23:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Break out function_modifiers into modules
2 participants