chore: simplify local index code #537
Conversation
…storage directory. This simplifies the code and improves readability.
🦋 Changeset detectedLatest commit: ab1127c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request removes the Changes
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
Documentation and Community
|
…ings. Updated imports and initialization to support UI configuration, including enabling/disabling the UI and setting starter questions. Adjusted tests to validate UI configuration functionality.
leehuwuj
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
llama-index-server/llama_index/server/__init__.py(1 hunks)llama-index-server/llama_index/server/server.py(5 hunks)llama-index-server/tests/test_llamaindex_server.py(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
llama-index-server/llama_index/server/__init__.py (1)
llama-index-server/llama_index/server/server.py (2)
LlamaIndexServer(39-197)UIConfig(17-36)
🪛 GitHub Actions: Build Package
llama-index-server/tests/test_llamaindex_server.py
[error] 100-100: KeyError: 'TITLE' in test_ui_is_downloaded. The expected key 'TITLE' was not found in the config JSON.
[error] 190-190: KeyError: 'TITLE' in test_ui_config_from_dict. The expected key 'TITLE' was not found in the config JSON.
⏰ Context from checks skipped due to timeout of 90000ms (27)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --no-files)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --no-files)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --llamacloud)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --example-file)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --no-files)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --no-files)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --llamacloud)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --example-file)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --no-files)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --llamacloud)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --example-file)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --no-files)
🔇 Additional comments (15)
llama-index-server/llama_index/server/__init__.py (2)
1-1: LGTM: Proper import statement with UIConfig.The import statement now includes the new
UIConfigclass along withLlamaIndexServer, which is consistent with the objective of making the UI configuration more accessible.
3-3: LGTM:UIConfigcorrectly added to__all__.Adding
UIConfigto the__all__list makes it part of the public API, allowing users to import it directly fromllama_index.server. This is consistent with the simplified approach of managing UI configuration.llama-index-server/tests/test_llamaindex_server.py (4)
1-4: LGTM: Added necessary imports for enhanced testing.The additional imports (
json,os,shutil) and the updated import forUIConfigare appropriate for the new test functionality.Also applies to: 10-10
43-43: LGTM: Improved route checking method.Changed the route check from direct path comparison to string-based search, which is more resilient to route representation changes.
69-80: LGTM: Updated server configuration to use UIConfig.Properly configured the test to use the new
UIConfigclass, ensuring that UI features are properly tested with the new configuration approach.
130-153: LGTM: Good test for UI config customization.This test verifies that custom UI configurations are properly applied, ensuring robust UI customization capabilities.
llama-index-server/llama_index/server/server.py (9)
4-4: LGTM: Updated imports to support new features.Added necessary imports for
UnionandBaseModelto support the newUIConfigclass.Also applies to: 9-10
17-37: LGTM: Well-structured UIConfig class.The new
UIConfigclass is well-designed with appropriate defaults and a clear method for generating configuration content. The encapsulation of UI configuration settings in a Pydantic model is a good practice.
42-42: LGTM: Added UI configuration attribute.Added
ui_configattribute with the proper type annotation.
50-50: LGTM: Updated method signature and documentation.The constructor parameter and its documentation have been updated to use the new
UIConfigclass.Also applies to: 65-65
76-81: LGTM: Robust initialization logic for UI configuration.The initialization logic handles three cases gracefully:
- No configuration provided (creates default)
- Dictionary provided (converts to UIConfig)
- UIConfig instance provided (uses as-is)
This provides flexibility for users while maintaining type safety.
94-97: LGTM: Updated UI enabling logic.Updated the conditional checks to use the new
ui_config.enabledproperty, maintaining the same behavior in a more structured way.
99-99: LGTM: Updated mount UI condition.Updated the condition to use the new
ui_config.enabledproperty consistently.
125-133: LGTM: Updated mount UI implementation.The updated implementation uses the new
ui_configproperties consistently throughout the method, removing redundancy.
141-147: LGTM: Updated UI config override method.The method now uses the
ui_configproperties andget_config_contentmethod for a more encapsulated approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
llama-index-server/llama_index/server/server.py (2)
94-97: Redundant UI enabling logic could be simplified.The logic for setting the
enabledattribute ofui_configis complex with multipleis Nonechecks. Since theenabledattribute has a default value ofTruein theUIConfigclass, it will never beNoneunless explicitly set.Consider simplifying the logic:
- if str(env).lower() == "dev": - self.allow_cors("*") - if self.ui_config.enabled is None: - self.ui_config.enabled = True - if self.ui_config.enabled is None: - self.ui_config.enabled = False + if str(env).lower() == "dev": + self.allow_cors("*") + # In dev environment, ensure UI is enabled unless explicitly disabled + if not isinstance(self.ui_config.enabled, bool): + self.ui_config.enabled = True + # In non-dev environments, UI is disabled by default unless explicitly enabled + elif not isinstance(self.ui_config.enabled, bool): + self.ui_config.enabled = False
125-125: Redundant UI enabled check.This check is redundant since the method
mount_uiis only called ifself.ui_config.enabledis truthy (Line 99).- if self.ui_config.enabled: - if not os.path.exists(self.ui_config.ui_path): + if not os.path.exists(self.ui_config.ui_path):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
llama-index-server/llama_index/server/server.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --no-files)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --no-files)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --llamacloud)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --no-files)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --example-file)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --no-files)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --llamacloud)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --example-file)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --no-files)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --llamacloud)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --example-file)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --example-file)
- GitHub Check: Unit Tests (windows-latest, 3.9)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --no-files)
🔇 Additional comments (4)
llama-index-server/llama_index/server/server.py (4)
17-37: Well-structured UI configuration class!The
UIConfigclass nicely encapsulates UI-related settings using Pydantic, which provides automatic validation and type checking. The default values make the configuration easy to use out of the box.
42-42: Good refactoring of UI configuration into a single attribute.This is in line with the suggestion from the previous review comment to use a single parameter for UI configuration.
50-50: Nice implementation of flexible UI configuration initialization.The ability to accept either a
UIConfiginstance or a dictionary provides flexibility for users while maintaining type safety. The initialization logic properly handles all cases.Also applies to: 76-81
146-146: Clean UI config content generation!The code uses the
get_config_contentmethod of theUIConfigclass, which is a good approach for encapsulating this functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
llama-index-server/README.md (2)
46-50: UI Configuration in Quick Start Example:
The example instantiation ofLlamaIndexServernow uses aui_configdictionary—which is great for consolidating UI settings. The provided keys (app_titleandstarter_questions) are clear. Consider adding a brief comment or annotation noting that additional options (e.g.,enabledandui_path) exist for further customization.
69-69: Outdated Parameter Reference in Documentation:
In the Quick Start instructions, the text still refers to settinginclude_ui=True. Since this parameter was replaced byui_config(with itsenabledoption), please update this reference to avoid confusion. For example, you might suggest:- Access the chat UI at `http://localhost:8000/` (Make sure you set the `env="dev"` or `include_ui=True` in the server configuration) + Access the chat UI at `http://localhost:8000/` (Make sure you set the `env="dev"` or configure `ui_config` with `enabled: True` in the server configuration)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
llama-index-server/README.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --no-files)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --no-files)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --example-file)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --llamacloud)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --no-files)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --llamacloud)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --example-file)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --no-files)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --example-file)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --llamacloud)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --example-file)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --no-files)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --example-file)
🔇 Additional comments (1)
llama-index-server/README.md (1)
79-83: Updated UIConfig Documentation:
The configuration options section now clearly outlines the structure of the newui_configparameter, listing keys such asenabled,app_title,starter_questions, andui_path. This update improves clarity. Please verify that the documented default values match the implementation, and consider adding example default values if they differ from what users might expect.
leehuwuj
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
templates/types/llamaindexserver/fastapi/app/index.py(1 hunks)templates/types/llamaindexserver/fastapi/generate.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`templates/**`: For files under the `templates` folder, do not report 'Missing Dependencies Detected' errors.
templates/**: For files under thetemplatesfolder, do not report 'Missing Dependencies Detected' errors.
templates/types/llamaindexserver/fastapi/app/index.pytemplates/types/llamaindexserver/fastapi/generate.py
🧬 Code Definitions (1)
templates/types/llamaindexserver/fastapi/generate.py (1)
templates/types/llamaindexserver/fastapi/app/settings.py (1)
init_settings(6-8)
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, ubuntu-22.04, nextjs, --no-files)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, windows-latest, nextjs, --no-files)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --llamacloud)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --example-file)
- GitHub Check: typescript (20, 3.11, macos-latest, nextjs, --no-files)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --llamacloud)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --example-file)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --llamacloud)
- GitHub Check: typescript (18, 3.11, ubuntu-22.04, nextjs, --no-files)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, ubuntu-22.04, fastapi, --no-files)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --example-file)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --llamacloud)
- GitHub Check: typescript (18, 3.11, windows-latest, nextjs, --no-files)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --example-file)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --llamacloud)
- GitHub Check: python (20, 3.11, windows-latest, fastapi, --no-files)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --example-file)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --llamacloud)
- GitHub Check: typescript (18, 3.11, macos-latest, nextjs, --no-files)
- GitHub Check: Unit Tests (windows-latest, 3.9)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --example-file)
- GitHub Check: python (20, 3.11, macos-latest, fastapi, --no-files)
🔇 Additional comments (3)
templates/types/llamaindexserver/fastapi/generate.py (1)
4-4: Good job on simplifying the import!Importing the constant
STORAGE_DIRfromapp.indexmakes the code more consistent and simplifies maintenance.templates/types/llamaindexserver/fastapi/app/index.py (2)
11-11: Good simplification with STORAGE_DIR constant.Using a constant instead of a class with a single attribute simplifies the code while maintaining the functionality. This aligns well with the PR objective to simplify the local index code.
16-16: Successfully refactored to use STORAGE_DIR throughout the code.You've consistently replaced all references to the previous
IndexConfigclass with the newSTORAGE_DIRconstant, ensuring the code remains functional and easier to understand.While this change reduces the flexibility of configuring the storage directory at runtime, it aligns with the goal of simplifying the code and makes the behavior more predictable.
Also applies to: 19-23
Summary by CodeRabbit