Feature/queue metrics #860

alvarorsant · 2022-11-21T07:57:25Z

Fix in tests, duplicate prometheus registry error

…er into feature/queue-metrics

adriangonz

Thanks for adding these changes!

Besides some naming conventions on the metrics, and some of the changes introduced in the tests, I think this looks great! We're almost there.

mlserver/parallel/dispatcher.py

tests/batching/test_adaptive.py

tests/metrics/test_endpoint.py

tests/metrics/test_rest.py

…/queue-metrics

adriangonz · 2022-11-29T17:42:03Z

Hey @alvarorsant ,

Just noticed some of the comments were marked as resolved, but I can't see any newer commits. Is there anything else that needs to be pushed before the next review cycle?

alvarorsant · 2022-11-30T08:06:39Z

Hi @adriangonz,
I've pushed the changes, let's see if everything is OK.

Thanks in advance for you dedication.

adriangonz

Nice one! Thanks for applying those changes and thanks a lot for your massive effort on this PR. I know there have been a few back and forths, so thanks for your patience! This is a great contribution to have in MLServer! 🚀

Once the tests and linter are green, this should be ready to go ahead. Let me know if you need any help with those. 👍

tests/batching/test_adaptive.py

tests/metrics/test_endpoint.py

adriangonz · 2022-12-07T09:41:29Z

BTW on the CI checks, linter seems to be complaining about a few unused imports. You can run this locally by calling make lint.

flake8 .
./tests/grpc/test_servicers.py:3:1: F401 '.conftest.delete_registry' imported but unused
./tests/grpc/test_servicers.py:4:1: F401 '..metrics.conftest.prometheus_registry' imported but unused
./tests/grpc/test_servicers.py:23:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:32:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:41:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:50:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:61:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:77:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:98:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:127:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:153:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:164:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:174:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:186:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/test_servicers.py:188:5: F811 redefinition of unused 'delete_registry' from line 3
./tests/grpc/test_servicers.py:205:5: F811 redefinition of unused 'prometheus_registry' from line 4
./tests/grpc/conftest.py:17:1: F811 redefinition of unused 'CollectorRegistry' from line 7
./tests/grpc/test_model_repository.py:20:1: F401 '..metrics.conftest.prometheus_registry' imported but unused
./tests/grpc/test_model_repository.py:91:5: F811 redefinition of unused 'prometheus_registry' from line 20
./tests/grpc/test_model_repository.py:108:5: F811 redefinition of unused 'prometheus_registry' from line 20
./tests/metrics/conftest.py:4:1: F401 'prometheus_client.registry.REGISTRY' imported but unused
./tests/metrics/conftest.py:4:1: F401 'prometheus_client.registry.CollectorRegistry' imported but unused
./tests/kafka/test_server.py:10:1: F401 '..metrics.conftest.prometheus_registry' imported but unused
./tests/kafka/test_server.py:15:5: F811 redefinition of unused 'prometheus_registry' from line 10
./tests/kafka/test_server.py:37:5: F811 redefinition of unused 'prometheus_registry' from line 10
./tests/batching/test_adaptive.py:15:1: F401 'prometheus_client.registry.CollectorRegistry' imported but unused
./tests/batching/test_hooks.py:6:1: F401 'prometheus_client.registry.CollectorRegistry' imported but unused
./tests/rest/test_endpoints.py:14:1: F401 '..metrics.conftest.prometheus_registry' imported but unused
./tests/rest/test_endpoints.py:15:1: F401 '.conftest.delete_registry' imported but unused
./tests/rest/test_endpoints.py:19:21: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:26:22: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:34:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:42:25: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:54:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:71:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:90:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:112:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:122:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:134:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:146:5: F811 redefinition of unused 'delete_registry' from line 15
./tests/rest/test_endpoints.py:149:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_endpoints.py:163:5: F811 redefinition of unused 'prometheus_registry' from line 14
./tests/rest/test_custom.py:2:1: F401 '..metrics.conftest.prometheus_registry' imported but unused
./tests/rest/test_custom.py:7:5: F811 redefinition of unused 'prometheus_registry' from line 2
./tests/rest/test_custom.py:18:5: F811 redefinition of unused 'prometheus_registry' from line 2
./tests/rest/test_server.py:6:1: F401 '..metrics.conftest.prometheus_registry' imported but unused
./tests/rest/test_server.py:11:5: F811 redefinition of unused 'prometheus_registry' from line 6
./tests/rest/test_server.py:25:5: F811 redefinition of unused 'prometheus_registry' from line 6

Similarly, tests seem to fail due to a import error as well. You can kick these off locally by calling python -m pytest (or make test, but this will be a bit slower).

tests/grpc/conftest.py:22: in <module>
    from ..metrics.conftest import prometheus_registry  # noqa: F401
E   ImportError: cannot import name 'prometheus_registry' from 'tests.metrics.conftest' (/home/runner/work/MLServer/MLServer/tests/metrics/conftest.py)

alvarorsant · 2022-12-15T15:53:45Z

how can I solve the last errors of linter?

adriangonz · 2022-12-16T09:21:17Z

Hey @alvarorsant ,

It may be that you're running an older version of some of the artefacts.

Could you try to rebase from master?

…/queue-metrics

alvarorsant · 2022-12-16T11:10:22Z

Done!

alvarorsant · 2022-12-19T06:56:40Z

If I do make lint locally, it works.
All done! ✨ 🍰 ✨
8 files reformatted, 278 files left unchanged.
flake8 .
mypy ./mlserver
Success: no issues found in 86 source files
for _runtime in ./runtimes/*;
do
mypy $_runtime || exit 1;
done
Success: no issues found in 8 source files
Success: no issues found in 21 source files
Success: no issues found in 31 source files
Success: no issues found in 7 source files
Success: no issues found in 16 source files
Success: no issues found in 6 source files
Success: no issues found in 7 source files
Success: no issues found in 7 source files
mypy ./benchmarking
Success: no issues found in 2 source files
mypy ./docs/examples
Success: no issues found in 4 source files

Check if something has changed after generation

git
--no-pager diff
--exit-code
.

adriangonz · 2022-12-19T09:34:43Z

Hey @alvarorsant ,

It seems one of the rebases missed the latest auto-generated gRPC stubs from master. Not sure why they weren't showing on the PR's files diff.

I've pushed them manually to this branch now, so hopefully linter should pass in CI now.

adriangonz · 2022-12-19T17:37:30Z

Hey @alvarorsant ,

It seems some of the runtime tests get stuck while running. I tried running these locally, and the errors I get look along the following lines:

======================================================= short test summary info ========================================================
ERROR runtimes/mlflow/tests/rest/test_endpoints.py::test_ping[-/health] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_endpoints.py::test_ping[file:-/ping] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_endpoints.py::test_ping[file:-/health] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_endpoints.py::test_version[] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_endpoints.py::test_version[file:] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_invalid_content_type[-] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_invalid_content_type[-None] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_invalid_content_type[-application/pdf] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_invalid_content_type[file:-] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_invalid_content_type[file:-None] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_invalid_content_type[file:-application/pdf] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_tensor[-application/json-payload0] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_tensor[-application/json-payload1] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_tensor[-application/json-payload2] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_tensor[file:-application/json-payload0] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_tensor[file:-application/json-payload1] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_tensor[file:-application/json-payload2] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_dataframe[-application/json-payload0] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_dataframe[-application/json-payload1] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_dataframe[file:-application/json-payload0] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...
ERROR runtimes/mlflow/tests/rest/test_invocations.py::test_invocations_dataframe[file:-application/json-payload1] - ValueError: Duplicated timeseries in CollectorRegistry: {'parallel_request_queue_bucket', 'parallel_request_queue_sum', 'parallel_r...

You should be able to run these locally with the command below (note that you can also do make test which will run the entire suite):

tox -e mlflow

alvarorsant · 2022-12-20T11:54:55Z

It's already corrected

alvarorsant · 2022-12-20T13:24:26Z

runtimes/mlflow/tests/rest/conftest.py:19: error: The return type of a generator function should be "Generator" or one of its supertypes [misc]
Found 1 error in 1 file (checked 16 source files) ¿?

I already used this method without a problem

alvarorsant · 2022-12-21T14:51:07Z

Hi @adriangonz ,
please, could you check it out now?

alvarorsant · 2023-01-10T09:52:59Z

Hi @adriangonz,
I consider the changes have been approved. In which release do they all come in?

adriangonz · 2023-01-10T16:21:36Z

Hey @alvarorsant ,

It seems some tests have failed due to an issue that's now fixed in master. Could you rebase to ensure the latest is on the PR?

alvarorsant · 2023-01-11T06:10:11Z

Done , I've already updated it

adriangonz · 2023-01-11T09:59:32Z

Hey @alvarorsant ,

It seems during your last merge from master, the old gRPC stubs came back again (i.e. the ones diverging from master). This is why, in general, I don't trust merges much and prefer to do rebases instead. They also tend to keep the history cleaner.

Given this PR has already been opened for a while, to keep things moving, I've pushed the new ones from master manually. Hope that's alright with you, and hope that fixes the linter.

adriangonz · 2023-01-11T10:35:13Z

All seems good now! Thanks a lot for your effort on this one @alvarorsant 👍

Regarding releases, this will likely come out on the 1.3 release, which is due in march.

alvarorsant · 2023-01-11T14:49:45Z

Thanks @adriangonz

alvarorsant added 4 commits November 16, 2022 11:48

Add queue batch and queue requests metrics

0a4f7c2

Merge branch 'feature/queue-metrics' of github.com:alvarorsant/MLServ…

ab7066c

…er into feature/queue-metrics

Merge branch 'feature/queue-metrics' of github.com:alvarorsant/MLServ…

9941db6

…er into feature/queue-metrics

Fix duplicate registry error in tests

6c43a26

alvarorsant mentioned this pull request Nov 21, 2022

Get metrics from the Queues in the inference in parallel/pool #728

Closed

adriangonz suggested changes Nov 22, 2022

View reviewed changes

Merge branch 'master' of github.com:alvarorsant/MLServer into feature…

b0a2b44

…/queue-metrics

alvarorsant added 3 commits November 30, 2022 08:48

Add test_queue with the new metrics

61300bf

Add prometheus deleting registry function in main conftest

814904a

Format

0ccc2f6

adriangonz mentioned this pull request Dec 7, 2022

Add worker_queue_request, batch_queue and worker_processes list metrics #826

Closed

adriangonz approved these changes Dec 7, 2022

View reviewed changes

tests/batching/test_adaptive.py Outdated Show resolved Hide resolved

tests/metrics/test_endpoint.py Show resolved Hide resolved

fix: tests and linter

aa3da29

Merge branch 'master' of github.com:alvarorsant/MLServer into feature…

03fdc7d

…/queue-metrics

Re-generate gRPC stubs as they'd diverged from master

bba664e

alvarorsant and others added 2 commits December 20, 2022 10:58

Merge branch 'SeldonIO:master' into feature/queue-metrics

826ac9e

fix: delete prometheus registry between tests

10ebaee

alvarorsant and others added 2 commits December 21, 2022 15:28

Merge branch 'SeldonIO:master' into feature/queue-metrics

ac2f78b

fix: delete registry between tests

6236fe1

Merge branch 'SeldonIO:master' into feature/queue-metrics

62edea9

Merge branch 'SeldonIO:master' into feature/queue-metrics

8e9c724

Restoring grpc stubs from master

458a4d7

adriangonz merged commit f13a129 into SeldonIO:master Jan 11, 2023

alvarorsant deleted the feature/queue-metrics branch January 11, 2023 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/queue metrics #860

Feature/queue metrics #860

alvarorsant commented Nov 21, 2022

adriangonz left a comment

adriangonz commented Nov 29, 2022

alvarorsant commented Nov 30, 2022

adriangonz left a comment

adriangonz commented Dec 7, 2022

alvarorsant commented Dec 15, 2022

adriangonz commented Dec 16, 2022

alvarorsant commented Dec 16, 2022

alvarorsant commented Dec 19, 2022

adriangonz commented Dec 19, 2022

adriangonz commented Dec 19, 2022

alvarorsant commented Dec 20, 2022

alvarorsant commented Dec 20, 2022 •

edited

Loading

alvarorsant commented Dec 21, 2022

alvarorsant commented Jan 10, 2023 •

edited

Loading

adriangonz commented Jan 10, 2023

alvarorsant commented Jan 11, 2023

adriangonz commented Jan 11, 2023 •

edited

Loading

adriangonz commented Jan 11, 2023

alvarorsant commented Jan 11, 2023

Feature/queue metrics #860

Feature/queue metrics #860

Conversation

alvarorsant commented Nov 21, 2022

adriangonz left a comment

Choose a reason for hiding this comment

adriangonz commented Nov 29, 2022

alvarorsant commented Nov 30, 2022

adriangonz left a comment

Choose a reason for hiding this comment

adriangonz commented Dec 7, 2022

alvarorsant commented Dec 15, 2022

adriangonz commented Dec 16, 2022

alvarorsant commented Dec 16, 2022

alvarorsant commented Dec 19, 2022

Check if something has changed after generation

adriangonz commented Dec 19, 2022

adriangonz commented Dec 19, 2022

alvarorsant commented Dec 20, 2022

alvarorsant commented Dec 20, 2022 • edited Loading

alvarorsant commented Dec 21, 2022

alvarorsant commented Jan 10, 2023 • edited Loading

adriangonz commented Jan 10, 2023

alvarorsant commented Jan 11, 2023

adriangonz commented Jan 11, 2023 • edited Loading

adriangonz commented Jan 11, 2023

alvarorsant commented Jan 11, 2023

alvarorsant commented Dec 20, 2022 •

edited

Loading

alvarorsant commented Jan 10, 2023 •

edited

Loading

adriangonz commented Jan 11, 2023 •

edited

Loading