feat: add test and fix python dependencies #304
Conversation
|
WalkthroughThe pull request introduces a new file containing end-to-end tests for resolving Python dependencies in projects that utilize various vector databases and tools. It modifies an existing utility function to accept an additional parameter for tools and updates dependency versions in another file. Additionally, a new job is added to the GitHub Actions workflow to automate the testing of Python dependencies across different Python versions. Changes
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
e2e/resolve_python_dependencies.spec.ts (1)
27-27: Address the TODO: Add data sources to the testThe test currently lacks data sources, as indicated by the TODO comment. Incorporating data sources will enhance test coverage and ensure that dependencies related to data sources are properly tested.
Would you like assistance in adding data sources to the tests or should I open a new GitHub issue to track this task?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- e2e/resolve_python_dependencies.spec.ts (1 hunks)
- e2e/utils.ts (2 hunks)
- helpers/python.ts (5 hunks)
Additional comments not posted (9)
e2e/utils.ts (2)
35-35: LGTM!The addition of the optional
toolsparameter to therunCreateLlamafunction is a good change. It provides flexibility to specify tools when needed, without breaking existing calls to the function. The type of the parameter is also appropriate and consistent with the intended usage.
69-69: LGTM!The change to the
--toolsoption in the command-line arguments is consistent with the addition of the optionaltoolsparameter to therunCreateLlamafunction. It allows the option to be dynamically set based on the provided value, or defaults to"none"if not provided, using the nullish coalescing operator. This change maintains the expected behavior and is implemented correctly.helpers/python.ts (7)
39-39: LGTM!The minor version update of
llama-index-vector-stores-mongodbfrom^0.1.3to^0.3.1should introduce new features and bug fixes while maintaining backward compatibility.
46-46: LGTM!The minor version update of
llama-index-vector-stores-postgresfrom^0.1.1to^0.2.5should introduce new features and bug fixes while maintaining backward compatibility.
60-60: LGTM!The minor version update of
llama-index-vector-stores-milvusfrom^0.1.20to^0.2.0should introduce new features and bug fixes while maintaining backward compatibility.
71-71: LGTM!The minor version update of
llama-index-vector-stores-astra-dbfrom^0.1.5to^0.2.0should introduce new features and bug fixes while maintaining backward compatibility.
85-85: LGTM!The minor version update of
llama-index-vector-stores-chromafrom^0.1.8to^0.2.0should introduce new features and bug fixes while maintaining backward compatibility.
92-92: LGTM!The minor version update of
llama-index-vector-stores-weaviatefrom^1.0.2to^1.1.1should introduce new features and bug fixes while maintaining backward compatibility.
133-133: LGTM!The patch version update of
llama-index-indices-managed-llama-cloudfrom^0.3.0to^0.3.1should introduce bug fixes and performance improvements while maintaining backward compatibility.
| if (vectorDb !== "none") { | ||
| if (vectorDb === "pg") { | ||
| expect(pyprojectContent).toContain( | ||
| "llama-index-vector-stores-postgres", | ||
| ); | ||
| } else { | ||
| expect(pyprojectContent).toContain( | ||
| `llama-index-vector-stores-${vectorDb}`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor to use a mapping for vectorDb dependencies
To enhance maintainability and scalability, consider using a mapping object to relate vectorDb values to their corresponding package names. This approach makes it easier to handle special cases and add new vector databases in the future.
Refactor the code as follows:
const vectorDbDependencies: { [key: string]: string } = {
pg: "llama-index-vector-stores-postgres",
};
const expectedDependency =
vectorDbDependencies[vectorDb] || `llama-index-vector-stores-${vectorDb}`;
expect(pyprojectContent).toContain(expectedDependency);Remove unnecessary check for vectorDb !== "none"
The vectorDbs array does not contain "none", so the condition if (vectorDb !== "none") is unnecessary. Removing this condition simplifies the code and improves readability.
Apply this diff to remove the unnecessary condition:
const pyprojectContent = fs.readFileSync(pyprojectPath, "utf-8");
-if (vectorDb !== "none") {
if (vectorDb === "pg") {
expect(pyprojectContent).toContain(
"llama-index-vector-stores-postgres",
);
} else {
expect(pyprojectContent).toContain(
`llama-index-vector-stores-${vectorDb}`,
);
}
-}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.
| if (vectorDb !== "none") { | |
| if (vectorDb === "pg") { | |
| expect(pyprojectContent).toContain( | |
| "llama-index-vector-stores-postgres", | |
| ); | |
| } else { | |
| expect(pyprojectContent).toContain( | |
| `llama-index-vector-stores-${vectorDb}`, | |
| ); | |
| } | |
| } | |
| if (vectorDb === "pg") { | |
| expect(pyprojectContent).toContain( | |
| "llama-index-vector-stores-postgres", | |
| ); | |
| } else { | |
| expect(pyprojectContent).toContain( | |
| `llama-index-vector-stores-${vectorDb}`, | |
| ); | |
| } |
| if (tool !== "none") { | ||
| if (tool === "wikipedia.WikipediaToolSpec") { | ||
| expect(pyprojectContent).toContain("wikipedia"); | ||
| } | ||
| if (tool === "google.GoogleSearchToolSpec") { | ||
| expect(pyprojectContent).toContain("google"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor to use a mapping for tool dependencies
Using a mapping object for tools and their required dependencies improves code clarity and makes it easier to manage additional tools in the future.
Refactor the code as follows:
const toolDependencies: { [key: string]: string } = {
"wikipedia.WikipediaToolSpec": "wikipedia",
"google.GoogleSearchToolSpec": "google",
};
const expectedDependency = toolDependencies[tool];
if (expectedDependency) {
expect(pyprojectContent).toContain(expectedDependency);
}Remove unnecessary check for tool !== "none"
The toolOptions array does not contain "none", so the condition if (tool !== "none") is unnecessary. Eliminating this condition streamlines the code.
Apply this diff to remove the unnecessary condition:
if (tool === "wikipedia.WikipediaToolSpec") {
expect(pyprojectContent).toContain("wikipedia");
}
if (tool === "google.GoogleSearchToolSpec") {
expect(pyprojectContent).toContain("google");
}
-}Committable suggestion was skipped due to low confidence.
| const { stdout, stderr } = await execAsync( | ||
| // Config poetry to create virtualenv in project directory. | ||
| // so that we can easily prune the e2e cache to avoid overloading the storage. | ||
| "poetry config virtualenvs.in-project true && poetry lock --no-update", | ||
| { | ||
| cwd: path.join(cwd, name), | ||
| }, | ||
| ); | ||
| console.log("poetry lock stdout:", stdout); | ||
| console.error("poetry lock stderr:", stderr); | ||
| } catch (error) { | ||
| console.error("Error running poetry lock:", error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Handle errors from execAsync more explicitly
Currently, the catch block logs the error but rethrows it without additional context. Providing more detailed error handling can aid in debugging when poetry lock fails.
Consider enhancing the error handling as follows:
} catch (error) {
console.error("Error running poetry lock:", error);
+ throw new Error(`Poetry lock failed for ${optionDescription}: ${error.message}`);
}This change adds context to the error message, indicating which combination of vectorDb and tool caused the failure.
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.
| const { stdout, stderr } = await execAsync( | |
| // Config poetry to create virtualenv in project directory. | |
| // so that we can easily prune the e2e cache to avoid overloading the storage. | |
| "poetry config virtualenvs.in-project true && poetry lock --no-update", | |
| { | |
| cwd: path.join(cwd, name), | |
| }, | |
| ); | |
| console.log("poetry lock stdout:", stdout); | |
| console.error("poetry lock stderr:", stderr); | |
| } catch (error) { | |
| console.error("Error running poetry lock:", error); | |
| throw error; | |
| } | |
| const { stdout, stderr } = await execAsync( | |
| // Config poetry to create virtualenv in project directory. | |
| // so that we can easily prune the e2e cache to avoid overloading the storage. | |
| "poetry config virtualenvs.in-project true && poetry lock --no-update", | |
| { | |
| cwd: path.join(cwd, name), | |
| }, | |
| ); | |
| console.log("poetry lock stdout:", stdout); | |
| console.error("poetry lock stderr:", stderr); | |
| } catch (error) { | |
| console.error("Error running poetry lock:", error); | |
| throw new Error(`Poetry lock failed for ${optionDescription}: ${error.message}`); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
.github/workflows/e2e.yml (1)
106-110: Consider using the same Node.js versions as thee2ejob for consistency.The Node.js version is hardcoded to 18, which may not be consistent with the versions used in the
e2ejob. Consider using a matrix strategy or environment variable to specify the Node.js version for consistency across the workflow.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/e2e.yml (1 hunks)
Additional comments not posted (9)
.github/workflows/e2e.yml (9)
92-92: LGTM!Using
actions/checkout@v4is a standard practice to check out the code in GitHub Actions workflows.
94-97: LGTM!Using
actions/setup-python@v5is a standard practice to set up Python in GitHub Actions workflows. The Python version is correctly specified from the matrix.
99-102: LGTM!Using
snok/install-poetry@v1is a good practice to install Poetry in GitHub Actions workflows. The Poetry version is correctly specified from the environment variable.
112-113: LGTM!Using
pnpm installis a standard practice to install dependencies in GitHub Actions workflows.
115-117: LGTM!Using
pnpm exec playwright install --with-depsis a standard practice to install Playwright browsers in GitHub Actions workflows.
119-121: LGTM!Using
pnpm run buildis a standard practice to build the project in GitHub Actions workflows.
123-125: LGTM!Using
pnpm run pack-installis a project-specific command to install the project in GitHub Actions workflows.
127-132: LGTM!Using
pnpm exec playwright testis a standard practice to run Playwright tests in GitHub Actions workflows. The test filee2e/resolve_python_dependencies.spec.tsis specific to testing Python dependencies, and the necessary environment variables for API keys are correctly specified.
134-139: LGTM!Using
actions/upload-artifact@v3is a standard practice to upload artifacts in GitHub Actions workflows. The Playwright report is correctly specified as the artifact to upload, and the retention period of 30 days is a reasonable default value.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
templates/types/streaming/fastapi/pyproject.toml (1)
12-12: Consider the impact on future Python compatibility.The change in the Python version constraint from
^3.11,<4.0to>=3.11,<3.13aligns the project with the current stable Python versions (3.11 and 3.12). This is a good practice to ensure compatibility with the latest Python releases.However, please consider the project's future Python compatibility goals. If the project intends to support future Python versions (3.13 and above), it might be beneficial to keep a wider version range. This can be achieved by using a constraint like
>=3.11,<4.0, which allows for Python 3.11 and above, but not 4.0 and above.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- helpers/python.ts (3 hunks)
- package.json (1 hunks)
- templates/types/multiagent/fastapi/pyproject.toml (1 hunks)
- templates/types/streaming/fastapi/pyproject.toml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- helpers/python.ts
Additional comments not posted (3)
templates/types/multiagent/fastapi/pyproject.toml (1)
13-13: LGTM! Verify compatibility with the updated Python and dependency versions.The changes to the Python version and dependency specifications look good. They allow for more flexibility in the runtime environment and ensure compatibility with the project's requirements.
Please ensure that the project has been thoroughly tested with the updated Python versions (3.11 and 3.12) and the specified version range of the
llama-index-agent-openaidependency (0.3.0 to 0.3.x) to avoid any potential compatibility issues.package.json (2)
27-27: LGTM!The change to the
e2escript command is valid and aligns with the PR objective of fixing Python dependencies. The use of the--grep-invertoption effectively excludes tests matching the pattern'resolve_python_dependencies'from thee2etest run, allowing for better isolation and targeting of specific test scenarios.
28-28: LGTM!The introduction of the
e2e:python-depsscript command is a good addition and aligns with the PR objective of fixing Python dependencies. The use of the--grepoption allows for targeted execution of tests that match the'resolve_python_dependencies'pattern, enabling better focus and isolation of Python dependency-related tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- e2e/resolve_python_dependencies.spec.ts (1 hunks)
- e2e/utils.ts (3 hunks)
- index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/resolve_python_dependencies.spec.ts
Additional comments not posted (6)
e2e/utils.ts (4)
45-45: LGTM!The change is a part of a larger refactor to handle different data source types more effectively. The change itself does not introduce any apparent issues.
50-60: LGTM!The new logic aligns with the PR objective to handle different data source types more effectively. The code segment correctly handles the three possible cases and does not introduce any apparent issues.
69-69: LGTM!The change aligns with the refactor to handle different data source types more effectively and allows for more flexible input handling. The change should improve the handling of different data source types.
82-82: LGTM!The change aligns with the addition of the new optional
toolsparameter to therunCreateLlamafunction and improves the flexibility by allowing the caller to specify the tools to be used. The default value of"none"ensures backward compatibility.index.ts (2)
93-106: LGTM!The addition of the
--web-sourceand--db-sourcecommand-line options is a great enhancement to support different types of data sources. The descriptions provided for these options are clear and informative.
232-252: Looks good!The code segment correctly handles the logic for the newly added
--web-sourceand--db-sourceoptions. It populates theprogram.dataSourcesarray with the appropriate data source configuration based on the detected option.The configuration objects include relevant details such as the data source type, connection details, and default query (for the database option), ensuring that the selected data source is properly configured and ready to be used by the program.
| @@ -32,6 +32,7 @@ export async function runCreateLlama( | |||
| appType?: AppType, | |||
| llamaCloudProjectName?: string, | |||
| llamaCloudIndexName?: string, | |||
| tools?: string, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Function runCreateLlama is still called without the tools argument.
Please update all calls to runCreateLlama to include the tools parameter to maintain consistency and prevent potential issues.
e2e/resolve_python_dependencies.spec.tse2e/streaming_template.spec.tse2e/multiagent_template.spec.tse2e/extractor_template.spec.ts
Analysis chain
LGTM! Verify the function usage across the codebase.
The addition of the optional tools parameter aligns with the PR objective and maintains backward compatibility. Please ensure that all existing calls to this function have been updated to provide the tools argument where necessary.
Run the following script to verify the function usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `runCreateLlama` provide the `tools` argument where necessary.
# Test: Search for the function usage. Expect: No occurrences of the old signature.
rg --type typescript -A 5 $'runCreateLlama\('
Length of output: 81
Script:
#!/bin/bash
# Description: Verify all function calls to `runCreateLlama` provide the `tools` argument where necessary.
# Test: Search for the function usage. Expect: No occurrences of the old signature.
rg --type ts -A 5 'runCreateLlama\('
Length of output: 1664
Summary by CodeRabbit
New Features
Bug Fixes