feat:shared_utils #324
feat:shared_utils #324
Conversation
gather some basic util methods from across our repos that were flagged with a TODO reorganize functions dumped at the top level module into their own files by topic for easier maintenance
WalkthroughThis pull request introduces significant refactoring of the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
ovos_utils/list_utils.py (2)
4-4: Improve parameter naming.The parameter name
lis ambiguous and could be confused with the number 1. Consider using a more descriptive name.-def rotate_list(l: List[Any], n: int = 1) -> List[Any]: +def rotate_list(lst: List[Any], n: int = 1) -> List[Any]:🧰 Tools
🪛 Ruff (0.8.2)
4-4: Ambiguous variable name:
l(E741)
35-35: Replace lambda with a regular function definition.Using lambda assignments is against Python style guidelines. Convert it to a regular function for better readability.
- _flatten = lambda l: [item for sublist in l for item in sublist] + def _flatten(lst): + return [item for sublist in lst for item in sublist]🧰 Tools
🪛 Ruff (0.8.2)
35-35: Do not assign a
lambdaexpression, use adefRewrite
_flattenas adef(E731)
35-35: Ambiguous variable name:
l(E741)
ovos_utils/text_utils.py (1)
31-42: Consider handling nested parentheses.The current regex pattern doesn't handle nested parentheses correctly. Consider using a more robust approach.
- return re.sub(r"\((.*?)\)", "", text).replace(" ", " ") + def remove_parentheses(text): + stack = [] + start = -1 + result = list(text) + for i, char in enumerate(text): + if char == '(': + if not stack: + start = i + stack.append(i) + elif char == ')' and stack: + stack.pop() + if not stack: + for j in range(start, i + 1): + result[j] = '' + return collapse_whitespaces(''.join(result)) + return remove_parentheses(text)ovos_utils/__init__.py (1)
29-35: Consider adding migration guidance in deprecation warnings.The deprecation warning could be more helpful by suggesting alternative approaches or explaining why the functionality is being removed.
warnings.warn( - "deprecated without replacement and will be removed in a future release.", + "The create_loop function is deprecated and will be removed in version 2.0.0. " + "Consider using asyncio.create_task or threading.Timer for similar functionality.", DeprecationWarning, stacklevel=2, )ovos_utils/thread_utils.py (4)
9-51: Improve thread cleanup and resource management.While the implementation is functional, consider these improvements for better resource management:
- The thread continues running in the background even after timeout, which could lead to resource leaks.
- No cleanup is performed for thread resources.
Consider this enhanced implementation:
def threaded_timeout(timeout: int = 5) -> Callable: def deco(func: Callable) -> Callable: @wraps(func) def wrapper(*args: Any, **kwargs: Any) -> Any: res = [Exception(f'function [{func.__name__}] timeout ' f'[{timeout}] exceeded!')] + stop_event = Event() def func_wrapped(): try: res[0] = func(*args, **kwargs) except Exception as e: res[0] = e + finally: + stop_event.set() t = Thread(target=func_wrapped) t.daemon = True try: t.start() - t.join(timeout) + stop_event.wait(timeout) + if not stop_event.is_set(): + LOG.warning(f"Function {func.__name__} timed out after {timeout}s") except Exception as je: raise je ret = res[0] if isinstance(ret, BaseException): raise ret return ret return wrapper return decoThis implementation:
- Uses an
Eventto properly signal completion- Logs a warning when timeout occurs
- Provides better cleanup of thread resources
54-74: Add input validation for better error handling.The function would benefit from input validation to prevent runtime errors.
Consider this enhanced implementation:
def create_killable_daemon(target: Callable, args: tuple = (), kwargs: Optional[dict] = None, autostart: bool = True) -> kthread.KThread: + if not callable(target): + raise TypeError("target must be a callable") + if not isinstance(args, tuple): + raise TypeError("args must be a tuple") + kwargs = kwargs or {} t = kthread.KThread(target=target, args=args, kwargs=kwargs) t.daemon = True if autostart: t.start() return tThis implementation:
- Validates that target is callable
- Ensures args is a tuple
- Safely handles None kwargs
77-97: Reduce code duplication with create_killable_daemon.The function shares significant code with
create_killable_daemon. Consider extracting common logic to reduce duplication.Consider this refactored implementation:
+def _validate_thread_args(target: Callable, args: tuple, kwargs: Optional[dict]) -> dict: + if not callable(target): + raise TypeError("target must be a callable") + if not isinstance(args, tuple): + raise TypeError("args must be a tuple") + return kwargs or {} + def create_daemon(target: Callable, args: tuple = (), kwargs: Optional[dict] = None, autostart: bool = True) -> Thread: + kwargs = _validate_thread_args(target, args, kwargs) t = Thread(target=target, args=args, kwargs=kwargs) t.daemon = True if autostart: t.start() return tAnd update
create_killable_daemon:def create_killable_daemon(target: Callable, args: tuple = (), kwargs: Optional[dict] = None, autostart: bool = True) -> kthread.KThread: + kwargs = _validate_thread_args(target, args, kwargs) t = kthread.KThread(target=target, args=args, kwargs=kwargs) t.daemon = True if autostart: t.start() return tThis refactoring:
- Extracts common validation logic
- Reduces code duplication
- Ensures consistent behavior across both functions
100-114: Enhance signal handling and fix f-string.The function could benefit from handling additional termination signals, and there's an unnecessary f-string in the log message.
Consider this enhanced implementation:
+import signal + def wait_for_exit_signal() -> None: + exit_event = Event() + + def signal_handler(signum, frame): + LOG.debug("Exiting on signal %s", signal.Signals(signum).name) + exit_event.set() + + signal.signal(signal.SIGTERM, signal_handler) + signal.signal(signal.SIGINT, signal_handler) + try: - Event().wait() + exit_event.wait() except KeyboardInterrupt: - LOG.debug(f"Exiting on KeyboardInterrupt") + LOG.debug("Exiting on KeyboardInterrupt")This implementation:
- Handles both SIGTERM and SIGINT signals
- Uses a shared Event for coordinated exit
- Removes unnecessary f-string
- Uses proper string formatting in log messages
🧰 Tools
🪛 Ruff (0.8.2)
114-114: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ovos_utils/__init__.py(2 hunks)ovos_utils/decorators.py(1 hunks)ovos_utils/list_utils.py(1 hunks)ovos_utils/text_utils.py(1 hunks)ovos_utils/thread_utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_utils/list_utils.py
4-4: Ambiguous variable name: l
(E741)
35-35: Do not assign a lambda expression, use a def
Rewrite _flatten as a def
(E731)
35-35: Ambiguous variable name: l
(E741)
ovos_utils/thread_utils.py
114-114: f-string without any placeholders
Remove extraneous f prefix
(F541)
ovos_utils/__init__.py
17-17: ovos_utils.decorators.classproperty imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: ovos_utils.decorators.timed_lru_cache imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
18-18: ovos_utils.list_utils.flatten_list imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
18-18: ovos_utils.list_utils.rotate_list imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
19-19: ovos_utils.log.LOG imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
20-20: ovos_utils.text_utils.camel_case_split imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: ovos_utils.thread_utils.wait_for_exit_signal imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: ovos_utils.thread_utils.threaded_timeout imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: ovos_utils.thread_utils.create_killable_daemon imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: license_tests
🔇 Additional comments (5)
ovos_utils/list_utils.py (1)
45-62: LGTM! Well-implemented deduplication with order preservation option.The implementation is clean and efficient, offering flexibility with the
keep_orderparameter.ovos_utils/text_utils.py (1)
43-58: LGTM! Well-implemented Unicode normalization.The function correctly handles Unicode normalization and preserves necessary punctuation marks.
ovos_utils/__init__.py (1)
17-21: LGTM! Re-exporting symbols from submodules.The imports are used for re-exporting symbols, making them available at the package level.
🧰 Tools
🪛 Ruff (0.8.2)
17-17:
ovos_utils.decorators.classpropertyimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
17-17:
ovos_utils.decorators.timed_lru_cacheimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
18-18:
ovos_utils.list_utils.flatten_listimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
18-18:
ovos_utils.list_utils.rotate_listimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
19-19:
ovos_utils.log.LOGimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
20-20:
ovos_utils.text_utils.camel_case_splitimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
21-21:
ovos_utils.thread_utils.wait_for_exit_signalimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
21-21:
ovos_utils.thread_utils.threaded_timeoutimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
21-21:
ovos_utils.thread_utils.create_killable_daemonimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
ovos_utils/decorators.py (2)
6-20: LGTM! Well-implemented classproperty decorator.The implementation correctly handles class-level properties with proper type hints and documentation.
23-71: LGTM! Robust implementation of timed LRU cache.The implementation properly handles:
- Optional decorator arguments
- Cache expiration using monotonic time
- Preservation of original cache management methods
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ovos_utils/thread_utils.py (4)
11-55: Review:threaded_timeoutDecorator Implementation
The implementation uses a mutable list (res) to capture the result or exception from the function call, which is an effective pattern for inner-function communication. However, note that the docstring states that "the thread is terminated" if the timeout is exceeded. In reality, when using a standardThread, there is no built-in mechanism to kill the thread—the thread will continue running in the background even after a timeout. Consider updating the docstring to clarify this behavior or switching to a killable thread (e.g., usingkthread) if termination is required.Example diff to update the docstring:
- If the timeout is exceeded, an exception is raised and the thread is terminated. + If the timeout is exceeded, an exception is raised. Note: The underlying thread is not forcefully terminated and may continue running in the background.
57-78: Review:create_killable_daemonFunction
This helper function correctly leverageskthread.KThreadto create a daemon thread with an option to autostart. One improvement suggestion is to ensure that thekwargsparameter is always a dictionary. Ifkwargsis passed asNone, it may be preferable to default it to an empty dictionary before passing it to the thread constructor.Example diff:
-def create_killable_daemon(target: Callable, args: tuple = (), kwargs: Optional[dict] = None, - autostart: bool = True) -> kthread.KThread: - t = kthread.KThread(target=target, args=args, kwargs=kwargs) +def create_killable_daemon(target: Callable, args: tuple = (), kwargs: Optional[dict] = None, + autostart: bool = True) -> kthread.KThread: + t = kthread.KThread(target=target, args=args, kwargs=kwargs or {})
81-101: Review:create_daemonFunction
This function mirrors the logic ofcreate_killable_daemonfor standard daemon threads. Similar to the previous function, consider handling thekwargsparameter to default to an empty dictionary ifNoneis provided. This can help avoid potential issues if the underlyingThreadconstructor does not handle aNonevalue gracefully.Example diff:
-def create_daemon(target: Callable, args: tuple = (), kwargs: Optional[dict] = None, autostart: bool = True) -> Thread: - t = Thread(target=target, args=args, kwargs=kwargs) +def create_daemon(target: Callable, args: tuple = (), kwargs: Optional[dict] = None, autostart: bool = True) -> Thread: + t = Thread(target=target, args=args, kwargs=kwargs or {})
104-127: Review:wait_for_exit_signalFunction
Thewait_for_exit_signalfunction is well implemented for gracefully blocking execution until a termination signal is received. The use of anEventand a custom signal handler is appropriate. One minor suggestion is to consider restoring the original signal handlers after the event is set if other parts of the application rely on them, though this might be unnecessary depending on the overall program design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_utils/thread_utils.py(1 hunks)
🔇 Additional comments (1)
ovos_utils/thread_utils.py (1)
1-10: Review: Imports and Module Dependencies
The import statements are clear and include all necessary modules for the threading utilities. One minor suggestion is to ensure that the external dependencykthreadis available in the environment or to document its installation as it is vital for the killable daemon functionality.
gather some basic util methods from across our repos that were flagged with a TODO
reorganize functions dumped at the top level module into their own files by topic for easier maintenance
Summary by CodeRabbit
Release Notes
Refactor
New Features
rotate_list,flatten_list, anddeduplicate_listcamel_case_split,collapse_whitespaces, andremove_accents_and_punctDeprecation
Performance
classpropertyandtimed_lru_cache