Materialization improvements #264
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elijahbenizzy
force-pushed
the
materialize-fixes
branch
from
ce5744b to
d534e72
Compare
It now: 1. Works without additional_vars included 2. Returns the graphviz object for rendering in a notebook
This was added when we added graph copying. This adds the ability for update_dependencies to wipe the dependencies before executing. Note that it does an in-place modification if this doesn't happen -- that's the reset_dependencies option. Note this is an internal API so I don't mind it being geared towards performance.
This was referring to data loaders instead of data savers in the registry.
elijahbenizzy
force-pushed
the
materialize-fixes
branch
from
8c0a81a to
7639297
Compare
elijahbenizzy
force-pushed
the
materialize-fixes
branch
from
7639297 to
a2fc7fa
Compare
We do the ML model example, and add some custom ones. Hopefully this gets people started. We have an easy script to run + a notebook.
elijahbenizzy
force-pushed
the
materialize-fixes
branch
from
a2fc7fa to
ae6636e
Compare
MUltiprocessing doesn't work in many cases due to the default pickling mechanism being garbage.
Previously we would inject a node with a parameter name into a
parameter consumed by downstream set of nodes. This would cause
name-clashes if, say, the parmeter name was `data`:
@load_from.json(...)
def foo(data: pd.DataFrame) -> ...:
...
To fix this, we did two things:
1. Change the data loader nodes that were created to have namespaces so
they're unique
2. Allow the NodeInjector to rename input nodes so it can communicate
the new names.
Note this is probably slightly more abstraction than needed but I have a
sense that NodeInjector will be necessary moving forward (external API
calls, etc...).
skrawcz
approved these changes
Aug 13, 2023
| 2. Alters the output to include the materializer nodes | ||
| 3. Processes a list of "additional variables" (for debugging) to return intermediary data | ||
| 4. Executes the DAG, including the materializers | ||
| 5. Returns a tuple of (`materialization metadata`, `additional variables`) |
There was a problem hiding this comment.
why "additional variables"? Because that's what we use in execute? Should we rename it to "additional_outputs"?
There was a problem hiding this comment.
Yep to mirror "final_vars", but I like "outputs" better than vars
There was a problem hiding this comment.
I would just make it outputs personally. But 🤷 .
There was a problem hiding this comment.
On second thought we already released this and I'm a stickler for semantic versioning...
Comment on lines
+57
to
+74
| to.json( | ||
| dependencies=["model_parameters"], id="model_params_to_json", path="./data/params.json" | ||
| ), | ||
| # classification report to .txt file | ||
| to.file( | ||
| dependencies=["classification_report"], | ||
| id="classification_report_to_txt", | ||
| path="./data/classification_report.txt", | ||
| ), | ||
| # materialize the model to a pickle file | ||
| to.pickle(dependencies=["fit_clf"], id="clf_to_pickle", path="./data/clf.pkl"), | ||
| # materialize the predictions we made to a csv file | ||
| to.csv( | ||
| dependencies=["predicted_output_with_labels"], | ||
| id="predicted_output_with_labels_to_csv", | ||
| path="./data/predicted_output_with_labels.csv", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
do we want path to be a source, and have it provided as inputs? Or at least one of these should show that pattern so that people can see what can still be dynamic.
There was a problem hiding this comment.
Yeah so i can add that as an example or at least note it
elijahbenizzy
force-pushed
the
materialize-fixes
branch
3 times, most recently
from
572cb96 to
317dbe6
Compare
This has: 1. A reference table, automatically generated using a custom sphinx directive 2. References for the base classes for extension Re (1) we generate a bare-bones table but it should be enough. For now, we just link to the code, but we will, at some point, link to actual class docs.
elijahbenizzy
force-pushed
the
materialize-fixes
branch
from
317dbe6 to
be724aa
Compare
This was referenced Aug 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Short description explaining the high-level reason for the pull request]
Changes
How I tested this
Notes
Checklist