Skip to content

Notebooks use PGVector, not FAISS - #43

Merged
JslYoon merged 1 commit into
redhat-ai-dev:mainfrom
JslYoon:JslYoon-pgvector-enable
Jul 23, 2026
Merged

Notebooks use PGVector, not FAISS#43
JslYoon merged 1 commit into
redhat-ai-dev:mainfrom
JslYoon:JslYoon-pgvector-enable

Conversation

@JslYoon

@JslYoon JslYoon commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?:

migrating from FAISS to pgvectors on notebooks

Which issue(s) this PR fixes:

RHDHBUGS-3302

PR acceptance criteria:

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened and linked to this PR, if they are not in the PR scope due to various constraints.

  • Tested and Verified

  • Documentation (READMEs, Product Docs, Blogs, Education Modules, etc.)

How to test changes / Special notes to the reviewer:

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Switch notebooks vector store from FAISS to remote PGVector

✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Add a pgvector Postgres service to local compose, with healthchecks and persisted volume.
• Configure notebooks provider to use remote::pgvector with HNSW + cosine distance.
• Introduce PGVECTOR_* environment defaults and remove FAISS notebooks KV store config.
Diagram

graph TD
  Compose["compose/compose.yaml"] --> Core["lightspeed-core"] --> PG[("PGVector DB")]
  Compose --> Env["env/default-values.env"] --> Cfg["llama-stack-configs/config.yaml"] --> Core
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Dual-backend toggle (FAISS fallback)
  • ➕ Safer rollout path; easy to revert without reverting PRs
  • ➕ Supports environments without Postgres/pgvector available
  • ➖ More config surface area and testing matrix
  • ➖ May delay fully removing FAISS assumptions
2. Use an existing Postgres instance instead of compose-managed pgvector
  • ➕ Avoids running an extra container in dev; closer to production DB patterns
  • ➕ Leverages existing backup/ops practices if already present
  • ➖ Harder local onboarding; more external prerequisites
  • ➖ Still requires pgvector extension installation/enablement
3. Add an explicit migration/compat layer for existing FAISS notebook data
  • ➕ Prevents silent ‘empty index’ behavior for existing users
  • ➕ Makes data continuity expectations explicit
  • ➖ Not always possible if formats differ; increases scope substantially
  • ➖ Requires additional tooling, docs, and testing

Recommendation: For the stated scope (notebooks moving to PGVector in dev notebooks), the chosen approach is appropriate: wire up a pgvector container, drive connection via env vars, and switch the provider config. If this is intended for broad consumption beyond dev notebooks, consider adding a simple feature toggle to keep FAISS as an escape hatch during rollout, and open a tracking issue for data migration/compatibility expectations.

Files changed (3) +46 / -6

Enhancement (1) +14 / -6
config.yamlSwitch notebooks provider from inline FAISS to remote PGVector (HNSW + cosine) +14/-6

Switch notebooks provider from inline FAISS to remote PGVector (HNSW + cosine)

• Changes the notebooks provider type to remote::pgvector and adds connection parameters sourced from PGVECTOR_* environment variables. Configures COSINE distance and an HNSW index, and updates persistence to use the pgvector namespace/backend while removing the old kv_notebooks FAISS sqlite store entry.

llama-stack-configs/config.yaml

Other (2) +32 / -0
compose.yamlAdd pgvector service and gate lightspeed-core startup on DB health +25/-0

Add pgvector service and gate lightspeed-core startup on DB health

• Introduces a new pgvector/pgvector:pg17 service exposing 5432 with a persisted volume and pg_isready healthcheck. Updates lightspeed-core to depend on pgvector being healthy before starting, and defines the pgvector-data named volume.

compose/compose.yaml

default-values.envAdd default PGVECTOR_* connection settings +7/-0

Add default PGVECTOR_* connection settings

• Adds PGVECTOR_HOST/PORT/DB/USER/PASSWORD defaults used by the notebooks vector-store configuration to connect to the pgvector service.

env/default-values.env

@qodo-code-review

qodo-code-review Bot commented Jul 22, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

Grey Divider


Remediation recommended

1. Weak creds on published DB ✓ Resolved 🐞 Bug ⛨ Security
Description
compose/compose.yaml publishes the pgvector Postgres port to the host (5432:5432) while
defaulting credentials to llama/llamapass, which makes the DB reachable to anyone who can access
the host port and guess defaults. This is a security footgun for shared, cloud, or otherwise
reachable dev environments and can expose/allow modification of notebook vector data.
Code

compose/compose.yaml[R17-25]

+  pgvector:
+    image: pgvector/pgvector:pg17
+    ports:
+      - "5432:5432"
+    environment:
+      POSTGRES_DB: ${PGVECTOR_DB:-llama_stack}
+      POSTGRES_USER: ${PGVECTOR_USER:-llama}
+      POSTGRES_PASSWORD: ${PGVECTOR_PASSWORD:-llamapass}
+    volumes:
Relevance

⭐⭐ Medium

Team accepted reducing local exposure (PR26) but rejected removing unsafe default creds placeholders
in compose/env (PR41).

PR-#26
PR-#41

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The compose file publishes Postgres to the host and defaults credentials; the env template and llama
stack config also default the password, making the weak credentials likely to be used in practice.

compose/compose.yaml[16-36]
env/default-values.env[33-39]
llama-stack-configs/config.yaml[53-61]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The pgvector Postgres service is published to the host network (`5432:5432`) while also providing predictable default credentials (`llama` / `llamapass`) in both compose and the default env template. This combination makes it easy to accidentally run an externally reachable DB with known credentials.

## Issue Context
This repo’s `compose/compose.yaml` is used for local dev (`make local-up`). Default env values are copied from `env/default-values.env` to `env/values.env`.

## Fix Focus Areas
- compose/compose.yaml[16-36]
- env/default-values.env[33-39]
- llama-stack-configs/config.yaml[53-61]

## Suggested changes
- Bind the published port to loopback only (e.g. `127.0.0.1:5432:5432`) or remove `ports:` entirely and use `expose: ["5432"]` (Lightspeed Core can still reach the DB via the compose network).
- Remove the hardcoded default password in `env/default-values.env` (leave empty) and/or require `PGVECTOR_PASSWORD` to be explicitly set.
- If defaults are kept for convenience, add a prominent comment warning that the service is published and credentials must be changed for any non-local use.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Pgvector dependency underspecified ✓ Resolved 🐞 Bug ☼ Reliability
Description
llama-stack-configs/config.yaml now defaults the notebooks vector store to remote::pgvector at
host pgvector, but this repo only provisions that hostname in local compose; other deployment
paths must supply an external pgvector/Postgres endpoint and credentials or notebook vector-store
operations will fail to connect. This makes the config change easy to adopt incompletely outside
compose.
Code

llama-stack-configs/config.yaml[R54-60]

+      provider_type: remote::pgvector
      config:
+        host: ${env.PGVECTOR_HOST:=pgvector}
+        port: ${env.PGVECTOR_PORT:=5432}
+        db: ${env.PGVECTOR_DB:=llama_stack}
+        user: ${env.PGVECTOR_USER:=llama}
+        password: ${env.PGVECTOR_PASSWORD:=llamapass}
Relevance

⭐ Low

Similar “document/clarify external provisioning/init” config concerns were rejected as out-of-scope
(PR18).

PR-#18

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The default llama-stack config hardcodes a pgvector hostname pgvector; the repo provisions that
name only in compose, and the GitOps generator only emits ConfigMaps (no DB provisioning), leaving
non-compose deployments dependent on external, undocumented setup.

llama-stack-configs/config.yaml[46-70]
compose/compose.yaml[16-53]
scripts/generate-gitops-manifests.sh[41-55]
docs/PROVIDERS.md[1-40]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The notebooks provider now points to a remote pgvector DB with defaults that assume a DNS name `pgvector`. The repo adds a compose service named `pgvector`, but there is no corresponding guidance or provisioning for non-compose deployments in this repository, so users can end up with a configuration that cannot connect to any database.

## Issue Context
- `llama-stack-configs/config.yaml` is exported to GitOps as a ConfigMap by `scripts/generate-gitops-manifests.sh`.
- `docs/PROVIDERS.md` currently documents provider env vars primarily for inference providers and does not describe the new pgvector vector store dependency.

## Fix Focus Areas
- llama-stack-configs/config.yaml[46-70]
- compose/compose.yaml[16-53]
- scripts/generate-gitops-manifests.sh[41-55]
- docs/PROVIDERS.md[1-40]

## Suggested changes
- Add explicit documentation for the notebooks pgvector dependency (required env vars, whether TLS is supported/required, and how it’s provisioned in non-compose deployments).
- Consider making the notebooks provider conditional (e.g., only enable/configure it when `PGVECTOR_HOST` is set), or set safer defaults that do not assume a service name exists in every environment.
- If this repo is expected to generate complete GitOps artifacts, add a companion manifest/chart values guidance for provisioning a pgvector-capable Postgres service, or clearly document that it is an external prerequisite.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread compose/compose.yaml Outdated

@Jdubrick Jdubrick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to force the usage of pgvector for all uses of Lightspeed or do we just want this to go on the rolling demo / dev environment for now? I ask because if we change it in the config.yaml instead of in the https://github.com/redhat-ai-dev/lightspeed-configs/blob/main/.github/workflows/sync-gitops.yml workflow then these changes are going to be required in the Operator, Helm and Local environments too. If we just want it on the cluster environment then we just have to update the sync to the environment, if we want it on all we can keep it in the config file but will need to have a discussion about the new requirement for a pgvector db on every install/deployment

@JslYoon

JslYoon commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

FAISS will still be used for lightspeed queries, pgvector will only be used for notebooks. What we can do for now is I can modify the sync-gitops.yml, test the changes on rolling-demo environment, and after confirming it working, can update the lightspeed-configs.

@JslYoon
JslYoon force-pushed the JslYoon-pgvector-enable branch 2 times, most recently from 5606a99 to 31a67d0 Compare July 22, 2026 21:43
@JslYoon
JslYoon requested a review from Jdubrick July 22, 2026 21:43
@Jdubrick

Copy link
Copy Markdown
Contributor

FAISS will still be used for lightspeed queries, pgvector will only be used for notebooks. What we can do for now is I can modify the sync-gitops.yml, test the changes on rolling-demo environment, and after confirming it working, can update the lightspeed-configs.

@JslYoon if we are using pgvector for notebooks and want that to extend to prod deployments we should just use pgvector for everything tbh and recommend no FAISS, possibly a parking lot topic today to discuss further? Since using pgvector in our actual product will introduce another container to manage

@Jdubrick Jdubrick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change looks good, I think this part: https://github.com/redhat-ai-dev/lightspeed-configs/blob/main/llama-stack-configs/config.yaml#L135-L137 is going to remain though, can we strip it from the yaml in the bash function you're adding just to keep the config clean?

Signed-off-by: Lucas <lyoon@redhat.com>
@JslYoon
JslYoon force-pushed the JslYoon-pgvector-enable branch from 31a67d0 to 31cb8f0 Compare July 23, 2026 16:42
@JslYoon
JslYoon requested a review from Jdubrick July 23, 2026 16:43
@JslYoon

JslYoon commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

fixed!

@Jdubrick Jdubrick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@JslYoon
JslYoon merged commit af97dfb into redhat-ai-dev:main Jul 23, 2026
3 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.

2 participants