♻️ Refactor - #55
Conversation
📝 WalkthroughWalkthroughThis PR introduces infrastructure improvements for the CraftGround environment: a constants module for configuration values, a custom exception hierarchy, enhanced port validation and connection timeout handling in socket IPC, validation for image dimensions, platform-specific process termination logic with comprehensive error handling, and updated API signatures with docstrings. Additionally, new unit tests validate configuration and IPC behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/craftground/environment/socket_ipc.py`:
- Around line 98-101: The PortInUseError message is using the
already-incremented port value, making "starting from {port}" inaccurate; change
the check in the loop that raises PortInUseError (where MAX_PORT and port are
referenced) to report the original starting port (store the initial port in a
variable like start_port before incrementing) and use that start_port in the
PortInUseError message so it matches the actual requested start value (reference
the port variable, MAX_PORT, and PortInUseError in your fix).
- Around line 281-284: The premature timeout comes from the extra next_output >
MAX_CONNECTION_RETRIES check and the raised ConnectionTimeoutError lacks
exception chaining; update the logic in socket_ipc.py (look for next_output,
wait_time, MAX_CONNECTION_RETRIES, and ConnectionTimeoutError) by removing the
redundant next_output > MAX_CONNECTION_RETRIES branch so the loop relies solely
on wait_time < MAX_CONNECTION_RETRIES for termination, and when you do raise
ConnectionTimeoutError include the original exception with "raise
ConnectionTimeoutError(...) from err" (capture the original exception variable
where the wait/connection error occurred) to preserve the exception chain.
- Around line 77-80: The error message uses the incremented port value instead
of the original starting port; capture the initial starting port (e.g.,
start_port = port) before the loop that increments port and then, when raising
PortInUseError, use that preserved start_port in the message (keep the
PortInUseError raise site and MAX_PORT check unchanged but swap the f-string to
reference start_port).
🧹 Nitpick comments (6)
tests/python/unit/test_environment_lifecycle.py (2)
13-19: Missing environment cleanup after test.The test creates an environment but doesn't close it, which may leak resources (sockets, processes). Consider using a fixture or explicit cleanup.
♻️ Suggested fix
def test_environment_creation(self): """Test that environment can be created.""" config = InitialEnvironmentConfig(image_width=64, image_height=64) env = make(initial_env_config=config, verbose=False) - assert env is not None - assert env.observation_space is not None - assert env.action_space is not None + try: + assert env is not None + assert env.observation_space is not None + assert env.action_space is not None + finally: + env.close()
21-33: Consider adding height validation test cases.The invalid size tests focus on
image_width. For symmetry and complete coverage, consider adding test cases for invalidimage_heightvalues as well.♻️ Suggested addition
# Invalid height - too small with pytest.raises(InvalidImageSizeError): InitialEnvironmentConfig(image_width=64, image_height=0) # Invalid height - too large with pytest.raises(InvalidImageSizeError): InitialEnvironmentConfig(image_width=64, image_height=10000)src/craftground/initial_environment_config.py (1)
133-139: Consider reordering validation and kwargs check.Currently,
_validate()is called before the unknown kwargs warning. If validation fails, users won't see any warning about unexpected parameters. Consider swapping the order so users get all feedback.♻️ Suggested reorder
- # Validate configuration - self._validate() - # Check for unknown kwargs if kwargs: print(f"Unexpected Kwargs: {kwargs}") + + # Validate configuration + self._validate()tests/python/unit/test_ipc_mock.py (3)
10-14: Unused import:ConnectionTimeoutError.
ConnectionTimeoutErroris imported but not used in any test. Remove it to keep imports clean.♻️ Suggested fix
from craftground.exceptions import ( - ConnectionTimeoutError, InvalidPortError, PortInUseError, )
40-62: The@patch("socket.socket")decorator is unnecessary.The
mock_socket_classparameter is unused because the test manually assignsipc.sock = mock_sock. Either remove the decorator or use the patched socket class to createmock_sock.♻️ Option 1: Remove the decorator
- `@patch`("socket.socket") - def test_send_action(self, mock_socket_class): + def test_send_action(self): """Test sending action through IPC.""" logger = Mock(spec=CsvLogger) initial_env = Mock(spec=InitialEnvironmentMessage)♻️ Option 2: Use the patched class
`@patch`("socket.socket") def test_send_action(self, mock_socket_class): """Test sending action through IPC.""" logger = Mock(spec=CsvLogger) initial_env = Mock(spec=InitialEnvironmentMessage) ipc = SocketIPC(logger, initial_env, port=8000, find_free_port=False) - # Mock socket - mock_sock = MagicMock() - ipc.sock = mock_sock + # Use the patched socket class + mock_sock = mock_socket_class.return_value + ipc.sock = mock_sock
64-83: Same issue: unusedmock_socket_classparameter.Apply the same fix as suggested for
test_send_action.
| if port > MAX_PORT: | ||
| raise PortInUseError( | ||
| f"Could not find available port starting from {port}" | ||
| ) |
There was a problem hiding this comment.
Incorrect port value in error message.
When the error is raised, port has already been incremented past MAX_PORT, making the message misleading. The original starting port should be preserved.
Proposed fix
def check_port(self, port: int) -> int:
+ original_port = port
# ... validation code ...
if os.name == "nt":
while True:
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
if s.connect_ex(("127.0.0.1", port)) == 0:
if self.find_free_port:
print(...)
port += 1
if port > MAX_PORT:
raise PortInUseError(
- f"Could not find available port starting from {port}"
+ f"Could not find available port starting from {original_port}"
)🧰 Tools
🪛 Ruff (0.14.13)
78-80: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@src/craftground/environment/socket_ipc.py` around lines 77 - 80, The error
message uses the incremented port value instead of the original starting port;
capture the initial starting port (e.g., start_port = port) before the loop that
increments port and then, when raising PortInUseError, use that preserved
start_port in the message (keep the PortInUseError raise site and MAX_PORT check
unchanged but swap the f-string to reference start_port).
| if port > MAX_PORT: | ||
| raise PortInUseError( | ||
| f"Could not find available port starting from {port}" | ||
| ) |
There was a problem hiding this comment.
Same error message issue with incremented port.
Similar to the Windows branch, port has already been incremented past MAX_PORT when this error is raised, making the "starting from" message inaccurate.
🧰 Tools
🪛 Ruff (0.14.13)
99-101: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@src/craftground/environment/socket_ipc.py` around lines 98 - 101, The
PortInUseError message is using the already-incremented port value, making
"starting from {port}" inaccurate; change the check in the loop that raises
PortInUseError (where MAX_PORT and port are referenced) to report the original
starting port (store the initial port in a variable like start_port before
incrementing) and use that start_port in the PortInUseError message so it
matches the actual requested start value (reference the port variable, MAX_PORT,
and PortInUseError in your fix).
| if next_output > MAX_CONNECTION_RETRIES: | ||
| raise ConnectionTimeoutError( | ||
| f"Server not started within {MAX_CONNECTION_RETRIES} seconds" | ||
| ) |
There was a problem hiding this comment.
Missing exception chaining and premature timeout.
- Per static analysis (B904), use
raise ... fromto preserve the exception chain. - The
next_output > MAX_CONNECTION_RETRIEScheck may trigger before the mainwait_time < MAX_CONNECTION_RETRIEScondition, causing premature timeout at 64 seconds (if MAX_CONNECTION_RETRIES is 60) rather than the intended retry count.
Proposed fix for exception chaining
except (ConnectionRefusedError, FileNotFoundError) as e:
if wait_time == next_output:
print(...)
next_output *= 2
if next_output > MAX_CONNECTION_RETRIES:
raise ConnectionTimeoutError(
f"Server not started within {MAX_CONNECTION_RETRIES} seconds"
- )
+ ) from eConsider whether the next_output > MAX_CONNECTION_RETRIES check is intentional or if it should be removed to let the main loop condition handle timeouts.
🧰 Tools
🪛 Ruff (0.14.13)
282-284: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
282-284: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@src/craftground/environment/socket_ipc.py` around lines 281 - 284, The
premature timeout comes from the extra next_output > MAX_CONNECTION_RETRIES
check and the raised ConnectionTimeoutError lacks exception chaining; update the
logic in socket_ipc.py (look for next_output, wait_time, MAX_CONNECTION_RETRIES,
and ConnectionTimeoutError) by removing the redundant next_output >
MAX_CONNECTION_RETRIES branch so the loop relies solely on wait_time <
MAX_CONNECTION_RETRIES for termination, and when you do raise
ConnectionTimeoutError include the original exception with "raise
ConnectionTimeoutError(...) from err" (capture the original exception variable
where the wait/connection error occurred) to preserve the exception chain.
There was a problem hiding this comment.
Pull request overview
This PR refactors the CraftGround codebase by introducing better error handling, extracting constants, improving code organization, and adding validation. The refactoring focuses on making the code more maintainable and testable while preserving functionality.
Changes:
- Introduced a new exceptions module with custom exception classes for better error categorization
- Extracted magic numbers into a constants module for better maintainability
- Added validation for image sizes and port numbers in configuration
- Refactored process termination logic into separate platform-specific methods
- Simplified action_space conversion using loops instead of repetitive assignments
- Added comprehensive docstrings to public methods
- Added new unit tests for IPC and environment lifecycle
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/craftground/exceptions.py | New file defining custom exception hierarchy for CraftGround errors |
| src/craftground/constants.py | New file centralizing magic numbers and configuration constants |
| src/craftground/initial_environment_config.py | Added validation for image dimensions using new constants and exceptions |
| src/craftground/environment/socket_ipc.py | Enhanced port validation, improved error handling, and added comprehensive docstrings |
| src/craftground/environment/environment.py | Refactored process termination into platform-specific methods with better error handling |
| src/craftground/environment/action_space.py | Simplified action conversion logic using loops and .get() with defaults |
| tests/python/unit/test_ipc_mock.py | New test file for IPC functionality with mocking |
| tests/python/unit/test_environment_lifecycle.py | New test file for environment creation and configuration validation |
| src/craftground/MinecraftEnv/.gitignore | Added minecraft_sources to ignore list |
| .gitignore | Added CODE_REVIEW.md to ignore list |
Comments suppressed due to low confidence (1)
src/craftground/environment/socket_ipc.py:86
- The Windows port checking logic uses an infinite loop (while True) that only exits when a port is available or an exception is raised. If find_free_port is True and all ports from the starting port to MAX_PORT are in use, the exception is raised correctly. However, the loop could be made clearer by adding an explicit condition like 'while port <= MAX_PORT' to make the termination condition more obvious.
while True:
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
if s.connect_ex(("127.0.0.1", port)) == 0: # The port is in use
if self.find_free_port:
print(
f"[Warning]: Port {port} is already in use. Trying another port."
)
port += 1
if port > MAX_PORT:
raise PortInUseError(
f"Could not find available port starting from {port}"
)
else:
raise PortInUseError(
f"Port {port} is already in use. Please choose another port."
)
else:
return port
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setattr(action_space, action_name, action_v2.get(action_name, False)) | ||
|
|
||
| # Hotbar actions (1-9) | ||
| for i in range(1, 10): | ||
| hotbar_key = f"hotbar.{i}" | ||
| hotbar_attr = f"hotbar_{i}" | ||
| setattr(action_space, hotbar_attr, action_v2.get(hotbar_key, False)) |
There was a problem hiding this comment.
The refactored code changes the behavior when action keys are missing from the input dictionary. The original code would raise a KeyError if a required action key was missing, while the new code silently defaults to False for missing keys. This could hide bugs where actions are incorrectly specified. Consider whether this is intentional or if the original error-raising behavior should be preserved for required keys.
| setattr(action_space, action_name, action_v2.get(action_name, False)) | |
| # Hotbar actions (1-9) | |
| for i in range(1, 10): | |
| hotbar_key = f"hotbar.{i}" | |
| hotbar_attr = f"hotbar_{i}" | |
| setattr(action_space, hotbar_attr, action_v2.get(hotbar_key, False)) | |
| setattr(action_space, action_name, action_v2[action_name]) | |
| # Hotbar actions (1-9) | |
| for i in range(1, 10): | |
| hotbar_key = f"hotbar.{i}" | |
| hotbar_attr = f"hotbar_{i}" | |
| setattr(action_space, hotbar_attr, action_v2[hotbar_key]) |
| raise PortInUseError( | ||
| f"Socket file {socket_path} already exists. Please choose another port." |
There was a problem hiding this comment.
The existing test in test_socket_ipc.py (line 39) expects FileExistsError but the code now raises PortInUseError. This is a breaking change that makes the existing test fail. The test needs to be updated to expect PortInUseError instead of FileExistsError.
|
|
||
| if port > MAX_PORT: | ||
| raise PortInUseError( | ||
| f"Could not find available port starting from {port}" |
There was a problem hiding this comment.
The error message is similar to line 79 - when port exceeds MAX_PORT, the message will say "Could not find available port starting from {port}" where port is beyond the valid range. Consider capturing the original port value or adjusting the message to be more accurate.
| ConnectionTimeoutError: If connection times out | ||
| RuntimeError: If server process fails to start | ||
| """ | ||
| wait_time = 0 |
There was a problem hiding this comment.
The variable 'wait_time' is initialized to 0 and counts the number of retries, but the constant MAX_CONNECTION_RETRIES is set to 1024 seconds. The comment on line 258 describes next_output as tracking doubling intervals (1, 2, 4, 8... seconds), which suggests time-based progression, but wait_time is actually counting iterations. Consider renaming wait_time to retry_count or elapsed_seconds for clarity, or adjust the logic to match the semantics.
|
|
||
| # Connection settings | ||
| MAX_CONNECTION_RETRIES = 1024 | ||
| CONNECTION_RETRY_INTERVAL = 1.0 |
There was a problem hiding this comment.
CONNECTION_RETRY_INTERVAL is defined but never used in the codebase. The retry logic in socket_ipc.py uses time.sleep(1) directly instead of this constant. Consider either using this constant in the sleep call (line 286 of socket_ipc.py) or removing it if it's not needed.
| CONNECTION_RETRY_INTERVAL = 1.0 |
| port += 1 | ||
| if port > MAX_PORT: | ||
| raise PortInUseError( | ||
| f"Could not find available port starting from {port}" |
There was a problem hiding this comment.
The error message is misleading. When the port reaches MAX_PORT, the error message will say "Could not find available port starting from {port}" where port is already MAX_PORT + 1 (beyond the valid range). Consider capturing the original port value before incrementing, or adjust the message to reflect that all ports in the valid range have been exhausted.
| def test_check_port_validation(self): | ||
| """Test port number validation.""" | ||
| logger = Mock(spec=CsvLogger) | ||
| initial_env = Mock(spec=InitialEnvironmentMessage) | ||
|
|
||
| # Invalid port (too low) | ||
| with pytest.raises(InvalidPortError): | ||
| SocketIPC(logger, initial_env, port=0, find_free_port=False) | ||
|
|
||
| # Invalid port (too high) | ||
| with pytest.raises(InvalidPortError): | ||
| SocketIPC(logger, initial_env, port=70000, find_free_port=False) | ||
|
|
||
| # Invalid type | ||
| with pytest.raises(TypeError): | ||
| SocketIPC(logger, initial_env, port="8000", find_free_port=False) | ||
|
|
||
| @patch("socket.socket") | ||
| def test_send_action(self, mock_socket_class): | ||
| """Test sending action through IPC.""" | ||
| logger = Mock(spec=CsvLogger) | ||
| initial_env = Mock(spec=InitialEnvironmentMessage) | ||
| ipc = SocketIPC(logger, initial_env, port=8000, find_free_port=False) |
There was a problem hiding this comment.
The SocketIPC constructor calls remove_orphan_java_processes() which iterates over processes and may perform system calls. In unit tests, this should be mocked to avoid side effects and dependencies on the system state. Consider adding @patch('craftground.environment.socket_ipc.SocketIPC.remove_orphan_java_processes') to these test methods.
| import pytest | ||
|
|
||
| from craftground.buffered_socket import BufferedSocket | ||
| from craftground.csv_logger import CsvLogger, LogBackend |
There was a problem hiding this comment.
LogBackend is imported but never used in this test file. Consider removing this unused import to keep the code clean.
| from craftground.csv_logger import CsvLogger, LogBackend | |
| from craftground.csv_logger import CsvLogger |
| from craftground.buffered_socket import BufferedSocket | ||
| from craftground.csv_logger import CsvLogger, LogBackend | ||
| from craftground.environment.socket_ipc import SocketIPC | ||
| from craftground.exceptions import ( | ||
| ConnectionTimeoutError, | ||
| InvalidPortError, | ||
| PortInUseError, | ||
| ) | ||
| from craftground.proto.action_space_pb2 import ActionSpaceMessageV2 | ||
| from craftground.proto.initial_environment_pb2 import InitialEnvironmentMessage | ||
| from craftground.proto.observation_space_pb2 import ObservationSpaceMessage |
There was a problem hiding this comment.
Several imports (BufferedSocket, ConnectionTimeoutError, ObservationSpaceMessage) are imported but never used in this test file. Consider removing these unused imports to keep the code clean.
| from craftground.buffered_socket import BufferedSocket | |
| from craftground.csv_logger import CsvLogger, LogBackend | |
| from craftground.environment.socket_ipc import SocketIPC | |
| from craftground.exceptions import ( | |
| ConnectionTimeoutError, | |
| InvalidPortError, | |
| PortInUseError, | |
| ) | |
| from craftground.proto.action_space_pb2 import ActionSpaceMessageV2 | |
| from craftground.proto.initial_environment_pb2 import InitialEnvironmentMessage | |
| from craftground.proto.observation_space_pb2 import ObservationSpaceMessage | |
| from craftground.csv_logger import CsvLogger, LogBackend | |
| from craftground.environment.socket_ipc import SocketIPC | |
| from craftground.exceptions import ( | |
| InvalidPortError, | |
| PortInUseError, | |
| ) | |
| from craftground.proto.action_space_pb2 import ActionSpaceMessageV2 | |
| from craftground.proto.initial_environment_pb2 import InitialEnvironmentMessage |
| import threading | ||
| from typing import Tuple, Optional, TypedDict, Union, List, Any, Dict | ||
| from enum import Enum | ||
| from typing import Any, Dict, List, Optional, Tuple, TypedDict, Union |
There was a problem hiding this comment.
Import of 'Any' is not used.
| from typing import Any, Dict, List, Optional, Tuple, TypedDict, Union | |
| from typing import Dict, List, Optional, Tuple, TypedDict, Union |
Pull
Summary by CodeRabbit
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.