-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Mamokari/observability #307
base: master
Are you sure you want to change the base?
Conversation
…ly if the env variable is set
Looks good to me |
value: diabetes_regression | ||
value: . |
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.
Why is this needed? This environment variable is used to keep the name of the folder, since the name will likely change during bootstrap. For this reason, we should avoid hardcoding this name in the pipeline and code.
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.
Looks great but we need to find a solution for all the diabetes_regression
references.
@@ -26,7 +26,11 @@ | |||
from azureml.core import Run | |||
import argparse | |||
import traceback | |||
from util.model_helper import get_model | |||
from diabetes_regression.util.model_helper import get_model |
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.
As @j-so mentioned, all the references to diabetes_regression
by name will not work when someone bootstraps and renames the project. We should find a way to reference this using a variable. Both in the python files and in the pipelines.
# Your application should run for at least this amount | ||
# of time so the exporter will meet this interval | ||
# Sleep can fulfill this https://pypi.org/project/opencensus-ext-azure/ | ||
time.sleep(self.export_interval) |
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.
Hey @mokarian , I am not sure if you should block each log_metric
call here. Probably would be better to add some runtime check at the end of the scripts using the log_metric
feature?! In the sample you are referencing they wait for the main script to be executed for 60s not each method call.
Hello @mokarian , thanks for your great work here! I am working on a fork of MLOpsPython for industry specific scenarios and we also want to integrate some observability features. I was so free to copy your work and slightly modified it. You may want to have a look at it here. I jumped started on it, so I couldn't discuss it so far with my team in detail, maybe we will change things further. |
This PR contains modules and documentation of Observability modules. Observability is an abstract solution to send logs and metrics to AzureML and Application Insights.
Components of this PR: