-
Notifications
You must be signed in to change notification settings - Fork 520
fix: optimize Neo4j Community Edition support and enhance MCP environment loading #754
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,14 +16,88 @@ | |||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def load_default_config(user_id="default_user"): | ||||||||||||
| """ | ||||||||||||
| Load MOS configuration from environment variables. | ||||||||||||
|
|
||||||||||||
| IMPORTANT for Neo4j Community Edition: | ||||||||||||
| Community Edition does not support administrative commands like 'CREATE DATABASE'. | ||||||||||||
| To avoid errors, ensure the following environment variables are set correctly: | ||||||||||||
| - NEO4J_DB_NAME=neo4j (Must use the default database) | ||||||||||||
| - NEO4J_AUTO_CREATE=false (Disable automatic database creation) | ||||||||||||
| - NEO4J_USE_MULTI_DB=false (Disable multi-tenant database mode) | ||||||||||||
| """ | ||||||||||||
| # Define mapping between environment variables and configuration parameters | ||||||||||||
| # We support both clean names and MOS_ prefixed names for compatibility | ||||||||||||
| env_mapping = { | ||||||||||||
| "OPENAI_API_KEY": "openai_api_key", | ||||||||||||
| "OPENAI_API_BASE": "openai_api_base", | ||||||||||||
| "MOS_TEXT_MEM_TYPE": "text_mem_type", | ||||||||||||
| "NEO4J_URI": "neo4j_uri", | ||||||||||||
| "NEO4J_USER": "neo4j_user", | ||||||||||||
| "NEO4J_PASSWORD": "neo4j_password", | ||||||||||||
| "NEO4J_DB_NAME": "neo4j_db_name", | ||||||||||||
| "NEO4J_AUTO_CREATE": "neo4j_auto_create", | ||||||||||||
| "NEO4J_USE_MULTI_DB": "use_multi_db", | ||||||||||||
| "MOS_NEO4J_SHARED_DB": "mos_shared_db", # Special handle later | ||||||||||||
| "MODEL_NAME": "model_name", | ||||||||||||
| "MOS_CHAT_MODEL": "model_name", | ||||||||||||
| "EMBEDDER_MODEL": "embedder_model", | ||||||||||||
| "MOS_EMBEDDER_MODEL": "embedder_model", | ||||||||||||
| "CHUNK_SIZE": "chunk_size", | ||||||||||||
| "CHUNK_OVERLAP": "chunk_overlap", | ||||||||||||
| "ENABLE_MEM_SCHEDULER": "enable_mem_scheduler", | ||||||||||||
| "MOS_ENABLE_SCHEDULER": "enable_mem_scheduler", | ||||||||||||
| "ENABLE_ACTIVATION_MEMORY": "enable_activation_memory", | ||||||||||||
| "TEMPERATURE": "temperature", | ||||||||||||
| "MOS_CHAT_TEMPERATURE": "temperature", | ||||||||||||
| "MAX_TOKENS": "max_tokens", | ||||||||||||
| "MOS_MAX_TOKENS": "max_tokens", | ||||||||||||
| "TOP_P": "top_p", | ||||||||||||
| "MOS_TOP_P": "top_p", | ||||||||||||
| "TOP_K": "top_k", | ||||||||||||
| "MOS_TOP_K": "top_k", | ||||||||||||
| "SCHEDULER_TOP_K": "scheduler_top_k", | ||||||||||||
| "MOS_SCHEDULER_TOP_K": "scheduler_top_k", | ||||||||||||
| "SCHEDULER_TOP_N": "scheduler_top_n", | ||||||||||||
|
||||||||||||
| "SCHEDULER_TOP_N": "scheduler_top_n", | |
| "SCHEDULER_TOP_N": "scheduler_top_n", | |
| "MOS_SCHEDULER_TOP_N": "scheduler_top_n", |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The env_mapping dictionary contains duplicate entries that map to the same parameter. For example, both MODEL_NAME and MOS_CHAT_MODEL map to "model_name" (lines 42-43). If both environment variables are set, the later one (MOS_CHAT_MODEL) will silently overwrite the earlier one (MODEL_NAME), which may lead to unexpected behavior. The comment states this is for "compatibility", but the implementation doesn't properly handle precedence when both are present.
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quote stripping logic doesn't handle single character values properly. If an environment variable is set to a single quote character (val = "'") or a single double quote character (val = '"'), the conditions will pass but val[1:-1] will result in an empty string, which may not be the intended behavior. While this is an edge case, it could cause silent failures.
| if (val.startswith('"') and val.endswith('"')) or ( | |
| val.startswith("'") and val.endswith("'") | |
| if len(val) > 1 and ( | |
| (val.startswith('"') and val.endswith('"')) | |
| or (val.startswith("'") and val.endswith("'")) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The numeric type conversion logic attempts to convert all non-boolean string values to numbers. This will incorrectly convert string values that happen to be numeric but should remain strings. For example, if someone sets NEO4J_DB_NAME="123" or NEO4J_PASSWORD="456", these will be converted to integers instead of remaining as strings.
The conversion should only be attempted for parameters where numeric values are expected (e.g., chunk_size, chunk_overlap, max_tokens, etc.), not for all parameters universally.
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling load_default_config with a user_id parameter in the register_cube function, the user_id might be None. The load_default_config function will then create a configuration with user_id=None, which gets passed to kwargs["user_id"] at line 64. This could lead to unexpected behavior if the downstream functions don't handle None user_id properly.
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return message for cube registration is inconsistent. When a new cube is created (path doesn't exist), it returns cube_id or cube (the object), but when registering an existing cube, it returns cube_id or cube_name_or_path (the string path). If cube_id is None and a new cube was created, this will return a string representation of the cube object instead of a meaningful identifier.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -181,15 +181,22 @@ def get_default_cube_config( | |||||
| # Configure text memory based on type | ||||||
| if text_mem_type == "tree_text": | ||||||
| # Tree text memory requires Neo4j configuration | ||||||
| # NOTE: Neo4j Community Edition does NOT support multiple databases. | ||||||
| # It only has one default database named 'neo4j'. | ||||||
| # If you are using Community Edition: | ||||||
| # 1. Set 'use_multi_db' to False (default) | ||||||
| # 2. Set 'db_name' to 'neo4j' (default) | ||||||
| # 3. Set 'auto_create' to False to avoid 'CREATE DATABASE' permission errors. | ||||||
| db_name = f"memos{user_id.replace('-', '').replace('_', '')}" | ||||||
| if not kwargs.get("use_multi_db", False): | ||||||
| db_name = kwargs.get("neo4j_db_name", "defaultdb") | ||||||
| db_name = kwargs.get("neo4j_db_name", "neo4j") | ||||||
|
|
||||||
| neo4j_config = { | ||||||
| "uri": kwargs.get("neo4j_uri", "bolt://localhost:7687"), | ||||||
| "user": kwargs.get("neo4j_user", "neo4j"), | ||||||
| "db_name": db_name, | ||||||
| "password": kwargs.get("neo4j_password", "12345678"), | ||||||
| "auto_create": True, | ||||||
| "auto_create": kwargs.get("neo4j_auto_create", True), | ||||||
|
||||||
| "auto_create": kwargs.get("neo4j_auto_create", True), | |
| "auto_create": kwargs.get("neo4j_auto_create", False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration has both MOS_NEO4J_SHARED_DB=true and NEO4J_USE_MULTI_DB=false, which according to the comment have "inverse logic". Setting MOS_NEO4J_SHARED_DB=true means users share one DB (no multi-db), and NEO4J_USE_MULTI_DB=false also means no multi-db. However, if someone changes one without changing the other, they could end up with conflicting settings. The example should either use only one of these variables or include a comment warning about keeping them synchronized.