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.

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

feat: Metrics Support in tritonfrontend #7703

Merged
merged 53 commits into from
Oct 31, 2024

Conversation

KrishnanPrash
Copy link
Contributor

@KrishnanPrash KrishnanPrash commented Oct 15, 2024

What does the PR do?

Adding support for Metrics in tritonfrontend. This involves two components:

  • In tritonfrontend_pybind.cc, added bindings for HTTPMetricsServer
  • In tritonfrontend/_api/_metrics.py, added a Metrics class

With this PR, similar to KServeHttp and KServeGrpc, the metrics service can used with:

import tritonserver
from tritonfrontend import Metrics
server = tritonserver.Server(model_repostory=...).start(wait_until_ready=True)
metrics_service = Metrics(server)
metrics_service.start()
...
metrics_service.stop()

Additional Changes made in this PR:

  • Modified test functions documentation based on this comment
  • Removed extra parameter in request.post(...) based on this comment

Test plan:

Added 3 test function to L0_python_api:

  • test_metrics_default_port() : Tests whether the metrics service can start as expected
  • test_metrics_custom_port(): Tests whether arguments defined in tritonfrontend.Metrics.Options are passed successfully to HTTPMetrics
  • test_metrics_update(): Tests whether nv_inference_count value goes from 0 to 1 if inference request is performed.
  • CI Pipeline ID: 19197748

GuanLuo
GuanLuo previously approved these changes Oct 23, 2024
@@ -57,14 +57,18 @@ Note: `model_path` may need to be edited depending on your setup.

2. Now, to start up the respective services with `tritonfrontend`
```python
from tritonfrontend import KServeHttp, KServeGrpc
from tritonfrontend import KServeHttp, KServeGrpc, Metrics
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love that the Metrics object is a web server, so it makes me wonder if we should rename these down the line, ex: KServeHttpService, MetricsService, etc

But I don't have a strong opinion on an alternative right now so I think it's fine, just mentioning for later. We will probably be restructing some packaging and naming in the near-mid future.

Copy link
Collaborator

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

Only minor comments - nice work!

Co-authored-by: Ryan McCormick <rmccormick@nvidia.com>
@KrishnanPrash KrishnanPrash merged commit 09cb1e5 into main Oct 31, 2024
3 checks passed
@KrishnanPrash KrishnanPrash deleted the kprashanth-tritonfrontend-metrics branch October 31, 2024 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feat A new feature
Development

Successfully merging this pull request may close these issues.

4 participants