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

Update dependents and fix frontend issues #287

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Update dependents and fix frontend issues #287

merged 2 commits into from
Mar 5, 2024

Conversation

umeshkumhar
Copy link
Collaborator

@umeshkumhar umeshkumhar commented Mar 5, 2024

  • introduce namespace helm module to prepare namespace before actual module, earlier this was done using depends_on = kuberay_operator
  • optmise frontend module to read endpoint from module output and remove addtional data sources.
  • set l4 node-locations default to us-central1-a for less quota or stockout issues.
  • disable IAP as default option.
  • tf plan reduced to 40s to 8s

@umeshkumhar
Copy link
Collaborator Author

/gcbrun

@@ -193,8 +198,7 @@ module "frontend" {
create_service_account = var.create_rag_service_account
google_service_account = var.rag_service_account
namespace = var.kubernetes_namespace
inference_service_name = module.inference-server.inference_service_name
inference_service_namespace = module.inference-server.inference_service_namespace
inference_service_endpoint = module.inference-server.inference_service_endpoint
db_secret_name = module.cloudsql.db_secret_name
db_secret_namespace = module.cloudsql.db_secret_namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this still needed?

@@ -24,6 +24,7 @@ kubernetes_namespace = "rag"
create_gcs_bucket = true
gcs_bucket = "rag-data-xyzu" # Choose a globally unique bucket name.

cloudsql_instance = "pgvector-instance"
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix formatting

output "inference_service_endpoint" {
description = "Endpoint of model inference service"
value = kubernetes_service.inference_service.status != null ? (kubernetes_service.inference_service.status[0].load_balancer != null ? "${kubernetes_service.inference_service.status[0].load_balancer[0].ingress[0].ip}" : "") : ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need these conditions?

@@ -171,7 +157,7 @@ resource "kubernetes_deployment" "rag_frontend_deployment" {
name = "DB_PASSWORD"
value_from {
secret_key_ref {
name = data.kubernetes_secret.db_secret.metadata.0.name
name = var.db_secret_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you test this? is the secret name the same as the value of "kubernetes_secret.db_secret.metadata.0.name"?
can you please make sure this works E2E? you will need to run the notebook to load data into the cloud sql instance, then launch the frontend and make a query.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 and please verify with Autopilot. Instructions for deploying AP cluster in the README.

@@ -142,14 +148,14 @@ module "jupyterhub" {
url_domain_name = var.jupyter_url_domain_name
members_allowlist = var.jupyter_members_allowlist

depends_on = [module.kuberay-operator, module.gcs]
depends_on = [module.namespace, module.gcs]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes way more sense, thank you. This will also improve apply time, since both kuberay-operator and jupyterhub take O(minutes) to deploy.

@@ -16,20 +16,6 @@ data "google_project" "project" {
}


data "kubernetes_service" "inference_service" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch.

@imreddy13 imreddy13 merged commit 9799818 into main Mar 5, 2024
6 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.

3 participants