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

GMC: adopt new common/menifests #203

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

KfreeZ
Copy link
Collaborator

@KfreeZ KfreeZ commented Jul 19, 2024

Description

This PR is a replica of #176 , since #176 has some difficulties to merge back because of the rebase issues. use this one to submit code.

this PR adopt the new manifests introduced by #173 and forget the old manifests. kept some code be comment out but not deleted in case the possible regression issue, after the old manifest deleted, these used code will be cleaned out.

this PR should've solved the issue #138, and also be part of solution to the #166 and #143

this PR also add support to WebRetriver service

Issues

#138, #166, #143

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)

Dependencies

'n/a'

Tests

e2e regression tests passed

here is the log of searchqa result:

kefei@satg-opea-4node-1:~/repos/kfreez/GenAIInfra/microservices-connector$ kubectl exec client-test-87d6c7d7b-wvxf5 -n chatqa -- curl http://router-service.searchqa.svc.cluster.local:8080  -X POST \
  -d '{"text":"What is the latest news? Give me also the source link."}' \
  -H 'Content-Type: application/json'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    65    0     0  100    65      0      2  0:00:32  0:00:27  0:00:05     0data: b'\n'

data: b'\n'

data: b'An'

data: b'swer'

data: b':'

data: b' The'

data: b' latest'

data: b' news'

data: b' can'

data: b' be'

data: b' found'

data: b' at'

data: b' the'

data: b' source'

data: b' link'

data: b':'

data: b' https'

data: b'://'

data: b'www'

data: b'.'

data: b'c'

data: b'nn'

data: b'.'

data: b'com'

data: b'/,'

data: b' where'

data: b' you'

data: b' can'

data: b' view'

data: b' breaking'

data: b' news'

data: b' today'

data: b' for'

data: b' various'

data: b' categories'

data: b' such'

data: b' as'

data: b' U'

data: b'.'

data: b'S'

data: b'.,'

data: b' world'

data: b','

data: b' weather'

data: b','

data: b' entertainment'

data: b','

data: b' politics'

data: b','

data: b' and'

data: b' health'

data: b'.'

data: b'</s>'

data: [DONE]

@@ -36,6 +36,7 @@ function copy_manifests() {
# Copy manifest into gmc
mkdir -p $(pwd)/config/manifests
cp $(dirname $(pwd))/manifests/ChatQnA/*.yaml -p $(pwd)/config/manifests/
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove coping from ChatQnA after all manifests finalized

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that is planned

tmpltFile := getManifestYaml(step)
if tmpltFile == "" {
return errors.New("unexpected target")
// func getManifestYaml(step string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@KfreeZ can these code lines be removed if they are not used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I'm on it

deployment.Spec.Template.Spec.Containers[i].Env = append(
deployment.Spec.Template.Spec.Containers[i].Env,
newEnvVars...)
if keyIsSomeEndpoint(name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if keyIsSomeEndpoint(name) {
if isDownStreamEndpointKey(name) {

Copy link
Collaborator

@zhlsunshine zhlsunshine left a comment

Choose a reason for hiding this comment

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

LGTM

@zhlsunshine zhlsunshine merged commit b185311 into opea-project:main Jul 22, 2024
7 checks passed
@KfreeZ KfreeZ mentioned this pull request Jul 23, 2024
3 tasks
@KfreeZ KfreeZ deleted the adoptNewManifest branch July 25, 2024 01:03
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.

4 participants