Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Model Monitoring] Fix SQL store with multiple filter criteria #5629

Merged
merged 22 commits into from
May 27, 2024
Merged

[Model Monitoring] Fix SQL store with multiple filter criteria #5629

merged 22 commits into from
May 27, 2024

Conversation

jond01
Copy link
Member

@jond01 jond01 commented May 26, 2024 •
edited by jira bot
Loading

Fixes ML-6541.

Add a test that fails before the fix introduced here: test_unique_last_analyzed_per_app.
The bug comes from the fact that sqlalchemy.sql.text cannot combine by itself multiple conditions.

jond01 added 17 commits May 26, 2024 13:56
@jond01
Add a basic SQLite store test
509e617
@jond01
Add an xfail test
1f74473 
Remove the previous dummy test
@jond01
Refactor generate application result
45145ce
@jond01
Import relevant sqlalchemy subpackages
dbfee43
@jond01
Add logs the the _get method
2175585
@jond01
Convert to UTC timezone in sql method
d825513
@jond01
Add a new test case to test_get_app_metrics_table
4a10953
@jond01
Move test_store.py to test_stores/test_sql.py
badadd5
@jond01
Refactor get SQL table functions
27ae3ad 
Use a shared function and substitute the cases
@jond01
Remove a misleading "empty" comment
fd547b5 
The last analyzed should not be empty
@jond01
Use criteria list instead of text in the _update method
b4f9df8
@jond01
Use an explicit import from utils
a3103b2
@jond01
Use a criteria list in the _delete method
000e57f
@jond01
Delete app results for all the endpoints
d576664 
It doesn't work otherwise (I did many tries):
```
NotImplementedError: This backend does not support multiple-table criteria within DELETE
```
@jond01
Use criteria in the _get method
4690837
@jond01
Remove the filter_endpoint_and_application_name method
53f567b
@jond01
Remove the xfail marker from the test
04d19b2
@assaf758 assaf758 requested a review from davesh0812 May 27, 2024 06:42
davesh0812
davesh0812 reviewed May 27, 2024
View reviewed changes
Copy link
Contributor

@davesh0812 davesh0812 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good!
Few extra comments.

mlrun/model_monitoring/db/stores/sqldb/sql_store.py Outdated Show resolved Hide resolved
mlrun/model_monitoring/db/stores/sqldb/sql_store.py Outdated Show resolved Hide resolved
mlrun/model_monitoring/db/stores/sqldb/models/__init__.py Show resolved Hide resolved
@jond01 jond01 requested a review from davesh0812 May 27, 2024 13:32
davesh0812
davesh0812 approved these changes May 27, 2024
View reviewed changes
Copy link
Contributor

@davesh0812 davesh0812 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
If it is not a big effort please add test for _delete_application_result and pass to it application_name - not mandatory for now.

@assaf758 assaf758 merged commit 76e658b into mlrun:development May 27, 2024
11 checks passed
@jond01 jond01 mentioned this pull request May 27, 2024
Merged
@jond01 jond01 deleted the fix/mysql-last-analyzed branch May 27, 2024 14:29
rokatyy pushed a commit to rokatyy/mlrun that referenced this pull request May 28, 2024
@jond01 @rokatyy
[Model Monitoring] Fix SQL store with multiple filter criteria (mlrun…
60f1cba 
…#5629)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davesh0812 davesh0812 davesh0812 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jond01 @davesh0812 @assaf758