-
Notifications
You must be signed in to change notification settings - Fork 244
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
[Model Monitoring] Add MySQL support for saving and querying metrics #5641
[Model Monitoring] Add MySQL support for saving and querying metrics #5641
Conversation
Remove the previous dummy test
Use a shared function and substitute the cases
The last analyzed should not be empty
It doesn't work otherwise (I did many tries): ``` NotImplementedError: This backend does not support multiple-table criteria within DELETE ```
This removal unnecessitates the type casting
…-metrics-and-query # Conflicts: # mlrun/model_monitoring/db/stores/sqldb/sql_store.py
…-metrics-and-query # Conflicts: # mlrun/model_monitoring/db/stores/sqldb/sql_store.py
…-query # Conflicts: # mlrun/model_monitoring/db/stores/sqldb/models/__init__.py # mlrun/model_monitoring/db/stores/sqldb/sql_store.py # tests/model_monitoring/test_stores/test_sql.py
/metrics
endpointThere 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.
Overall looks good.
I have few comments.
Just saying we will have a lot of BC fun in 1.8.0 :)
) | ||
|
||
return [ | ||
mm_schemas.ModelEndpointMonitoringMetric( |
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’m not sure if this is the right PR, but yesterday you mentioned that the result kind is being returned to the UI. I don't see it here. I noticed it’s being returned from the TSDB, but it seems more appropriate to return it from the /metrics
endpoint since it’s metadata.
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.
"kind" isn't designed to be in this response, see section 2-c in the description of ML-6119.
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.
The design can be changed. Don't you think it's more accurate the way I mentioned?
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 think that's a valid point. I would also like to send the latest status in case of a result, to indicate which results should be inspected.
However, we discussed showing the status in the dropdown, and we deferred it (to >=1.8), so I think we shouldn't reopen now the API.
I believe it might not be that big here 😉 |
Implements ML-6268.
I tested the
/metrics
endpoint on a system configured to use MySQL in model monitoring, and it works:Example