Add Library API to QueryWeaver #253
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a packaged Python library for QueryWeaver with synchronous and asynchronous clients, shared base logic, examples, docs, packaging config, and tests. Introduces async API, exports via package init, updates a server helper signature, and includes README updates and new documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client as QueryWeaverClient/AsyncQueryWeaverClient
participant Base as BaseQueryWeaverClient
participant Falkor as FalkorDB
participant Core as api.core.text2sql
rect rgb(245,248,255)
note over User,Client: Initialization
User->>Client: create_client(...)/create_async_client(...)
Client->>Base: __init__(falkordb_url, api keys, models)
Base->>Falkor: connect + ping
Falkor-->>Base: ok
Base-->>Client: configured
end
rect rgb(245,255,245)
note over User,Core: Load database
User->>Client: load_database(db_name, db_url)
Client->>Core: resolve loader + load schema (async)
Core-->>Client: progress -> success
Client-->>User: True
end
rect rgb(255,248,240)
note over User,Core: Text to SQL and Query
User->>Client: text_to_sql(db, text, ...)
Client->>Core: query_database(stream)
Core-->>Client: chunks (sql_query, ...)
Client-->>User: sql string
User->>Client: query(db, text, execute_sql=?)
Client->>Core: query_database(stream)
Core-->>Client: chunks (sql_query, analysis, results?, final_result)
Client-->>User: {sql_query, analysis, results?, error}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a comprehensive Python library API to QueryWeaver, enabling users to work directly from Python without running the FastAPI server. The implementation provides both synchronous and asynchronous clients for Text2SQL operations, complete with documentation, examples, and extensive testing.
Key Changes
- Created a full Python library interface with sync and async clients
- Added comprehensive documentation and usage examples
- Implemented extensive test coverage for library functionality
- Refactored existing FastAPI routes to use shared core modules for better code organization
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/queryweaver/init.py | Main package initialization with version info and exports |
| src/queryweaver/sync.py | Synchronous QueryWeaver client implementation |
| src/queryweaver/async_client.py | Asynchronous QueryWeaver client with context manager support |
| setup.py | Package configuration for library distribution |
| tests/test_library_api.py | Comprehensive unit tests for sync library API |
| tests/test_async_library_api.py | Comprehensive unit tests for async library API |
| tests/test_integration.py | Integration tests for library functionality |
| examples/library_usage.py | Detailed examples for synchronous usage |
| examples/async_library_usage.py | Detailed examples for asynchronous usage |
| docs/library-usage.md | Complete documentation for library usage |
| api/core/text2sql.py | Core text2sql functionality extracted from routes |
| api/core/schema_loader.py | Database schema loading functionality |
| api/core/errors.py | Custom exception classes |
| api/routes/graphs.py | Refactored to use shared core modules |
| api/routes/database.py | Simplified to use shared schema loader |
Files not reviewed (1)
- app/package-lock.json: Language not supported
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
setup.py (1)
50-52: Reconsider including the entire 'api' package in the library distribution.Including the entire 'api.core' package may expose internal server components that aren't meant for library usage. This could lead to namespace conflicts and increase package size unnecessarily. Consider explicitly listing only the modules needed for the library functionality.
Consider being more selective about which api modules to include:
- packages=find_packages(where="src", include=["queryweaver", "queryweaver.*"]) + - find_packages(include=["api.core", "api.core.*"]), - py_modules=["api.config"], + packages=find_packages(where="src", include=["queryweaver", "queryweaver.*"]) + + ["api.core.schema_loader", "api.core.errors"], # Only include necessary modules + py_modules=[], # Remove api.config if not needed for library
🧹 Nitpick comments (29)
setup.py (2)
8-9: Remove unused variable assignment.Line 8 creates an empty list that is immediately overwritten on line 10.
def read_requirements(): """Read requirements from Pipfile.""" - requirements = [] # Core dependencies needed for the library functionality requirements = [
17-17: Use PyPI release for graphiti-core instead of git URLgraphiti-core is published on PyPI (includes a [falkordb] extra and a graphiti-core-falkordb package); replace the git URL in setup.py (line 17) with a pinned PyPI requirement (e.g., graphiti-core[falkordb]== or graphiti-core-falkordb==) to avoid installs failing where git isn't available.
README.md (1)
114-124: Documentation section is duplicated.The Documentation section with the list of resources appears twice in the README - here and would presumably appear again later (though not shown in the hunks). This duplication should be removed.
Please remove one of the duplicate Documentation sections to avoid confusion.
tests/test_async_library_api.py (2)
52-55: Import at module level for better style.The
osimport should be at the top of the file with other imports, not inside the test method.import pytest import asyncio +import os from unittest.mock import patch, AsyncMock from queryweaver import AsyncQueryWeaverClient, create_async_client ... def test_init_without_api_key_raises_error(self, mock_falkordb): """Test that missing API key raises ValueError.""" # Clear any existing API keys - import os os.environ.pop("OPENAI_API_KEY", None) os.environ.pop("AZURE_API_KEY", None)
20-26: Consider removing unused fixture parameter.The
mock_falkordbparameter in theasync_clientfixture is passed but not used by AsyncQueryWeaverClient (it's already patched globally). While it ensures proper dependency ordering, consider if this dependency is necessary.If the mock_falkordb fixture is only ensuring FalkorDB is mocked, you might simplify by removing the unused parameter or documenting why the dependency is needed:
@pytest.fixture -def async_client(mock_falkordb): - """Fixture to create an AsyncQueryWeaverClient for testing.""" +def async_client(mock_falkordb): # mock_falkordb ensures DB is mocked + """Fixture to create an AsyncQueryWeaverClient for testing. + + Args: + mock_falkordb: Ensures FalkorDB is mocked before client creation. + """examples/library_usage.py (4)
22-30: Narrow overly-broad exception handling (BLE001).Catching
Exceptionhides actionable errors and triggers linters. Catch the specific exceptions the library raises.- except Exception as e: + except (ValueError, ConnectionError, RuntimeError) as e: print(f"Error loading database: {e}") ... - except Exception as e: + except (ValueError, ConnectionError, RuntimeError) as e: print(f"Error generating SQL: {e}") ... - except Exception as e: + except (ValueError, ConnectionError, RuntimeError) as e: print(f"Error executing query: {e}") ... - except Exception as e: + except (ValueError, ConnectionError, RuntimeError) as e: print(f"Failed to load {db_name}: {e}") ... - except Exception as e: + except (ValueError, ConnectionError, RuntimeError) as e: print(f"Error getting schema: {e}") ... - except Exception as e: + except (ValueError, ConnectionError, RuntimeError) as e: print(f"Error processing query {i+1}: {e}")Also applies to: 33-41, 43-55, 79-84, 147-151, 218-219
51-53: Avoid potential KeyError on optional analysis field.Use
.getto safely access nested fields.- if result['analysis']: - print(f"Explanation: {result['analysis']['explanation']}") + analysis = result.get('analysis') + if analysis: + explanation = analysis.get('explanation') + if explanation: + print(f"Explanation: {explanation}")
164-169: Confirm Azure model identifiers match deployments.Ensure
completion_modelandembedding_modelalign with your Azure deployment names (often deployment IDs, not raw model names). Consider moving these to env vars for parity with other examples.
239-240: Add final newline to satisfy linters (C0304).- print("and update the database URLs and API keys with your actual values.") + print("and update the database URLs and API keys with your actual values.") +docs/library-usage.md (1)
87-93: Document accepted FalkorDB URL schemes.Tests enforce
redis://orrediss://. Clarify this in the parameter docs.- - `falkordb_url` (str): Redis URL for FalkorDB connection + - `falkordb_url` (str): Redis URL for FalkorDB connection (supports `redis://` and `rediss://`)tests/test_integration.py (1)
74-95: Remove ad-hoc main runner; use pytest entry instead.Directly calling patched tests triggers Pylint E1120 and broad exception catches (BLE001). Replace with
pytest.main.-if __name__ == "__main__": - # Run tests - test_library_import() - print("✓ Library import test passed") - - test_client_initialization() - print("✓ Client initialization test passed") - - test_convenience_function() - print("✓ Convenience function test passed") - - # Only run real connection test if environment is set up - if os.getenv("FALKORDB_URL") and (os.getenv("OPENAI_API_KEY") or os.getenv("AZURE_API_KEY")): - try: - test_real_connection() - print("✓ Real connection test passed") - except Exception as e: - print(f"✗ Real connection test failed: {e}") - else: - print("⚠ Skipping real connection test (missing environment variables)") - - print("\nAll available tests completed!") +if __name__ == "__main__": + raise SystemExit(pytest.main([__file__, "-q"]))tests/test_library_api.py (3)
5-9: Move local import of os to module top (C0415).Avoid in-function imports unless necessary.
import pytest import asyncio import json +import os from unittest.mock import Mock, patch, AsyncMock from queryweaver import QueryWeaverClient, create_client ... - # Clear any existing API keys - import os + # Clear any existing API keys os.environ.pop("OPENAI_API_KEY", None) os.environ.pop("AZURE_API_KEY", None)Also applies to: 50-58
32-37: Silence unused fixture warnings by prefixing with underscore.Fixtures are needed to activate mocking but the variable isn’t used.
- def test_init_with_openai_key(self, mock_falkordb): + def test_init_with_openai_key(self, _mock_falkordb): ... - def test_init_with_azure_key(self, mock_falkordb): + def test_init_with_azure_key(self, _mock_falkordb): ... - def test_init_without_api_key_raises_error(self, mock_falkordb): + def test_init_without_api_key_raises_error(self, _mock_falkordb): ... - def test_init_with_invalid_falkordb_url_raises_error(self, mock_falkordb): + def test_init_with_invalid_falkordb_url_raises_error(self, _mock_falkordb): ... - def test_create_client_success(self, mock_falkordb): + def test_create_client_success(self, _mock_falkordb): ... - def test_create_client_with_additional_args(self, mock_falkordb): + def test_create_client_with_additional_args(self, _mock_falkordb):Also applies to: 42-49, 50-59, 60-67, 257-263, 266-273
98-106: Use AsyncMock instead of asyncio.Future() for async internals.Simplifies stubbing and avoids event-loop edge cases.
- mock_load_async.return_value = asyncio.Future() - mock_load_async.return_value.set_result(True) + mock_load_async.return_value = AsyncMock(return_value=True) ... - mock_load_async.return_value = asyncio.Future() - mock_load_async.return_value.set_result(False) + mock_load_async.return_value = AsyncMock(return_value=False) ... - mock_generate_async.return_value = asyncio.Future() - mock_generate_async.return_value.set_result("SELECT * FROM users;") + mock_generate_async.return_value = AsyncMock(return_value="SELECT * FROM users;") ... - mock_generate_async.return_value = asyncio.Future() - mock_generate_async.return_value.set_result("SELECT * FROM users LIMIT 10;") + mock_generate_async.return_value = AsyncMock(return_value="SELECT * FROM users LIMIT 10;") ... - mock_query_async.return_value = asyncio.Future() - mock_query_async.return_value.set_result(expected_result) + mock_query_async.return_value = AsyncMock(return_value=expected_result) ... - mock_query_async.return_value = asyncio.Future() - mock_query_async.return_value.set_result(expected_result) + mock_query_async.return_value = AsyncMock(return_value=expected_result) ... - mock_schema_async.return_value = asyncio.Future() - mock_schema_async.return_value.set_result(expected_schema) + mock_schema_async.return_value = AsyncMock(return_value=expected_schema)Also applies to: 109-118, 133-145, 146-161, 176-195, 196-215, 240-252
src/queryweaver/__init__.py (2)
55-59: Set warnings.warn stacklevel to point at the caller.Improves debuggability and satisfies Ruff B028.
- warnings.warn( + warnings.warn( f"Sync QueryWeaver client not available due to missing dependencies: {e}. " "Please install all required dependencies.", - ImportWarning + ImportWarning, + stacklevel=2, ) ... - warnings.warn( + warnings.warn( f"Async QueryWeaver client not available due to missing dependencies: {e}. " "Please install all required dependencies.", - ImportWarning + ImportWarning, + stacklevel=2, )Also applies to: 69-73
83-83: Add final newline to satisfy linters (C0304).if _async_available: __all__.extend(["AsyncQueryWeaverClient", "create_async_client"]) +src/queryweaver/base.py (2)
154-154: Add missing final newline.File should end with a newline character.
Apply this diff:
return list(self._loaded_databases) +
73-82: Avoid mutating Config class attributes at runtime; adopt a safer config pattern.
Mutating Config.COMPLEION_MODEL / EMBEDDING_MODEL via object.setattr is global and not thread-safe — api/config.py creates EMBEDDING_MODEL at import-time and Config is imported across the codebase. Use one of: pass model instances explicitly, switch to an instance-based/singleton config with locking, or add thread-safe setters + reinitialization of dependent clients.
Affected: src/queryweaver/base.py (mutation ~lines 73–82); source: api/config.py (COMPLETION_MODEL at ~68–73; EMBEDDING_MODEL at ~79).examples/async_library_usage.py (3)
16-16: Remove trailing whitespace.Multiple lines have trailing whitespace that should be removed.
Apply this diff to remove trailing whitespace from all affected lines:
- print("=== Basic Async Usage Example ===") - + print("=== Basic Async Usage Example ===") + # Initialize the async client async with AsyncQueryWeaverClient( falkordb_url="redis://localhost:6379/0", openai_api_key="your-openai-api-key" - ) as client: - + ) as client: +Note: Apply similar changes to lines 22, 33, 43, 63, 68, 72, 80, and 333.
30-30: Consider more specific exception handling.Using broad
Exceptioncatches makes debugging harder and might mask unexpected errors. Consider catching specific exceptions where possible.For example, in the database loading section:
try: success = await client.load_database( database_name="ecommerce", database_url="postgresql://user:password@localhost:5432/ecommerce_db" ) print(f"Database loaded successfully: {success}") -except Exception as e: +except (ValueError, ConnectionError, RuntimeError) as e: print(f"Error loading database: {e}") return +except Exception as e: + print(f"Unexpected error loading database: {e}") + raiseAlso applies to: 41-41, 55-55, 99-99, 131-131, 236-236, 269-269, 272-272
255-255: Line exceeds maximum length.Line 255 exceeds 100 characters.
Apply this diff to break the line:
- ("load_valid", lambda: client.load_database("test", "postgresql://user:pass@host/test")), + ("load_valid", lambda: client.load_database( + "test", "postgresql://user:pass@host/test")),src/queryweaver/sync.py (4)
39-45: Move imports to module level.The late import of
api.core.text2sqlcomponents could cause issues with static analysis tools and IDE support.Consider moving these imports to the top of the file after the path manipulation:
# Add the project root to Python path for api imports project_root = Path(__file__).parent.parent.parent sys.path.insert(0, str(project_root)) +from api.core.text2sql import ( + query_database, + get_database_type_and_loader, + GraphNotFoundError, + InternalError, + InvalidArgumentError +) + # Import base class and api modules from .base import BaseQueryWeaverClient -from api.core.text2sql import ( - query_database, - get_database_type_and_loader, - GraphNotFoundError, - InternalError, - InvalidArgumentError -)
145-146: Use logging.exception for better error tracking.When logging exceptions in except blocks, use
logging.exceptionto include the traceback.Apply this pattern throughout the file:
except Exception as e: - logging.error("Error loading database '%s': %s", database_name, str(e)) + logging.exception("Error loading database '%s'", database_name) raise RuntimeError(f"Failed to load database '{database_name}': {e}") from e
148-160: Unused parameter in _load_database_async.The
database_nameparameter is not used in the method body.Either use the parameter for logging or remove it:
-async def _load_database_async(self, database_name: str, database_url: str, loader_class) -> bool: +async def _load_database_async(self, database_url: str, loader_class) -> bool: """Async helper for loading database schema."""And update the call site:
-success = asyncio.run(self._load_database_async(database_name, database_url, loader_class)) +success = asyncio.run(self._load_database_async(database_url, loader_class))
34-36: Avoid runtime sys.path manipulation — make the project importable or use package-relative imports.
Modifying sys.path at runtime is brittle and causes import fragility; convert the repo to a proper package (install editable with pip, set PYTHONPATH in dev, or use relative/package imports) and remove these insertions.
Address in: src/queryweaver/sync.py (lines 34–36) and src/queryweaver/async_client.py.src/queryweaver/async_client.py (4)
138-151: Tighten helper signature and exception logging; drop unused arg.
database_nameis unused and a blindexcept Exceptionhides causes.- async def _load_database_async(self, database_name: str, database_url: str, loader_class) -> bool: + async def _load_database_async(self, database_url: str, loader_class) -> bool: """Async helper for loading database schema.""" try: success = False async for progress in loader_class.load(self._user_id, database_url): success, result = progress if not success: logging.error("Database loader error: %s", result) break return success - except Exception as e: - logging.error("Exception during database loading: %s", str(e)) + except Exception: + logging.exception("Exception during database loading") return FalseAlso update the call site as shown in the previous diff.
331-341: Importget_schemaat module scope; avoid dynamic import and log with stack.Once text2sql is under package internals, import at top and keep this simple.
- try: - from api.core.text2sql import get_schema - schema = await get_schema(self._user_id, database_name) - return schema - except GraphNotFoundError as e: - raise ValueError(str(e)) from e - except InternalError as e: - raise RuntimeError(str(e)) from e + try: + schema = await get_schema(self._user_id, database_name) + return schema + except GraphNotFoundError as e: + raise ValueError(str(e)) from e + except InternalError as e: + raise RuntimeError(str(e)) from e
160-171: Minor: tighten return docs and typing.Return types already annotated. Consider using builtin generics (
dict,list) per PEP 585 across the file for consistency.Also applies to: 236-243
61-90: Optional: reduce__init__params by grouping model config.Bundle
completion_model/embedding_modelinto a config dict to avoid R0913 and make future options extensible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ASYNC_API_IMPLEMENTATION.md(1 hunks)LIBRARY_IMPLEMENTATION.md(1 hunks)MANIFEST.in(1 hunks)README.md(1 hunks)api/core/schema_loader.py(2 hunks)docs/library-usage.md(1 hunks)examples/async_library_usage.py(1 hunks)examples/library_usage.py(1 hunks)setup.py(1 hunks)src/queryweaver/__init__.py(1 hunks)src/queryweaver/async_client.py(1 hunks)src/queryweaver/base.py(1 hunks)src/queryweaver/sync.py(1 hunks)tests/test_async_library_api.py(1 hunks)tests/test_integration.py(1 hunks)tests/test_library_api.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
src/queryweaver/base.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
88-88: Avoid specifying long messages outside the exception class
(TRY003)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
129-129: Avoid specifying long messages outside the exception class
(TRY003)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_integration.py
90-90: Do not catch blind exception: Exception
(BLE001)
examples/library_usage.py
28-28: Do not catch blind exception: Exception
(BLE001)
39-39: Do not catch blind exception: Exception
(BLE001)
53-53: Do not catch blind exception: Exception
(BLE001)
82-82: Do not catch blind exception: Exception
(BLE001)
150-150: Do not catch blind exception: Exception
(BLE001)
218-218: Do not catch blind exception: Exception
(BLE001)
tests/test_async_library_api.py
20-20: Unused function argument: mock_falkordb
(ARG001)
31-31: Unused method argument: mock_falkordb
(ARG002)
41-41: Unused method argument: mock_falkordb
(ARG002)
49-49: Unused method argument: mock_falkordb
(ARG002)
59-59: Unused method argument: mock_falkordb
(ARG002)
256-256: Unused method argument: mock_falkordb
(ARG002)
269-269: Unused method argument: mock_falkordb
(ARG002)
278-278: Unused method argument: mock_falkordb
(ARG002)
src/queryweaver/sync.py
118-121: Avoid specifying long messages outside the exception class
(TRY003)
126-129: Avoid specifying long messages outside the exception class
(TRY003)
142-142: Abstract raise to an inner function
(TRY301)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
146-146: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Unused method argument: database_name
(ARG002)
157-157: Consider moving this statement to an else block
(TRY300)
158-158: Do not catch blind exception: Exception
(BLE001)
159-159: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
194-194: Consider moving this statement to an else block
(TRY300)
197-197: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
220-220: Avoid specifying long messages outside the exception class
(TRY003)
222-222: Consider moving this statement to an else block
(TRY300)
263-263: Consider moving this statement to an else block
(TRY300)
266-266: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
267-267: Avoid specifying long messages outside the exception class
(TRY003)
312-312: Consider moving this statement to an else block
(TRY300)
334-334: Avoid specifying long messages outside the exception class
(TRY003)
339-339: Consider moving this statement to an else block
(TRY300)
342-342: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
343-343: Avoid specifying long messages outside the exception class
(TRY003)
350-350: Consider moving this statement to an else block
(TRY300)
tests/test_library_api.py
21-21: Unused function argument: mock_falkordb
(ARG001)
32-32: Unused method argument: mock_falkordb
(ARG002)
42-42: Unused method argument: mock_falkordb
(ARG002)
50-50: Unused method argument: mock_falkordb
(ARG002)
60-60: Unused method argument: mock_falkordb
(ARG002)
257-257: Unused method argument: mock_falkordb
(ARG002)
266-266: Unused method argument: mock_falkordb
(ARG002)
src/queryweaver/__init__.py
55-55: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
69-69: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
src/queryweaver/async_client.py
117-120: Avoid specifying long messages outside the exception class
(TRY003)
132-132: Abstract raise to an inner function
(TRY301)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
135-135: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
136-136: Avoid specifying long messages outside the exception class
(TRY003)
138-138: Unused method argument: database_name
(ARG002)
147-147: Consider moving this statement to an else block
(TRY300)
148-148: Do not catch blind exception: Exception
(BLE001)
149-149: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
183-183: Consider moving this statement to an else block
(TRY300)
186-186: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
187-187: Avoid specifying long messages outside the exception class
(TRY003)
208-208: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Consider moving this statement to an else block
(TRY300)
250-250: Consider moving this statement to an else block
(TRY300)
253-253: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
254-254: Avoid specifying long messages outside the exception class
(TRY003)
299-299: Consider moving this statement to an else block
(TRY300)
321-321: Avoid specifying long messages outside the exception class
(TRY003)
325-325: Consider moving this statement to an else block
(TRY300)
328-328: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
336-336: Consider moving this statement to an else block
(TRY300)
examples/async_library_usage.py
30-30: Do not catch blind exception: Exception
(BLE001)
41-41: Do not catch blind exception: Exception
(BLE001)
55-55: Do not catch blind exception: Exception
(BLE001)
99-99: Do not catch blind exception: Exception
(BLE001)
131-131: Do not catch blind exception: Exception
(BLE001)
236-236: Do not catch blind exception: Exception
(BLE001)
269-269: Do not catch blind exception: Exception
(BLE001)
272-272: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: Pylint
src/queryweaver/base.py
[warning] 154-154: Final newline missing (missing-final-newline) - C0304
tests/test_integration.py
[error] 1-1: No value for argument 'mock_falkordb' in function call (E1120)
[warning] 95-95: Final newline missing (missing-final-newline) - C0304
examples/library_usage.py
[warning] 240-240: Final newline missing (missing-final-newline) - C0304
tests/test_async_library_api.py
[warning] 6-6: C0411: Import not at top of module (os) (import-outside-toplevel)
src/queryweaver/sync.py
[warning] 39-39: Pylint: Import outside toplevel (C0411).
tests/test_library_api.py
[warning] 6-6: C0411: Import not at top of module (asyncio) (import-outside-toplevel)
[warning] 38-38: C0415: Import outside-toplevel (os) (import-outside-toplevel)
src/queryweaver/__init__.py
[warning] 83-83: Final newline missing (missing-final-newline) - C0304
src/queryweaver/async_client.py
[warning] 40-40: Import not at top of module (C0413: import-outside-toplevel)
[warning] 41-41: Import not at top of module (C0413: import-outside-toplevel)
[warning] 61-61: Too many arguments in function call (R0913: too-many-arguments)
examples/async_library_usage.py
[warning] 16-16: Trailing whitespace (C0303: trailing-whitespace)
[warning] 22-22: Trailing whitespace (C0303: trailing-whitespace)
[warning] 33-33: Trailing whitespace (C0303: trailing-whitespace)
[warning] 43-43: Trailing whitespace (C0303: trailing-whitespace)
[warning] 63-63: Trailing whitespace (C0303: trailing-whitespace)
[warning] 68-68: Trailing whitespace (C0303: trailing-whitespace)
[warning] 72-72: Trailing whitespace (C0303: trailing-whitespace)
[warning] 80-80: Trailing whitespace (C0303: trailing-whitespace)
[warning] 255-255: Line too long (105/100) (C0301: line-too-long)
[warning] 333-333: Trailing whitespace (C0303: trailing-whitespace)
🪛 markdownlint-cli2 (0.17.2)
LIBRARY_IMPLEMENTATION.md
49-49: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
49-49: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (7)
MANIFEST.in (1)
1-13: LGTM! The manifest correctly excludes development artifacts.The MANIFEST.in file properly includes essential files (README, LICENSE, SECURITY) and source code while excluding test files, examples, and build artifacts. This follows best practices for Python package distribution.
api/core/schema_loader.py (1)
148-148: Good improvement makinggeneral_prefixoptional.The change from required to optional parameter provides better API flexibility while maintaining backward compatibility. This aligns well with the library API's needs where demo graphs may not always be needed.
README.md (1)
125-148: Excellent addition of the Python Library section!The Quick Example provides a clear, concise demonstration of the library's core functionality. The code example effectively shows the key operations: initialization, database loading, SQL generation, and query execution.
tests/test_async_library_api.py (1)
1-285: Comprehensive test coverage for async client!The test suite thoroughly covers all major functionality of the AsyncQueryWeaverClient including:
- Initialization with different API key configurations
- Error handling for invalid inputs
- Database loading operations
- Text-to-SQL generation
- Query execution with and without SQL execution
- Utility methods and context manager support
The use of mocks and fixtures is appropriate for unit testing without external dependencies.
ASYNC_API_IMPLEMENTATION.md (1)
1-230: LGTM! Comprehensive async API documentation.This documentation provides excellent coverage of the async API implementation, including clear examples, performance benefits, and best practices. The structure and content are well-organized and informative.
LIBRARY_IMPLEMENTATION.md (1)
1-214: LGTM! Excellent implementation summary.The documentation comprehensively covers the library implementation details, API design, and usage patterns. It effectively addresses all requirements from issue #252.
src/queryweaver/async_client.py (1)
195-197: Incorrect — keep the awaitquery_database(...) is an async function that returns the inner async generator (it does
return generate()), sogenerator = await query_database(...)correctly awaits the coroutine to obtain the async generator; removingawaitwould makegeneratora coroutine and breakasync for. (api/core/text2sql.py:590).Likely an incorrect or invalid review comment.
src/queryweaver/async_client.py
Outdated
| # Add the project root to Python path for api imports | ||
| project_root = Path(__file__).parent.parent.parent | ||
| sys.path.insert(0, str(project_root)) | ||
|
|
||
| # Import base class and api modules | ||
| from .base import BaseQueryWeaverClient | ||
| from api.core.text2sql import ( | ||
| query_database, | ||
| get_database_type_and_loader, | ||
| GraphNotFoundError, | ||
| InternalError, | ||
| InvalidArgumentError | ||
| ) | ||
|
|
There was a problem hiding this comment.
Stop path-hacking; import from package internals to make the library installable.
Mutating sys.path and importing a top-level api.* module will break when this is installed via pip. Fold the Text→SQL engine under the queryweaver package (e.g., queryweaver.core.text2sql) and use relative imports. Also fixes the Pylint C0413 warnings.
Apply this diff after relocating the module into src/queryweaver/core/text2sql.py:
-# Add the project root to Python path for api imports
-project_root = Path(__file__).parent.parent.parent
-sys.path.insert(0, str(project_root))
-
-# Import base class and api modules
-from .base import BaseQueryWeaverClient
-from api.core.text2sql import (
- query_database,
- get_database_type_and_loader,
- GraphNotFoundError,
- InternalError,
- InvalidArgumentError
-)
+# Import base class and package-internal modules
+from .base import BaseQueryWeaverClient
+from .core.text2sql import (
+ query_database,
+ get_database_type_and_loader,
+ get_schema,
+ GraphNotFoundError,
+ InternalError,
+ InvalidArgumentError,
+)📝 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.
| # Add the project root to Python path for api imports | |
| project_root = Path(__file__).parent.parent.parent | |
| sys.path.insert(0, str(project_root)) | |
| # Import base class and api modules | |
| from .base import BaseQueryWeaverClient | |
| from api.core.text2sql import ( | |
| query_database, | |
| get_database_type_and_loader, | |
| GraphNotFoundError, | |
| InternalError, | |
| InvalidArgumentError | |
| ) | |
| # Import base class and package-internal modules | |
| from .base import BaseQueryWeaverClient | |
| from .core.text2sql import ( | |
| query_database, | |
| get_database_type_and_loader, | |
| get_schema, | |
| GraphNotFoundError, | |
| InternalError, | |
| InvalidArgumentError, | |
| ) |
🧰 Tools
🪛 GitHub Actions: Pylint
[warning] 40-40: Import not at top of module (C0413: import-outside-toplevel)
[warning] 41-41: Import not at top of module (C0413: import-outside-toplevel)
🤖 Prompt for AI Agents
In src/queryweaver/async_client.py around lines 35 to 48, remove the sys.path
manipulation and top-level api.* imports; instead import the relocated Text→SQL
engine as a package-internal module (move api/core/text2sql.py into
src/queryweaver/core/text2sql.py) and change imports to relative package imports
(e.g., from .core.text2sql import query_database, get_database_type_and_loader,
GraphNotFoundError, InternalError, InvalidArgumentError). Also remove the
project_root insertion, update any other references to use the new package path,
and ensure package __init__.py files and setup/pyproject include the
queryweaver.core package so installation via pip works without path-hacking.
src/queryweaver/async_client.py
Outdated
| # Suppress FalkorDB logs if too verbose | ||
| logging.getLogger("falkordb").setLevel(logging.WARNING) | ||
|
|
There was a problem hiding this comment.
Avoid module-level logging side effects.
Setting logger levels at import time surprises host apps. Remove this and let callers configure logging.
-# Suppress FalkorDB logs if too verbose
-logging.getLogger("falkordb").setLevel(logging.WARNING)📝 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.
| # Suppress FalkorDB logs if too verbose | |
| logging.getLogger("falkordb").setLevel(logging.WARNING) |
🤖 Prompt for AI Agents
In src/queryweaver/async_client.py around lines 49-51, remove the module-level
call that sets the "falkordb" logger level
(logging.getLogger("falkordb").setLevel(logging.WARNING) ) because import-time
logging side effects should be avoided; instead delete that line and either
document that callers should configure logging themselves or add an explicit
helper function (e.g., set_falkordb_log_level(level)) that callers can call when
they want to change the logger level, leaving no logging configuration at import
time.
src/queryweaver/async_client.py
Outdated
| generator = await query_database(self._user_id, database_name, chat_data) | ||
|
|
||
| # Process the streaming response from query_database | ||
| async for chunk in generator: | ||
| if isinstance(chunk, str): | ||
| try: | ||
| data = json.loads(chunk) | ||
|
|
There was a problem hiding this comment.
Critical: same async-generator misuse as above.
Remove await before iterating the stream.
- generator = await query_database(self._user_id, database_name, chat_data)
+ generator = query_database(self._user_id, database_name, chat_data)📝 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.
| generator = await query_database(self._user_id, database_name, chat_data) | |
| # Process the streaming response from query_database | |
| async for chunk in generator: | |
| if isinstance(chunk, str): | |
| try: | |
| data = json.loads(chunk) | |
| generator = query_database(self._user_id, database_name, chat_data) | |
| # Process the streaming response from query_database | |
| async for chunk in generator: | |
| if isinstance(chunk, str): | |
| try: | |
| data = json.loads(chunk) |
🤖 Prompt for AI Agents
In src/queryweaver/async_client.py around lines 267 to 274, the code awaits
query_database and then async-iterates the result, incorrectly awaiting an async
generator; change the assignment to call query_database without await (i.e.,
generator = query_database(self._user_id, database_name, chat_data)) so you
obtain an async iterator and then use async for chunk in generator to consume
it; ensure any exceptions from the async generator are handled where iteration
occurs.
src/queryweaver/async_client.py
Outdated
| result["analysis"] = { | ||
| "explanation": data.get("exp", ""), | ||
| "assumptions": data.get("assumptions", ""), | ||
| "ambiguities": data.get("amb", ""), | ||
| "missing_information": data.get("miss", "") | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify analysis payload keys.
The stream maps analysis using keys exp, assumptions, amb, miss. Confirm these match producer fields.
Expected: matching keys in the producer. If mismatched, align names here or normalize in one place.
🏁 Script executed:
#!/bin/bash
# Inspect producers of analysis payloads
rg -nP -C3 '"type"\s*:\s*"analysis"|analysis\b.*(exp|assumptions|amb|miss)' api/ src/ || trueLength of output: 1358
Align analysis payload keys between producer and consumer.
async_client reads short keys ('exp' / 'amb' / 'miss') while the producer uses full keys ('explanation' / 'ambiguities' / 'missing_information'); update the consumer to use the producer's keys or make the producer emit the short keys. Files: src/queryweaver/async_client.py:279-285, api/agents/analysis_agent.py:34-50.
🤖 Prompt for AI Agents
In src/queryweaver/async_client.py around lines 279-285, the consumer is reading
short keys ("exp","amb","miss") but the producer emits full keys
("explanation","ambiguities","missing_information"); change the consumer to pull
data.get("explanation",""), data.get("assumptions",""),
data.get("ambiguities",""), and data.get("missing_information","") when building
result["analysis"], preserving the empty-string fallback for missing fields so
payload keys match the producer.
fix #252
Summary by CodeRabbit
New Features
Documentation
Tests
Chores