Fix issues #544 and #364: Add --extra-docker-config-file and --docker… #604

mrleftyhookz · 2025-12-05T21:23:36Z

…-timeout options

Issue #544: Fix Windows Argument Parsing for --extra-docker-config

Added --extra-docker-config-file option to all commands that support --extra-docker-config
Created lean/components/util/json_parser.py with robust JSON parsing
Handles Windows shell quote mangling with fallback strategies
Provides helpful error messages for JSON parsing failures
Updated commands: backtest, research, optimize, live/deploy

Issue #364: Add Docker Client Timeout Configuration

Added timeout parameter to DockerManager.init() with default of 60 seconds
Modified _get_docker_client() to use configurable timeout
Added --docker-timeout option to all relevant commands
Supports DOCKER_CLIENT_TIMEOUT environment variable
Updated container.py to read environment variable on initialization
Updated commands: backtest, research, optimize, live/deploy

All changes are backward compatible - existing usage continues to work. Tested with Python 3.12 - all tests pass successfully.

…config-file and --docker-timeout options Issue QuantConnect#544: Fix Windows Argument Parsing for --extra-docker-config - Added --extra-docker-config-file option to all commands that support --extra-docker-config - Created lean/components/util/json_parser.py with robust JSON parsing - Handles Windows shell quote mangling with fallback strategies - Provides helpful error messages for JSON parsing failures - Updated commands: backtest, research, optimize, live/deploy Issue QuantConnect#364: Add Docker Client Timeout Configuration - Added timeout parameter to DockerManager.__init__() with default of 60 seconds - Modified _get_docker_client() to use configurable timeout - Added --docker-timeout option to all relevant commands - Supports DOCKER_CLIENT_TIMEOUT environment variable - Updated container.py to read environment variable on initialization - Updated commands: backtest, research, optimize, live/deploy All changes are backward compatible - existing usage continues to work. Tested with Python 3.12 - all tests pass successfully.

Copilot

Pull request overview

This PR addresses two issues: fixing Windows argument parsing for Docker configuration (#544) and adding Docker client timeout configuration (#364). The changes introduce a new --extra-docker-config-file option as an alternative to --extra-docker-config to avoid Windows shell quote issues, and add a --docker-timeout option to configure Docker client operation timeouts.

Key Changes:

Created a new JSON parser utility (lean/components/util/json_parser.py) with fallback strategies for handling Windows shell quote mangling
Added configurable timeout parameter to DockerManager with support for environment variable override
Updated all relevant commands (backtest, research, optimize, live/deploy) to support both new options

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
lean/components/util/json_parser.py	New utility module with JSON parsing functions supporting file and string input with Windows shell workarounds
lean/components/docker/docker_manager.py	Added timeout parameter to constructor and `_get_docker_client()` with environment variable support
lean/container.py	Modified DockerManager initialization to read timeout from `DOCKER_CLIENT_TIMEOUT` environment variable
lean/commands/backtest.py	Added `--extra-docker-config-file` and `--docker-timeout` options with corresponding parameter handling
lean/commands/research.py	Added `--extra-docker-config-file` and `--docker-timeout` options with corresponding parameter handling
lean/commands/optimize.py	Added `--extra-docker-config-file` and `--docker-timeout` options with corresponding parameter handling
lean/commands/live/deploy.py	Added `--extra-docker-config-file` and `--docker-timeout` options with corresponding parameter handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T21:28:13Z

lean/components/util/json_parser.py

+    attempts = [
+        json_string,
+        json_string.replace("'", '"'),  # Single quotes to double quotes
+        '{"' + json_string.strip('{}').replace(':', '":').replace(',', ',"') + '}',  # Add missing quotes


This string manipulation logic is broken and will produce malformed JSON in many cases. For example, with input key1:value1,key2:value2, this will produce {"key1":value1","key2":"value2"} (extra quote after value1). The logic incorrectly assumes every colon and comma needs quote handling.

This fallback strategy should be removed or completely reimplemented with proper parsing logic. Consider using a more targeted approach that only handles specific known Windows shell issues rather than attempting generic quote repair.

Suggested change

'{"' + json_string.strip('{}').replace(':', '":').replace(',', ',"') + '}', # Add missing quotes

Copilot · 2025-12-05T21:28:13Z

lean/commands/live/deploy.py

+
+    # Set Docker timeout if specified
+    if docker_timeout is not None:
+        container.docker_manager._timeout = docker_timeout


Directly accessing the private attribute _timeout breaks encapsulation. Consider adding a public setter method to DockerManager instead:

# In DockerManager class: def set_timeout(self, timeout: int) -> None: """Set the timeout for Docker client operations.""" self._timeout = timeout

Then use: container.docker_manager.set_timeout(docker_timeout)

Suggested change

container.docker_manager._timeout = docker_timeout

container.docker_manager.set_timeout(docker_timeout)

Copilot · 2025-12-05T21:28:13Z

lean/commands/backtest.py

+
+    # Set Docker timeout if specified
+    if docker_timeout is not None:
+        container.docker_manager._timeout = docker_timeout


Directly accessing the private attribute _timeout breaks encapsulation. Consider adding a public setter method to DockerManager instead:

# In DockerManager class: def set_timeout(self, timeout: int) -> None: """Set the timeout for Docker client operations.""" self._timeout = timeout

Then use: container.docker_manager.set_timeout(docker_timeout)

Suggested change

container.docker_manager._timeout = docker_timeout

container.docker_manager.set_timeout(docker_timeout)

Copilot · 2025-12-05T21:28:14Z

lean/components/util/json_parser.py

+    """
+    Loads JSON configuration from either a string or a file.
+    
+    :param json_string: JSON string to parse (optional)
+    :param json_file: Path to JSON file (optional)
+    :return: Parsed dictionary
+    :raises ValueError: If both parameters are None or if parsing fails


The docstring states "raises ValueError: If both parameters are None or if parsing fails", but the function returns an empty dict {} when both parameters are None (line 94) instead of raising ValueError. This is inconsistent with the documented behavior.

Either update the implementation to raise an error:

if json_string is None and json_file is None: raise ValueError("Either json_string or json_file must be provided") return {}

Or update the docstring to accurately reflect the current behavior:

:return: Parsed dictionary, or empty dict if both parameters are None

Copilot · 2025-12-05T21:28:14Z

lean/commands/backtest.py

+
+    # Set Docker timeout if specified
+    if docker_timeout is not None:
+        container.docker_manager._timeout = docker_timeout


[nitpick] Missing blank line after the timeout setting block for consistency. Other commands (research.py, optimize.py, deploy.py) all have a blank line after this block. Add a blank line after line 336 for consistency.

Suggested change

container.docker_manager._timeout = docker_timeout

container.docker_manager._timeout = docker_timeout

Copilot · 2025-12-05T21:28:15Z

lean/components/docker/docker_manager.py

+            from os import environ
+
+            # Check for environment variable override
+            timeout = int(environ.get("DOCKER_CLIENT_TIMEOUT", self._timeout))


Missing error handling for invalid DOCKER_CLIENT_TIMEOUT environment variable values. If the environment variable contains a non-integer value, int() will raise a ValueError that will be caught by the generic exception handler at line 581, resulting in a misleading "Docker is not running" error message.

Add explicit error handling:

try: timeout = int(environ.get("DOCKER_CLIENT_TIMEOUT", self._timeout)) except ValueError: timeout = self._timeout # Fall back to instance timeout on invalid value

Suggested change

timeout = int(environ.get("DOCKER_CLIENT_TIMEOUT", self._timeout))

try:

timeout = int(environ.get("DOCKER_CLIENT_TIMEOUT", self._timeout))

except ValueError:

timeout = self._timeout # Fall back to instance timeout on invalid value

Copilot · 2025-12-05T21:28:15Z

lean/commands/research.py

+
+    # Set Docker timeout if specified
+    if docker_timeout is not None:
+        container.docker_manager._timeout = docker_timeout


Directly accessing the private attribute _timeout breaks encapsulation. Consider adding a public setter method to DockerManager instead:

# In DockerManager class: def set_timeout(self, timeout: int) -> None: """Set the timeout for Docker client operations.""" self._timeout = timeout

Then use: container.docker_manager.set_timeout(docker_timeout)

Suggested change

container.docker_manager._timeout = docker_timeout

container.docker_manager.set_timeout(docker_timeout)

Copilot · 2025-12-05T21:28:15Z

lean/commands/optimize.py


+    # Set Docker timeout if specified
+    if docker_timeout is not None:
+        container.docker_manager._timeout = docker_timeout


Directly accessing the private attribute _timeout breaks encapsulation. Consider adding a public setter method to DockerManager instead:

# In DockerManager class: def set_timeout(self, timeout: int) -> None: """Set the timeout for Docker client operations.""" self._timeout = timeout

Then use: container.docker_manager.set_timeout(docker_timeout)

Suggested change

container.docker_manager._timeout = docker_timeout

container.docker_manager.set_timeout(docker_timeout)

Copilot · 2025-12-05T21:28:15Z

lean/commands/backtest.py

+                   "This is recommended over --extra-docker-config on Windows to avoid shell quote issues.")
+@option("--docker-timeout",
+              type=int,
+              help="Docker client timeout in seconds (default: 60). "


[nitpick] The help text for --docker-timeout is inconsistent across commands. This command uses "Docker client timeout in seconds" while other commands (research.py, optimize.py, deploy.py) use "Timeout in seconds for Docker operations".

For consistency and clarity, use the same wording across all commands:

help="Timeout in seconds for Docker operations (default: 60). "

Suggested change

help="Docker client timeout in seconds (default: 60). "

help="Timeout in seconds for Docker operations (default: 60). "

Copilot · 2025-12-05T21:28:16Z

lean/components/util/json_parser.py

+    if json_file is not None:
+        if not json_file.exists():
+            raise ValueError(f"Configuration file not found: {json_file}")
+
+        try:
+            with open(json_file, 'r', encoding='utf-8') as f:
+                content = f.read()
+                return loads(content)
+        except JSONDecodeError as e:
+            raise ValueError(
+                f"Failed to parse JSON from file {json_file}: {e}\n"
+                f"Please ensure the file contains valid JSON."
+            )
+        except Exception as e:
+            raise ValueError(f"Failed to read file {json_file}: {e}")
+
+    if json_string is not None:
+        return parse_json_safely(json_string)
+
+    return {}


The function silently prioritizes json_file over json_string when both are provided, which could confuse users. Consider adding validation to prevent both parameters from being specified simultaneously, or at least logging a warning.

if json_file is not None and json_string is not None: raise ValueError("Cannot specify both json_string and json_file. Please use only one.")

Alternatively, update the docstring to clearly document this precedence behavior.

…de quality - Remove broken JSON parsing fallback logic that produced malformed JSON - Add set_timeout() method to DockerManager for proper encapsulation - Fix docstring inconsistency in load_json_from_file_or_string() - Add error handling for invalid DOCKER_CLIENT_TIMEOUT environment variable - Make help text consistent across all commands - Document precedence when both json_file and json_string are provided - Fix formatting consistency (add blank lines)

Copilot AI review requested due to automatic review settings December 5, 2025 21:23

Copilot started reviewing on behalf of mrleftyhookz December 5, 2025 21:24 View session

Copilot AI reviewed Dec 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix issues #544 and #364: Add --extra-docker-config-file and --docker… #604

Fix issues #544 and #364: Add --extra-docker-config-file and --docker… #604

mrleftyhookz commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	container.docker_manager._timeout = docker_timeout
	container.docker_manager.set_timeout(docker_timeout)

	help="Docker client timeout in seconds (default: 60). "
	help="Timeout in seconds for Docker operations (default: 60). "

Fix issues #544 and #364: Add --extra-docker-config-file and --docker… #604

Are you sure you want to change the base?

Fix issues #544 and #364: Add --extra-docker-config-file and --docker… #604

Conversation

mrleftyhookz commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant