-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MMM Evaluation, Diagnostics and MLFlow Registry #1368
Conversation
…e compliant with standards
…nclude in calc_metric
…conform with API guidelines and enable model registering
…ing an MMMEvaluator class and moving functions there as methods. Breaking out pm.compute_log_likelihood into its own method. Moving over diagnostic functions from MLFlow into this module
…V), adding remaining methods
…Adding log_model to autolog, along with log_loocv()
…metrics as distributions first, before then taking summary stats over the distributions of the metrics
…ngle-purpose, just logging of LOOCV metrics and no loglikelihood computation
… to metrics, removing MMMEvaluator and replacing with functions then updating the MLFlow module appropriately
WAIC is another good metric to include if the use case involves out-of-sample predictions: |
Can we address this in another pr. This is already months deep |
def load_mmm( | ||
run_id: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather this take model_uri
which is more similar to the sklearn mlflow function: https://mlflow.org/docs/latest/python_api/mlflow.sklearn.html#mlflow.sklearn.load_model
This would allow loading from run or from registered model.
If we'd like, we can add helpers to construct the run URI and the registered model URI
23be92f
to
9e92f6f
Compare
…ed in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments :) I will let @wd60622 review the mlflow components in detail as I have not been using it lately :)
@@ -1065,6 +1065,148 @@ def plot_channel_parameter(self, param_name: str, **plt_kwargs: Any) -> plt.Figu | |||
) | |||
return fig | |||
|
|||
def plot_prior_vs_posterior( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to take it out, but there are some things I prefer in plot_prior_vs_posterior
:
- Option to sort by difference / alphabetically (I find to be useful when diagnosing models with many channels)
- Legend giving you differences in means rather than having to calculate them & add them to the fig
You think it's better to remove though?
It seems there is one error with a test ____________________ ERROR collecting tests/test_mlflow.py _____________________
ImportError while importing test module '/home/runner/work/pymc-marketing/pymc-marketing/tests/test_mlflow.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.10.16/x64/lib/python3.10/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
tests/test_mlflow.py:29: in <module>
from pymc_marketing.mlflow import (
E ImportError: cannot import name 'log_summary_metrics' from 'pymc_marketing.mlflow' (/home/runner/work/pymc-marketing/pymc-marketing/pymc_marketing/mlflow.py) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1368 +/- ##
==========================================
- Coverage 95.35% 93.86% -1.49%
==========================================
Files 47 48 +1
Lines 4995 5135 +140
==========================================
+ Hits 4763 4820 +57
- Misses 232 315 +83 ☔ View full report in Codecov by Sentry. |
Thanks for the work on this @louismagowan |
Description
Third time's the charm...
This PR is a recreation of this one which I was told to close and restart due to git issues on the pymc-marketing repo and then this one after, which was also closed due to some Git issues.
It could be nice to have some standardised model evaluation and diagnostic functions added to pymc-marketing. Ideally they'd be formulated in a way that makes them easy to log in MLFLow later on.
It would also be cool to build on top of the MLFlow module, to create a custom mlflow.pyfunc.PythonModel class to allow users to be able to register their models in the MLFlow registry. This would allow people to serve and maintain their MMMs more easily, and could help with MMM refreshes too.
Standard model metrics could include:
etc.
Diagnostic metrics could include:
Some additional plots (also useful for diagnosing models):
Model Registry / Additional Logging Code:
A wrapper for an MMM model to make it conform to the MLFlow api, enabling registering and easier deployment
Also an option to load models from the registry as well / or download the idata from MLflow too.
I'll open this as a Draft for now - since I'll need advice on how where best to put this code, as well as overall design etc.
Related Issue
Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--1368.org.readthedocs.build/en/1368/