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

chatqna ui - nginx proxy_pass #616

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

SeanCondon
Copy link

@SeanCondon SeanCondon commented Aug 17, 2024

Description

This adds a proxy_pass inside the nginx.conf of the chatqna-conversation-ui. This has the effect of avoiding CORS errors when the Javascript in the browser calls the backend service. CORS error occur when the javascript makes a call to a backend host:port not equal to where it was loaded from.

The effect is that the javascript now calls http://host:port/v1/chatqna which the nginx service in the pod will remap to http://chatqna:8888/v1/chatqna inside the cluster.

This was undertaken on the suggestion of @Ruoyu-y in this PR 305 on GenAIInfra

This is an additional fix for #493 and #528

Issues

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

List the newly introduced 3rd party dependency if exists.

Tests

Load the services with docker compose according to the instructions in README.md in docker/xeon


location ~* \.(gif|jpe?g|png|webp|ico|svg|css|js|mp4|woff2)$ {
expires 1d;
}
}

location "/v1/chatqna" {
proxy_pass $CHAT_BASE_URL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the environment variable correct ? compose.yaml is setting APP_BACKEND_SERVICE_ENDPOINT and APP_DATA_PREP_SERVICE_URL as env variables according to https://github.com/opea-project/GenAIExamples/blob/main/ChatQnA/docker/xeon/README.md?plain=1#L403.

Copy link
Author

Choose a reason for hiding this comment

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

@jaswanth8888 thanks for pointing this out.

In the other UI the same variable is given the name CHAT_BASE_URL.
I think I would change this README (and the one for Gaudi and Gpu) to use the same env var name.

  • APP_BACKEND_SERVICE_ENDPOINT -> CHAT_BASE_URL

I don't see any point in having 2 names for the same thing.
I would do the same thing for

  • APP_DATA_PREP_SERVICE_URL -> UPLOAD_FILE_BASE_URL

@jaswanth8888
Copy link
Collaborator

@SeanCondon with this approach for every api that would be integrated in the UI we have to update nginx.conf file. will this don't be a overhead when it comes to productivity suite in which all the examples would be part of single UI.

@SeanCondon
Copy link
Author

@SeanCondon with this approach for every api that would be integrated in the UI we have to update nginx.conf file. will this don't be a overhead when it comes to productivity suite in which all the examples would be part of single UI.

Do you have a pointer to the productivity suite?
I think this proxy_pass for the nginx.conf will be necessary in all situations, as it is the best way for avoiding CORS restrictions securely.
The only other alternative is to have a separate load balancer container in front of the UI that will handle these redirects, but that would require the same amount of maintenance.

@jaswanth8888
Copy link
Collaborator

@SeanCondon with this approach for every api that would be integrated in the UI we have to update nginx.conf file. will this don't be a overhead when it comes to productivity suite in which all the examples would be part of single UI.

Do you have a pointer to the productivity suite? I think this proxy_pass for the nginx.conf will be necessary in all situations, as it is the best way for avoiding CORS restrictions securely. The only other alternative is to have a separate load balancer container in front of the UI that will handle these redirects, but that would require the same amount of maintenance.

@SeanCondon #592 please have look at this PR

@chensuyue chensuyue modified the milestones: v0.9, v1.0 Aug 27, 2024
@lvliang-intel
Copy link
Collaborator

@SeanCondon,
Maybe you can have a check about the recent changes in 'svelte' UI. We have added some endpoints for other file operations.

CHAT_BASE_URL = 'http://backend_address:8888/v1/chatqna'

UPLOAD_FILE_BASE_URL = 'http://backend_address:6007/v1/dataprep'

GET_FILE = 'http://backend_address:6007/v1/dataprep/get_file'

DELETE_FILE = 'http://backend_address:6007/v1/dataprep/delete_file'

@SeanCondon
Copy link
Author

@SeanCondon, Maybe you can have a check about the recent changes in 'svelte' UI. We have added some endpoints for other file operations.

CHAT_BASE_URL = 'http://backend_address:8888/v1/chatqna'

UPLOAD_FILE_BASE_URL = 'http://backend_address:6007/v1/dataprep'

GET_FILE = 'http://backend_address:6007/v1/dataprep/get_file'

DELETE_FILE = 'http://backend_address:6007/v1/dataprep/delete_file'

I have looked at this UI and I think it needs the same treatment of having a proxy.

To be honest I have tried to submit this PR and did not get any kind of positive feedback.

I do not have time to keep chasing the CI while I wait for some constructive feedback.

CC @jaswanth8888

@SeanCondon SeanCondon force-pushed the scChatUiNginx branch 2 times, most recently from 748ba55 to 6ff0e67 Compare September 9, 2024 09:03
@SeanCondon
Copy link
Author

@SeanCondon, Maybe you can have a check about the recent changes in 'svelte' UI. We have added some endpoints for other file operations.

CHAT_BASE_URL = 'http://backend_address:8888/v1/chatqna'

UPLOAD_FILE_BASE_URL = 'http://backend_address:6007/v1/dataprep'

GET_FILE = 'http://backend_address:6007/v1/dataprep/get_file'

DELETE_FILE = 'http://backend_address:6007/v1/dataprep/delete_file'

@lvliang-intel please see my latest changes to tackle the Svelte UI

@chensuyue
Copy link
Collaborator

If this is target v1.0, we need to merge by today.

@chensuyue chensuyue removed this from the v1.0 milestone Sep 12, 2024
@SeanCondon SeanCondon force-pushed the scChatUiNginx branch 2 times, most recently from c7ed49c to 0e475e2 Compare September 12, 2024 15:50
@chensuyue
Copy link
Collaborator

Do we still need this PR? @SeanCondon

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