plugin MLFlow
#945
plugin MLFlow
#945
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 70e620b in 1 minute and 16 seconds
More details
- Looked at
734lines of code in7files - Skipped
1files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_q2KEKG2CuxHXzKwa
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
examples/mlflow/README.md
Outdated
|
|
||
| This pairs nicely with the `HamiltonTracker` and the [Hamilton UI](https://hamilton.dagworks.io/en/latest/hamilton-ui/ui/) which gives you execution observability. | ||
|
|
||
| We're looking forward to better link Hamilton "projects" with MLFlow "experiments" and runs from both projects. |
There was a problem hiding this comment.
| We're looking forward to better link Hamilton "projects" with MLFlow "experiments" and runs from both projects. | |
| We're looking forward to the better linking of Hamilton "projects" with MLFlow "experiments" and runs from both projects. |
There was a problem hiding this comment.
A few comments, overall looks good. Only thing I'm iffy about is a lot of non-transparent coupling between the materializer types and MLFlow. Can we add a docs section about this?
Also, can we add this to the docs? We have docstrings + everything. This should at least go in reference
examples/mlflow/tutorial.ipynb
Outdated
There was a problem hiding this comment.
Error -- rerun? MlflowException: Invalid experiment name: Ellipsis. Expects a string.
There was a problem hiding this comment.
Yeah, I added code snippets at the end for demonstration purposes. I turned them into markdown snippets
| run_description: Optional[str] = None, | ||
| log_system_metrics: bool = False, | ||
| ): | ||
| """Configure the MLFlow client and experiment for the lifetime of the tracker |
There was a problem hiding this comment.
Show usage in docstring? This should go in docs.
| # special case for matplotlib and plotly | ||
| # log materialized figure. Allows great degree of control over rendering format | ||
| # and also save interactive plotly visualization as HTML | ||
| elif node_tags["hamilton.data_saver.sink"] in ["plt", "plotly"]: |
There was a problem hiding this comment.
Add todo -- we'll want to add more things (datasets, etc...) that can come from this tag. Wil want to be a dict rather than a if/elif statement
| model_name: Optional[str] = None | ||
| version: Optional[Union[str, int]] = None | ||
| version_alias: Optional[str] = None | ||
| flavor: Optional[str] = None |
There was a problem hiding this comment.
"flavor" isn't a great name -- is there more typing (E.G. a Literal[])?
There was a problem hiding this comment.
If its just mlflow's name then that makes sense
There was a problem hiding this comment.
This is the MLFlow terminology, didn't reinvent the wheel here
There was a problem hiding this comment.
Each flavor is a library / backend and each have multiple types. We shouldn't explicitly handle them and let that be delegated to MLFlow
|
Skipped PR review on f73d81e because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away. Generated with ❤️ by ellipsis.dev |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The MLFlow plugin for Hamilton includes two sets of features:
MLFlowModelSaverandMLFlowModelLoadermaterializersMLFlowTracker.Changes
hamilton/plugins/mlflow_extensions.pycontains the materializershamilton/plugins/h_mlflow.pycontains the trackerexamples/mlflow/tutorial.ipynbis a tutorial notebookHow I tested this
tests/plugins/test_mlflow_extensions.pytests the materializersTODO / ideas
display_all_functions()andvisualize_execution()from theHamiltonTrackerto store in the MLFlow tracking server