Making big modules 'optional' for Docker#209
Conversation
There was a problem hiding this comment.
Pull request overview
Makes certain heavyweight/optional dependencies import-tolerant so the core framework can load in minimal environments (e.g., Docker) or newer Python versions (3.13+).
Changes:
- Guarded
telnetlibimport to accommodate Python 3.13 removal. - Made
capture(cv2/tesseract/PIL) andwebpageController(selenium) optional imports inframework.corepackage exports.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| framework/core/testControl.py | Wrapes telnetlib import in a try/except for Python 3.13 compatibility. |
| framework/core/init.py | Wraps imports of heavier modules and falls back to None when dependencies are missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR aims to make certain heavyweight/optional dependencies non-fatal at import time (notably for Docker/minimal environments), and adds a compatibility guard for telnetlib removal in newer Python versions.
Changes:
- Wrap
telnetlibimport inframework/core/testControl.pyto tolerate Python versions where it’s unavailable. - Make
capture(cv2/pytesseract/PIL) andwebpageController(selenium) optional exports fromframework.coreby guarding imports inframework/core/__init__.py.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| framework/core/testControl.py | Adds a guarded import for telnetlib to avoid import-time failure when the stdlib module is missing. |
| framework/core/init.py | Guards importing capture and webpageController so importing framework.core can succeed without optional heavy deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR aims to make heavyweight/optional dependencies (image capture + Selenium UI control) non-fatal when running in minimal environments like Docker, and to tolerate telnetlib removal in newer Python versions.
Changes:
- Guard
telnetlibimport intestControl.pyfor Python versions where it may be absent. - Add optional-import behavior in
framework.corepackage exports forcaptureandwebpageController.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
framework/core/testControl.py |
Wraps telnetlib import in a try/except and falls back to None. |
framework/core/__init__.py |
Makes capture and webpageController imports optional to reduce dependency requirements at import time. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR aims to make heavyweight/optional dependencies (image capture, Selenium/web control, telnetlib) non-blocking for Docker/minimal installs, and improves console selection behavior by respecting an enabled flag.
Changes:
- Wrap imports of optional modules (
capture,webpageController,telnetlib) to allow running without those dependencies. - Add console selection improvements: skip disabled consoles and add fallback behavior when a requested console isn’t available.
- Logging improvements: set UTF-8 encoding for file handlers and adjust handler cleanup logic.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/core/testControl.py | Makes capture/webpageController imports optional; adds logic to select an enabled console |
| framework/core/logModule.py | Updates handler cleanup loop; uses UTF-8 encoded file handlers |
| framework/core/deviceManager.py | Skips disabled consoles and adds safe console-session fallback behavior |
| framework/core/commandModules/telnetClass.py | Adds a Python 3.13+ fallback shim for removed stdlib telnetlib |
| framework/core/commandModules/serialClass.py | Changes serial input-buffer clearing method |
| framework/core/init.py | Makes capture/webpageController exports resilient to missing optional dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| import telnetlib | ||
| except ModuleNotFoundError: | ||
| # telnetlib was removed in Python 3.13, provide a minimal compatibility layer | ||
| import socket | ||
| import time | ||
|
|
||
| class Telnet: | ||
| def __init__(self, host=None, port=23, timeout=socket._GLOBAL_DEFAULT_TIMEOUT): | ||
| self.host = host | ||
| self.port = port | ||
| self.timeout = timeout | ||
| self.sock = None | ||
| if host: | ||
| self.open(host, port, timeout) | ||
|
|
||
| def open(self, host, port=23, timeout=socket._GLOBAL_DEFAULT_TIMEOUT): | ||
| self.host = host | ||
| self.port = port | ||
| self.sock = socket.create_connection((host, port), timeout) | ||
|
|
||
| def close(self): | ||
| if self.sock: | ||
| self.sock.close() | ||
| self.sock = None | ||
|
|
||
| def write(self, buffer): | ||
| if self.sock: | ||
| self.sock.send(buffer) | ||
|
|
||
| def read_until(self, match, timeout=None): | ||
| if not self.sock: | ||
| return b'' | ||
|
|
||
| buffer = b'' | ||
| start_time = time.time() | ||
| while True: | ||
| if timeout and (time.time() - start_time) > timeout: | ||
| break | ||
| try: | ||
| data = self.sock.recv(1024) | ||
| if not data: | ||
| break | ||
| buffer += data | ||
| if match in buffer: | ||
| break | ||
| except socket.timeout: | ||
| break | ||
| except Exception: | ||
| break | ||
| return buffer | ||
|
|
||
| def read_all(self): | ||
| if not self.sock: | ||
| return b'' | ||
| buffer = b'' | ||
| try: | ||
| while True: | ||
| data = self.sock.recv(1024) | ||
| if not data: | ||
| break | ||
| buffer += data | ||
| except Exception: | ||
| pass | ||
| return buffer | ||
|
|
||
| # Create a mock telnetlib module | ||
| class telnetlib: | ||
| Telnet = Telnet | ||
|
|
There was a problem hiding this comment.
The Python 3.13 compatibility Telnet shim does not implement several methods that the rest of this file calls (read_eager, read_very_eager, read_some). In environments without stdlib telnetlib, telnet.read_eager() will raise AttributeError at runtime. Either implement the missing methods in the shim (matching the subset of the telnetlib API used here) or refactor the wrapper to avoid calling methods that aren't provided by the fallback.
| except ModuleNotFoundError: | ||
| webpageController = None # selenium not installed (e.g. Docker) |
There was a problem hiding this comment.
except ModuleNotFoundError: here will swallow any missing-module error while importing webpageController, potentially hiding real packaging bugs unrelated to Selenium. To avoid masking unexpected failures, catch ModuleNotFoundError as e and only treat it as optional when e.name == 'selenium' (and otherwise re-raise). Also consider providing a callable stub that raises a clear ImportError when used, rather than setting webpageController = None, to fail with a more actionable message.
| except ModuleNotFoundError: | |
| webpageController = None # selenium not installed (e.g. Docker) | |
| except ModuleNotFoundError as e: | |
| if e.name == "selenium": | |
| class _WebpageControllerMissingDependency: | |
| def __init__(self, *args, **kwargs): | |
| raise ImportError( | |
| "webpageController functionality is unavailable because the " | |
| "'selenium' package is not installed." | |
| ) from e | |
| webpageController = _WebpageControllerMissingDependency | |
| else: | |
| # Unexpected missing module (e.g., bug inside webpageController.py) - do not hide it. | |
| raise |
| while self.log.handlers: | ||
| self.log.removeHandler(self.log.handlers[0]) | ||
|
|
There was a problem hiding this comment.
In __del__, handlers are removed but never closed. For file handlers this can leak file descriptors until process exit (and __del__ is not guaranteed to run promptly). Consider calling handler.close() after removing each handler (and similarly for csvLogger handlers if applicable).
| while self.log.handlers: | |
| self.log.removeHandler(self.log.handlers[0]) | |
| # Properly remove and close all handlers associated with this logger | |
| try: | |
| # Close handlers attached to the main logger | |
| for handler in list(getattr(self.log, "handlers", [])): | |
| try: | |
| self.log.removeHandler(handler) | |
| finally: | |
| try: | |
| handler.close() | |
| except Exception: | |
| # Suppress all exceptions in __del__ | |
| pass | |
| # Close handlers attached to the CSV logger, if present | |
| csv_logger = getattr(self, "csvLogger", None) | |
| if csv_logger is not None: | |
| for handler in list(getattr(csv_logger, "handlers", [])): | |
| try: | |
| csv_logger.removeHandler(handler) | |
| finally: | |
| try: | |
| handler.close() | |
| except Exception: | |
| # Suppress all exceptions in __del__ | |
| pass | |
| except Exception: | |
| # Ensure __del__ never raises | |
| pass |
| """ | ||
| self.log.info("Clearing Serial console log") | ||
| self.serialCon.reset_input_buffer() | ||
| self.serialCon.flushInput() |
There was a problem hiding this comment.
flushInput() is deprecated in pyserial in favor of reset_input_buffer(). Since this repo pins pyserial==3.5, reset_input_buffer() should be available and is the preferred API; using the deprecated alias can introduce warnings and makes the intent less clear.
| self.serialCon.flushInput() | |
| self.serialCon.reset_input_buffer() |
| # Respect 'enabled: false' in console config — skip disabled consoles | ||
| if config.get("enabled") is False: | ||
| self.type = None | ||
| self.session = None | ||
| return |
There was a problem hiding this comment.
New behavior around enabled: false and console fallback would benefit from unit coverage (e.g., verify disabled consoles are excluded from self.consoles, and that getConsoleSession() falls back to the first enabled console when the requested one is missing/disabled). There are existing deviceManager unit tests under tests/singleton_tests/; extending them would help prevent regressions.
| if e.name == "selenium": | ||
| webpageController = None | ||
| else: | ||
| raise |
There was a problem hiding this comment.
capture and webpageController are set to None on missing optional deps, but later in __init__ they are called as constructors when captureConfig / webDriverConfig are present. This will raise a TypeError: 'NoneType' object is not callable instead of gracefully disabling the feature. Consider using a callable stub that raises a clear ImportError (similar to framework/core/__init__.py) and/or guarding instantiation with if capture is None: / if webpageController is None: to log and skip setup.
| raise | |
| raise | |
| # Provide callable stubs when optional dependencies are missing so that | |
| # attempts to instantiate these classes fail with a clear ImportError | |
| if capture is None: | |
| def _capture_missing(*args, **kwargs): | |
| raise ImportError( | |
| "Optional dependencies for 'capture' (cv2/pytesseract/PIL) are not " | |
| "installed; capture functionality is disabled." | |
| ) | |
| capture = _capture_missing | |
| if 'webpageController' in globals() and webpageController is None: | |
| def _webpage_controller_missing(*args, **kwargs): | |
| raise ImportError( | |
| "Optional dependency 'selenium' is not installed; webpageController " | |
| "functionality is disabled." | |
| ) | |
| webpageController = _webpage_controller_missing |
| console_name = "default" # Fallback to default | ||
| try: | ||
| # Get the raw config to check enabled status | ||
| dut_config = self.slotInfo.config.get("devices", [{}])[0].get("dut", {}) | ||
| consoles_config = dut_config.get("consoles", []) | ||
|
|
||
| # Find the enabled console | ||
| for console_item in consoles_config: | ||
| for name, config in console_item.items(): | ||
| if config.get("enabled", False): | ||
| console_name = name | ||
| self.log.info(f"Using enabled console: {console_name} (type: {config.get('type')})") | ||
| break | ||
| if console_name != "default": | ||
| break |
There was a problem hiding this comment.
The enabled-console detection only looks at self.slotInfo.config.get('devices', [{}])[0], so it will miss the DUT config if dut isn't the first entry in the devices list. In that case console_name may remain default even when another console is explicitly marked enabled, which undermines the purpose of the change. Consider deriving the enabled console from the DUT device config found by name (or directly from self.dut.consoles / self.dut.rawConfig) instead of indexing [0].
Optionality for Docker Potential fix for pull request finding Agreed Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Improve compatibility and add console enable/disable support - Add Python 3.13+ compatibility shim for removed telnetlib module - Use flushInput() instead of reset_input_buffer() in serialClass for broader pyserial compatibility - Support 'enabled: false' in console config to skip disabled consoles, with automatic fallback to the first enabled console - Make capture (cv2/pytesseract) and webpageController (selenium) imports optional to support minimal environments (e.g. Docker) - Add UTF-8 encoding to log file handlers - Fix logger cleanup to use self.log.handlers instead of hasHandlers() ModuleNotFoundError is finer grain
|
Closing - cleaner PR code pending |
No description provided.