-
Notifications
You must be signed in to change notification settings - Fork 18
enh / fix: refactor quantbook support for docker instead of hacky original #8
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This pull request introduces a major architectural enhancement by integrating Docker to run QuantBook research environments in isolated, secure containers using lean-cli. The change makes QuantBook functionality optional through containerized sessions, improving security and providing robust session management while maintaining API compatibility.
Key changes include:
- Replaced direct QuantBook imports with containerized execution using lean-cli and Docker
- Introduced optional QuantBook support with
ENABLE_QUANTBOOK
environment variable and[quantbook]
extra dependency - Added comprehensive session management with automatic cleanup and resource monitoring
Reviewed Changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
quantconnect_mcp/src/utils.py | Enhanced safe_print function with better error handling for broken pipes and encoding issues |
quantconnect_mcp/src/tools/universe_tools.py | Added conditional import wrapper to avoid Docker dependency issues when QuantBook unavailable |
quantconnect_mcp/src/tools/quantbook_tools.py | Complete refactor to containerized session management with lean-cli integration |
quantconnect_mcp/src/tools/portfolio_tools.py | Added conditional import wrapper to gracefully handle missing QuantBook dependencies |
quantconnect_mcp/src/tools/data_tools.py | Converted to container-based execution with notebook modification approach |
quantconnect_mcp/src/tools/analysis_tools.py | Added conditional import wrapper for optional QuantBook functionality |
quantconnect_mcp/src/tools/init.py | Updated exports to conditionally import QuantBook tools |
quantconnect_mcp/src/server.py | Removed main function, moved configuration to main.py |
quantconnect_mcp/src/resources/system_resources.py | Updated to work with new session manager instead of direct QuantBook instances |
quantconnect_mcp/src/adapters/session_manager.py | New session manager for handling multiple research sessions with lifecycle management |
quantconnect_mcp/src/adapters/research_session_lean_cli.py | New lean-cli based research session implementation with Jupyter notebook integration |
quantconnect_mcp/main.py | Enhanced main function with conditional QuantBook imports, signal handling, and improved error handling |
pyproject.toml | Added optional quantbook dependencies with Docker requirement |
README.md | Comprehensive documentation update explaining new container-based architecture |
Comments suppressed due to low confidence (4)
quantconnect_mcp/src/adapters/research_session_lean_cli.py:565
- The 'import os' statement should be moved to the top of the file with other imports rather than being placed inside the execute method. This follows Python import conventions and improves code readability.
# Write the updated notebook back
quantconnect_mcp/src/adapters/research_session_lean_cli.py:566
- The 'from datetime import datetime as dt' import should be moved to the top of the file with other imports. Also, since 'datetime' is already imported at the top as a full module, this creates a potential naming conflict.
notebook_json = json.dumps(notebook_content, indent=2)
quantconnect_mcp/src/adapters/research_session_lean_cli.py:567
- Hardcoded absolute path contains a username and should not be in production code. This creates a security risk and will fail on systems where this path doesn't exist. Use a configurable path or temporary directory instead.
write_cmd = f"cat > {notebook_path} << 'EOF'\n{notebook_json}\nEOF"
quantconnect_mcp/src/adapters/session_manager.py:160
- Type annotation uses lowercase 'any' instead of 'Any'. Should be 'Any' (capital A) which needs to be imported from typing module.
def list_sessions(self) -> List[Dict[str, any]]:
# If we still can't print, log instead | ||
try: | ||
logger.info(f"[Output] {text}") | ||
except: |
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.
Bare except clause should specify exception type. Consider using 'except Exception:' to catch specific exceptions while allowing system exits and keyboard interrupts to propagate.
except: | |
except Exception: |
Copilot uses AI. Check for mistakes.
"count": len(_quantbook_instances), | ||
"status": "success", | ||
} | ||
try: |
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 exception handling in list_quantbook_instances should provide more specific error messages. The current generic 'Failed to list QuantBook instances' doesn't help users understand what went wrong (e.g., session manager not initialized, Docker unavailable, etc.).
Copilot uses AI. Check for mistakes.
"status": "error", | ||
"error": f"Unsupported data type '{data_type}'. Currently supported: SmartInsiderTransaction", | ||
} | ||
# TODO: Convert to container execution like other functions |
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 TODO comment indicates incomplete functionality for add_alternative_data. This function returns an error stating it's 'temporarily disabled', which may confuse users. Consider either implementing the functionality or removing the tool registration until it's complete.
Copilot uses AI. Check for mistakes.
for col in history.columns: | ||
data[col] = history[col].unstack(level=0).to_dict() | ||
|
||
# TODO: Convert to container execution like other functions |
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 TODO comment indicates incomplete functionality for get_alternative_data_history. This function returns an error stating it's 'temporarily disabled', which may confuse users. Consider either implementing the functionality or removing the tool registration until it's complete.
Copilot uses AI. Check for mistakes.
# Cleanup | ||
if _session_manager: | ||
try: | ||
import asyncio |
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 'import asyncio' statement inside the finally block should be moved to the top-level imports since asyncio is already imported at the module level.
import asyncio |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Containerized QuantBook Support with lean-cli Integration
This pull request introduces a major architectural enhancement by integrating Docker to run QuantBook research
environments in isolated, secure containers using lean-cli. This change makes QuantBook functionality optional, improves
security, and provides robust session management.
Key Features & Changes
🐳 Containerized QuantBook Sessions: All QuantBook tools now execute within dedicated Docker containers managed
by lean-cli and a new
SessionManager
. This isolates research environments, preventing conflicts and enhancingstability. See the new
ResearchSession
andSessionManager
for implementation details.** Jupyter Notebook Integration**:
/LeanCLI/research.ipynb
whereqb
is pre-initializedexecute()
method automatically updates the notebook file and ensures code runs in the proper kernelenvironment
** lean-cli Integration**:
** Optional Installation**: QuantBook support is now an optional feature.
pip install quantconnect-mcp
.[quantbook]
extra:pip install "quantconnect-mcp[quantbook]"
.ENABLE_QUANTBOOK=true
environment variable.** Enhanced Security**: Each container runs with resource limits (CPU/memory) and proper isolation, providing a
secure sandbox for code execution.
** New Management Tools**:
execute_quantbook_code
: Execute Python code directly within aQuantBook session via notebook modification
get_session_manager_status
: Inspect the state of the sessionmanager and view active sessions
check_quantbook_container
: Verify container status andavailability
** Architectural Improvements**:
quantbook_tools.py
have beenrefactored to be fully
async
and communicate with containerized sessionsqb
is needed** Documentation & Usability**:
qb
is pre-initialized and available only in notebooksREADME.md
has been updated with new instructions for installation, configuration, and usage--version
flag has been added to the CLIImportant Usage Notes
qb
) is pre-initialized in the Jupyter environment at/LeanCLI/research.ipynb
Foundation-Py-Default
kernel to accessqb
execute_quantbook_code
function works by modifying the notebook file, not running standalone scripts