Skip to content

fix: add dependencies for pg vector store #312

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

Merged
merged 10 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/strong-wasps-nail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"create-llama": patch
---

Add dependencies for PGVectorStore
6 changes: 6 additions & 0 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# coderabbit.yml
reviews:
path_instructions:
- path: "templates/**"
instructions: |
For files under the `templates` folder, do not report 'Missing Dependencies Detected' errors.
75 changes: 75 additions & 0 deletions e2e/resolve_ts_dependencies.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { expect, test } from "@playwright/test";
import { exec } from "child_process";
import fs from "fs";
import path from "path";
import util from "util";
import { TemplateFramework, TemplateVectorDB } from "../helpers/types";
import { createTestDir, runCreateLlama } from "./utils";

const execAsync = util.promisify(exec);

const templateFramework: TemplateFramework = process.env.FRAMEWORK
? (process.env.FRAMEWORK as TemplateFramework)
: "nextjs";
const dataSource: string = process.env.DATASOURCE
? process.env.DATASOURCE
: "--example-file";

if (
templateFramework == "nextjs" ||
templateFramework == "express" // test is only relevant for TS projects
) {
// vectorDBs combinations to test
const vectorDbs: TemplateVectorDB[] = [
"mongo",
"pg",
"pinecone",
"milvus",
"astra",
"qdrant",
"chroma",
"llamacloud",
"weaviate",
];

test.describe("Test resolve TS dependencies", () => {
for (const vectorDb of vectorDbs) {
const optionDescription = `vectorDb: ${vectorDb}, dataSource: ${dataSource}`;

test(`options: ${optionDescription}`, async () => {
const cwd = await createTestDir();

const result = await runCreateLlama(
cwd,
"streaming",
templateFramework,
dataSource,
vectorDb,
3000, // port
8000, // externalPort
"dependencies", // postInstallAction
);
const name = result.projectName;

// Check if the app folder exists
const appDir = path.join(cwd, name);
const dirExists = fs.existsSync(appDir);
expect(dirExists).toBeTruthy();

// Run tsc -b --diagnostics and capture the output
const { stdout, stderr } = await execAsync(
"pnpm exec tsc -b --diagnostics",
{
cwd: appDir,
},
);
Copy link

Choose a reason for hiding this comment

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

⚠️ Issue

Handle execAsync rejections to properly catch TypeScript compilation errors

Currently, if the TypeScript compilation fails due to errors, execAsync will reject the promise and throw an exception. This can cause the test to fail unexpectedly without providing clear feedback on the compilation errors.

Consider wrapping the execAsync call in a try...catch block to handle exceptions and provide informative error messages or assertions.

Apply this diff to handle the error appropriately:

+try {
  const { stdout, stderr } = await execAsync(
    "pnpm exec tsc -b --diagnostics",
    {
      cwd: appDir,
    },
  );
+  // Ensure that there are no compilation errors
+  expect(stdout).not.toMatch(/error TS\d{4}:/);
+} catch (error) {
+  // Handle the error thrown by execAsync
+  console.error('TypeScript compilation failed:', error);
+  throw error; // Re-throw the error to fail the test
+}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { stdout, stderr } = await execAsync(
"pnpm exec tsc -b --diagnostics",
{
cwd: appDir,
},
);
try {
const { stdout, stderr } = await execAsync(
"pnpm exec tsc -b --diagnostics",
{
cwd: appDir,
},
);
// Ensure that there are no compilation errors
expect(stdout).not.toMatch(/error TS\d{4}:/);
} catch (error) {
// Handle the error thrown by execAsync
console.error('TypeScript compilation failed:', error);
throw error; // Re-throw the error to fail the test
}


// Check if there's any error output
expect(stderr).toBeFalsy();
Copy link

Choose a reason for hiding this comment

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

⚠️ Issue

Check stdout for TypeScript errors instead of stderr

The TypeScript compiler outputs compilation errors to stdout, not stderr. Therefore, checking stderr may not detect compilation errors. Instead, you should inspect stdout for error messages or check the process exit code.

Apply this diff to adjust the assertion:

-// Check if there's any error output
-expect(stderr).toBeFalsy();
+// Ensure that there are no compilation errors
+expect(stdout).not.toMatch(/error TS\d{4}:/);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Check if there's any error output
expect(stderr).toBeFalsy();
// Ensure that there are no compilation errors
expect(stdout).not.toMatch(/error TS\d{4}:/);


// Log the stdout for debugging purposes
console.log("TypeScript type-check output:", stdout);
});
}
});
}
17 changes: 16 additions & 1 deletion helpers/typescript.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ export const installTSTemplate = async ({
framework,
ui,
observability,
vectorDb,
});

if (postInstallAction === "runApp" || postInstallAction === "dependencies") {
Expand All @@ -200,9 +201,16 @@ async function updatePackageJson({
framework,
ui,
observability,
vectorDb,
}: Pick<
InstallTemplateArgs,
"root" | "appName" | "dataSources" | "framework" | "ui" | "observability"
| "root"
| "appName"
| "dataSources"
| "framework"
| "ui"
| "observability"
| "vectorDb"
> & {
relativeEngineDestPath: string;
}): Promise<any> {
Expand Down Expand Up @@ -249,6 +257,13 @@ async function updatePackageJson({
};
}

if (vectorDb === "pg") {
packageJson.dependencies = {
...packageJson.dependencies,
pg: "^8.12.0",
};
}

if (observability === "traceloop") {
packageJson.dependencies = {
...packageJson.dependencies,
Expand Down
7 changes: 5 additions & 2 deletions templates/components/vectordbs/typescript/pg/generate.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
/* eslint-disable turbo/no-undeclared-env-vars */
import * as dotenv from "dotenv";
import { VectorStoreIndex, storageContextFromDefaults } from "llamaindex";
import { PGVectorStore } from "llamaindex/storage/vectorStore/PGVectorStore";
import {
PGVectorStore,
VectorStoreIndex,
storageContextFromDefaults,
} from "llamaindex";
import { getDocuments } from "./loader";
import { initSettings } from "./settings";
import {
Expand Down
3 changes: 1 addition & 2 deletions templates/components/vectordbs/typescript/pg/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/* eslint-disable turbo/no-undeclared-env-vars */
import { VectorStoreIndex } from "llamaindex";
import { PGVectorStore } from "llamaindex/storage/vectorStore/PGVectorStore";
import { PGVectorStore, VectorStoreIndex } from "llamaindex";
import {
PGVECTOR_SCHEMA,
PGVECTOR_TABLE,
Expand Down
Loading