Sorted metric tags to avoid duplicate prom data with gRPC requests #4006

harshita-meena · 2022-03-24T23:53:54Z

What this PR does / why we need it:
To ensure no duplication of metrics occur in prometheus due to non-unique temp keys dependent on "tags" of custom metrics.

Which issue(s) this PR fixes:

Fixes #3968

Special notes for your reviewer:

seldondev · 2022-03-24T23:54:02Z

Hi @harshita-meena. Thanks for your PR.

I'm waiting for a SeldonIO or todo member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository.

ukclivecox · 2022-03-25T07:14:28Z

@harshita-meena Can you look at the linting and test errors?

harshita-meena · 2022-03-25T16:02:24Z

@harshita-meena Can you look at the linting and test errors?

Yes sure..taking a look now

axsaucedo · 2022-03-28T17:24:36Z

/test integration

axsaucedo · 2022-03-28T17:24:53Z

/ok-to-test

harshita-meena · 2022-03-28T18:57:13Z

/test this

seldondev · 2022-03-28T18:57:15Z

@harshita-meena: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test this

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository.

python/seldon_core/seldon_methods.py

axsaucedo · 2022-03-29T09:26:58Z

Btw just as a heads up @harshita-meena I had to add a dummy commit as it seems an issue in github was preventing the gh action pipelines to run, but that seems to have done the job

axsaucedo · 2022-03-29T09:32:37Z

@harshita-meena it seems there are some failures on the tests, are you able to run these locally? You can do so with act command

harshita-meena · 2022-03-29T13:49:13Z

@harshita-meena it seems there are some failures on the tests, are you able to run these locally? You can do so with act command

I was trying to follow the readme here but didn't get to complete the test, ill try your suggestion. thanks!

harshita-meena · 2022-03-30T15:27:07Z

I ran the act command locally, but seems to be unrelated to my change, I ran again from main branch, which is failing as well. Do you think it is something related to how my env is setup ? is there a particular python version i should use

| ________________ ERROR collecting adserver/tests/test_server.py ________________
| ImportError while importing test module '/home/hmeena/development/test/seldon-core/components/alibi-detect-server/adserver/tests/test_server.py'.
| Hint: make sure your test modules/packages have valid Python names.
| Traceback:
| /opt/conda/lib/python3.7/importlib/__init__.py:127: in import_module
|     return _bootstrap._gcd_import(name[level:], package, level)
| adserver/tests/test_server.py:1: in <module>
|     from adserver.server import Protocol, CEServer, CEModel
| adserver/server.py:11: in <module>
|     from adserver.base import CEModel, ModelResponse
| adserver/base/__init__.py:2: in <module>
|     from .alibi_model import AlibiDetectModel
| adserver/base/alibi_model.py:4: in <module>
|     from alibi_detect.utils.saving import load_detector, Data
| /opt/conda/lib/python3.7/site-packages/alibi_detect/utils/saving.py:24: in <module>
|     from alibi_detect.od import (IForest, LLR, Mahalanobis, OutlierAE, OutlierAEGMM, OutlierProphet,
| /opt/conda/lib/python3.7/site-packages/alibi_detect/__init__.py:1: in <module>
|     from . import ad, cd, models, od, utils
| /opt/conda/lib/python3.7/site-packages/alibi_detect/od/__init__.py:10: in <module>
|     from .llr import LLR
| /opt/conda/lib/python3.7/site-packages/alibi_detect/od/llr.py:13: in <module>
|     from alibi_detect.utils.perturbation import mutate_categorical
| /opt/conda/lib/python3.7/site-packages/alibi_detect/utils/perturbation.py:5: in <module>
|     import cv2
| /opt/conda/lib/python3.7/site-packages/cv2/__init__.py:8: in <module>
|     from .cv2 import *
| E   ImportError: libGL.so.1: cannot open shared object file: No such file or directory
| =========================== short test summary info ============================
| ERROR adserver/tests/test_ad_model.py
| ERROR adserver/tests/test_cd_model.py
| ERROR adserver/tests/test_cm_model.py
| ERROR adserver/tests/test_od_model.py
| ERROR adserver/tests/test_server.py
| !!!!!!!!!!!!!!!!!!! Interrupted: 5 errors during collection !!!!!!!!!!!!!!!!!!!!

axsaucedo · 2022-03-31T12:38:43Z

@harshita-meena it seems the docker image that you are using to run these tests doesn't have the dependencies required for Python CV2 library - you may need to specify explicitly the base image you use to run, alternatively you will have to extend the image you are using to have the cv2 dependencies (ie RUN apt-get install ffmpeg libsm6 libxext6 -y).

What is the command that you are using to run these tests? Are you using the -P parameter to override the default image?

harshita-meena · 2022-03-31T14:27:10Z

@harshita-meena it seems the docker image that you are using to run these tests doesn't have the dependencies required for Python CV2 library - you may need to specify explicitly the base image you use to run, alternatively you will have to extend the image you are using to have the cv2 dependencies (ie RUN apt-get install ffmpeg libsm6 libxext6 -y).

What is the command that you are using to run these tests? Are you using the -P parameter to override the default image?

I am just using simple act from the head of the repo, make sense what you said. Ill give it a shot.

harshita-meena · 2022-03-31T20:32:31Z

I am guessing this is the image used by default.

harshita-meena · 2022-05-12T22:23:20Z

@RafalSkolasinski Added the changes, is there a way I can trigger PR tests ? I did not quite figure out how to work it locally.

RafalSkolasinski · 2022-05-12T22:44:27Z

/test integration

RafalSkolasinski · 2022-05-12T22:47:38Z

Just unit tests should be sufficient here, however, would you be able to add an unit tests for it?

Some model that returns for first request metrics from predict with tags {"a": "a", "b": "b"} and for second {"b": "b", "a": "a"}should be fine. Could be a modification to UserObject test case here https://github.com/SeldonIO/seldon-core/blob/master/python/tests/test_runtime_metrics_tags.py

harshita-meena · 2022-05-12T22:50:34Z

Yups definitely, will add it, thanks for an example.

seldondev · 2022-05-13T01:22:30Z

@harshita-meena: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
integration	`b5b22c5`	link	`/test integration`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. I understand the commands that are listed here.

harshita-meena · 2022-05-13T01:59:57Z

/test integration

seldondev · 2022-05-13T01:59:59Z

@harshita-meena: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test integration

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository.

RafalSkolasinski · 2022-05-13T08:39:17Z

/ok-to-test

RafalSkolasinski · 2022-05-13T10:49:30Z

The python lint job seemed to fail, could you run fmt target from python/Makefile?

harshita-meena · 2022-05-13T20:59:15Z

/ok-to-test

harshita-meena · 2022-05-13T21:46:24Z

Great! I am pretty sure that now knowing the root of issue one could craft an example that also works wrongly for the predict / rest cases

I have tried these cases before as well but could not recreate it, maybe its still possible.
For REST the json data always ensured a sorted order, as for predict with gRPC, there was never a scenario where metrics were ingested from proto messages (a json "meta" object was converted to proto after metrics ingestion before sending gRPC response).

Either way, this small change should cover all such cases :)

harshita-meena · 2022-05-13T23:04:32Z

The fmt command got stuck so I used black command first as suggested, now ran isort.

RafalSkolasinski

Nice, thanks for finding the issue and contributing the fix!

seldondev · 2022-05-14T11:29:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RafalSkolasinski

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RafalSkolasinski]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

harshita-meena · 2022-05-20T15:14:05Z

@RafalSkolasinski Hi!
Wanted to know when can i expect the new patch version with this change ?

RafalSkolasinski · 2022-05-20T15:16:25Z

Hi, it will come with Seldon Core 1.14 but I don't know the exact release schedule for it. @cliveseldon may know more

Update seldon_methods.py

30511c7

seldondev added needs-ok-to-test size/XS labels Mar 24, 2022

Update seldon_methods.py

810ed62

harshita-meena changed the title ~~Sorted dictionary tags~~ Sorted dictionary tags to handle raw proto requests Mar 24, 2022

Update seldon_methods.py

48e4a11

axsaucedo requested a review from RafalSkolasinski March 28, 2022 10:23

seldondev added ok-to-test and removed needs-ok-to-test labels Mar 28, 2022

axsaucedo removed size/XS ok-to-test labels Mar 28, 2022

RafalSkolasinski reviewed Mar 28, 2022

View reviewed changes

python/seldon_core/seldon_methods.py Outdated Show resolved Hide resolved

Test commit to trigger CI

5d28a36

seldondev added the size/XS label Mar 29, 2022

harshita-meena added 2 commits March 29, 2022 13:45

iterate all tags in list of metrics

b95ed54

iterate all tags in list of metrics

6c33b67

Remove extra line

b5b22c5

add unit tests

1c452f8

seldondev added size/S and removed size/XS labels May 13, 2022

comment fix

0894598

seldondev added the ok-to-test label May 13, 2022

fix formatting

6e34822

harshita-meena changed the title ~~Sorted dictionary tags to handle raw proto requests~~ Sorted dictionary tags to avoid duplicate prom data with raw proto requests May 13, 2022

harshita-meena changed the title ~~Sorted dictionary tags to avoid duplicate prom data with raw proto requests~~ Sorted dictionary tags to avoid duplicate prom data with gRPC requests May 13, 2022

harshita-meena changed the title ~~Sorted dictionary tags to avoid duplicate prom data with gRPC requests~~ Sorted metric tags to avoid duplicate prom data with gRPC requests May 13, 2022

fix formatting

55869df

RafalSkolasinski approved these changes May 14, 2022

View reviewed changes

seldondev added the approved label May 14, 2022

seldondev merged commit e130e2c into SeldonIO:master May 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sorted metric tags to avoid duplicate prom data with gRPC requests #4006

Sorted metric tags to avoid duplicate prom data with gRPC requests #4006

harshita-meena commented Mar 24, 2022 •

edited

Loading

seldondev commented Mar 24, 2022

ukclivecox commented Mar 25, 2022

harshita-meena commented Mar 25, 2022

axsaucedo commented Mar 28, 2022

axsaucedo commented Mar 28, 2022

harshita-meena commented Mar 28, 2022

seldondev commented Mar 28, 2022

axsaucedo commented Mar 29, 2022

axsaucedo commented Mar 29, 2022

harshita-meena commented Mar 29, 2022

harshita-meena commented Mar 30, 2022 •

edited

Loading

axsaucedo commented Mar 31, 2022

harshita-meena commented Mar 31, 2022 •

edited

Loading

harshita-meena commented Mar 31, 2022

harshita-meena commented May 12, 2022

RafalSkolasinski commented May 12, 2022

RafalSkolasinski commented May 12, 2022 •

edited

Loading

harshita-meena commented May 12, 2022

seldondev commented May 13, 2022

harshita-meena commented May 13, 2022

seldondev commented May 13, 2022

RafalSkolasinski commented May 13, 2022

RafalSkolasinski commented May 13, 2022

harshita-meena commented May 13, 2022

harshita-meena commented May 13, 2022 •

edited

Loading

harshita-meena commented May 13, 2022

RafalSkolasinski left a comment

seldondev commented May 14, 2022

harshita-meena commented May 20, 2022

RafalSkolasinski commented May 20, 2022

Sorted metric tags to avoid duplicate prom data with gRPC requests #4006

Sorted metric tags to avoid duplicate prom data with gRPC requests #4006

Conversation

harshita-meena commented Mar 24, 2022 • edited Loading

seldondev commented Mar 24, 2022

ukclivecox commented Mar 25, 2022

harshita-meena commented Mar 25, 2022

axsaucedo commented Mar 28, 2022

axsaucedo commented Mar 28, 2022

harshita-meena commented Mar 28, 2022

seldondev commented Mar 28, 2022

axsaucedo commented Mar 29, 2022

axsaucedo commented Mar 29, 2022

harshita-meena commented Mar 29, 2022

harshita-meena commented Mar 30, 2022 • edited Loading

axsaucedo commented Mar 31, 2022

harshita-meena commented Mar 31, 2022 • edited Loading

harshita-meena commented Mar 31, 2022

harshita-meena commented May 12, 2022

RafalSkolasinski commented May 12, 2022

RafalSkolasinski commented May 12, 2022 • edited Loading

harshita-meena commented May 12, 2022

seldondev commented May 13, 2022

harshita-meena commented May 13, 2022

seldondev commented May 13, 2022

RafalSkolasinski commented May 13, 2022

RafalSkolasinski commented May 13, 2022

harshita-meena commented May 13, 2022

harshita-meena commented May 13, 2022 • edited Loading

harshita-meena commented May 13, 2022

RafalSkolasinski left a comment

Choose a reason for hiding this comment

seldondev commented May 14, 2022

harshita-meena commented May 20, 2022

RafalSkolasinski commented May 20, 2022

harshita-meena commented Mar 24, 2022 •

edited

Loading

harshita-meena commented Mar 30, 2022 •

edited

Loading

harshita-meena commented Mar 31, 2022 •

edited

Loading

RafalSkolasinski commented May 12, 2022 •

edited

Loading

harshita-meena commented May 13, 2022 •

edited

Loading