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

Generalize BaseStatistics code a bit + document it #1107

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eero-t
Copy link
Contributor

@eero-t eero-t commented Jan 3, 2025

Description

Generalize BaseStatistics code a bit + document it.

Issues

n/a.

Type of change

  • Others (enhancement, documentation, validation, etc.)

Dependencies

n/a.

Tests

Pylint likes the change, and I manually checked ChatQnA microservice before and after the change.

@eero-t eero-t requested a review from lvliang-intel as a code owner January 3, 2025 12:08
@eero-t eero-t marked this pull request as draft January 3, 2025 12:08
@eero-t
Copy link
Contributor Author

eero-t commented Jan 3, 2025

Setting as draft, until I've done more testing on it.

Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
comps/cores/mega/base_statistics.py 14.28% 12 Missing ⚠️
Files with missing lines Coverage Δ
comps/cores/mega/base_statistics.py 45.71% <14.28%> (+3.60%) ⬆️

@eero-t
Copy link
Contributor Author

eero-t commented Jan 3, 2025

Attention: Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.

Hm. It appears that statistics output is not tested, so none of the code I changed is covered.

Because my refactoring removed more code than it added, the overall coverage should have improved though.

@eero-t eero-t marked this pull request as ready for review January 3, 2025 16:32
@eero-t
Copy link
Contributor Author

eero-t commented Jan 3, 2025

Was hard finding a microservice providing these metrics. None of "DocSum" ones provided them, but "ChatQnA" had one.

It works the same before and after the change:

$ curl --no-progress-meter 172.21.145.237:7000/v1/statistics | jq
{
  "opea_service@retriever_redis": {
    "p50_latency": 0.0011343955993652344,
    "p99_latency": 0.002569167613983175,
    "average_latency": 0.0012106266286638048,
    "p50_latency_first_token": null,
    "p99_latency_first_token": null,
    "average_latency_first_token": null
  }
}

@Spycsh
Copy link
Member

Spycsh commented Jan 6, 2025

Attention: Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.

Hm. It appears that statistics output is not tested, so none of the code I changed is covered.

Because my refactoring removed more code than it added, the overall coverage should have improved though.

Thanks for adding documentation. Could you also add the test to cover your code? There is a very simple test of this functionality here https://github.com/eero-t/GenAIComps/blob/stats/tests/cores/mega/test_base_statistics.py. You can add some unit test there to for coverage. (The original test init a server so the code is not detected by the coverage test I guess).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants