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

Add Megaservice support for MMRAG VideoRAGQnA usecase #603

Merged
merged 14 commits into from
Sep 9, 2024

Conversation

BaoHuiling
Copy link
Collaborator

@BaoHuiling BaoHuiling commented Sep 3, 2024

Description

Add support for upcoming MMRAG VideoRAGQnA usecase in GenAIExamples, the PR is under developing.
Also fix bug founded

Issues

List the issue or RFC link this PR is working on. If there is no such link, please mark it as n/a.

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)
  • Others (enhancement, documentation, validation, etc.)

Dependencies

N/A

Tests

tests/cores/mega/test_service_orchestrator_with_videoragqnagateway.py

@BaoHuiling
Copy link
Collaborator Author

@tileintel Hello Tiep, I raise this PR for my megaservice. It's independent from you megaservice, just want to let you know. Thanks

@BaoHuiling BaoHuiling changed the title Add Megaservice support for MMRAG Add Megaservice support for MMRAG VideoRAGQnA usecase Sep 3, 2024
@BaoHuiling BaoHuiling added this to the v1.0 milestone Sep 3, 2024
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
comps/cores/mega/gateway.py 21.05% 15 Missing ⚠️
Files with missing lines Coverage Δ
comps/cores/mega/constants.py 98.24% <100.00%> (+0.03%) ⬆️
comps/cores/mega/gateway.py 35.18% <21.05%> (-0.72%) ⬇️

... and 2 files with indirect coverage changes

@BaoHuiling
Copy link
Collaborator Author

@chensuyue Hello Suyue, does this change requires additional test script/function? If yes, where to add?

@hshen14 hshen14 requested a review from ftian1 September 3, 2024 11:12
@BaoHuiling
Copy link
Collaborator Author

@ftian1 @chensuyue @ZePan110 Hello, anyone has comment on this one?

@hshen14
Copy link
Collaborator

hshen14 commented Sep 4, 2024

@chensuyue Hello Suyue, does this change requires additional test script/function? If yes, where to add?

please add unit tests to improve the code coverage.

@BaoHuiling
Copy link
Collaborator Author

BaoHuiling commented Sep 4, 2024

@chensuyue Hello Suyue, does this change requires additional test script/function? If yes, where to add?

please add unit tests to improve the code coverage.

No problem. Where should I add it, or do you have any references? I didn't saw the unit test for other gateways.

@chensuyue
Copy link
Collaborator

@chensuyue Hello Suyue, does this change requires additional test script/function? If yes, where to add?

please add unit tests to improve the code coverage.

No problem. Where should I add it, or do you have any references? I didn't saw the unit test for other gateways.

https://github.com/opea-project/GenAIComps/tree/main/tests/cores/mega

@BaoHuiling
Copy link
Collaborator Author

Test script added at tests/cores/mega/test_service_orchestrator_with_videoragqnagateway.py.
Would you help to review and merge the code? @lvliang-intel @ftian1 @hshen14 Thanks a lot

@BaoHuiling BaoHuiling requested a review from XuhuiRen September 6, 2024 07:37
@BaoHuiling
Copy link
Collaborator Author

Per Chang, Hanwen's requirement, update related code structure based on PR #625

@BaoHuiling
Copy link
Collaborator Author

Rebase the changes per @letonghan 's request, will wait until #625 is merged

@BaoHuiling
Copy link
Collaborator Author

Hello, @XuhuiRen @kevinintel the CI is all passed, could you help to merge the code? We have another PR in example repo depending on this, I’m afraid we don’t have enough time to wait until the Reorg changes merged.

@kevinintel kevinintel closed this Sep 9, 2024
@kevinintel kevinintel reopened this Sep 9, 2024
@kevinintel kevinintel merged commit 2c48bc8 into opea-project:main Sep 9, 2024
19 of 22 checks passed
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.

5 participants