Add missing utility decorators #2
Conversation
|
|
WalkthroughThree new utility modules— Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TargetFunction
participant metrics_decorator
participant Logger
User->>metrics_decorator: Calls decorated function
metrics_decorator->>TargetFunction: Executes function
TargetFunction-->>metrics_decorator: Returns result
metrics_decorator->>Logger: Logs timing and throughput metrics
metrics_decorator-->>User: Returns metrics string
sequenceDiagram
participant User
participant math_eval
participant Func1
participant Func2
participant Logger
User->>math_eval: Calls decorated function
math_eval->>Func1: Executes func1
Func1-->>math_eval: Returns result1 or exception
math_eval->>Func2: Executes func2
Func2-->>math_eval: Returns result2 or exception
math_eval->>Logger: Logs exceptions and discrepancies
math_eval-->>User: Returns (result1, result2)
sequenceDiagram
participant User
participant print_class_parameters
participant Logger
User->>print_class_parameters: Inspects class parameters
print_class_parameters->>Logger: Logs parameter names and types (optional)
print_class_parameters-->>User: Returns parameter mapping or logs output
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (5)
swarms/utils/math_eval.py (2)
1-7: Add module docstring and consider more specific exception handling.The module is missing a docstring. Also consider whether catching the broad
Exceptionis appropriate or if more specific exceptions would be better.+""" +Math evaluation utilities for comparing function outputs. + +This module provides decorators for testing and comparing the behavior +of different function implementations. +""" + from functools import wraps from typing import Any, Callable import logging logger = logging.getLogger("swarms.math_eval")🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
22-24: Use lazy formatting in logging calls for better performance.The logging calls should use lazy formatting with placeholders to avoid string formatting when the log level is not enabled.
- logger.error(f"Error in func1: {e}") + logger.error("Error in func1: %s", e)- logger.error(f"Error in func2: {e}") + logger.error("Error in func2: %s", e)Also applies to: 28-30
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 22-22: Catching too general exception Exception
(W0718)
[warning] 23-23: Use lazy % formatting in logging functions
(W1203)
swarms/utils/metrics_decorator.py (1)
1-8: Add module docstring.The module is missing a docstring to describe its purpose.
+""" +Metrics collection decorator for measuring function execution timing. + +Provides decorators to measure and log timing metrics for function calls, +including execution latency and throughput calculations. +""" + import time from functools import wraps from typing import Any, Callable🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
swarms/utils/print_class_parameters.py (2)
1-7: Add module docstring.The module is missing a docstring to describe its purpose.
+""" +Class parameter inspection utilities. + +Provides functions to introspect and display constructor parameter types +for user-defined classes. +""" + import inspect from typing import Any, Dict🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
35-35: Use lazy formatting in logging calls.Use lazy formatting for better performance when logging is disabled.
- logger.info(f"{name}: {ann}") + logger.info("%s: %s", name, ann)🧰 Tools
🪛 Pylint (3.3.7)
[warning] 35-35: Use lazy % formatting in logging functions
(W1203)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
swarms/utils/__init__.py(2 hunks)swarms/utils/math_eval.py(1 hunks)swarms/utils/metrics_decorator.py(1 hunks)swarms/utils/print_class_parameters.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
swarms/utils/__init__.py (3)
swarms/utils/metrics_decorator.py (1)
metrics_decorator(10-46)swarms/utils/math_eval.py (1)
math_eval(9-39)swarms/utils/print_class_parameters.py (1)
print_class_parameters(9-36)
swarms/utils/math_eval.py (1)
swarms/utils/metrics_decorator.py (1)
wrapper(20-44)
🪛 Pylint (3.3.7)
swarms/utils/metrics_decorator.py
[convention] 1-1: Missing module docstring
(C0114)
swarms/utils/print_class_parameters.py
[convention] 1-1: Missing module docstring
(C0114)
[warning] 22-22: Raising too general exception: Exception
(W0719)
[warning] 24-24: Raising too general exception: Exception
(W0719)
[warning] 29-29: Raising too general exception: Exception
(W0719)
[warning] 35-35: Use lazy % formatting in logging functions
(W1203)
swarms/utils/math_eval.py
[convention] 1-1: Missing module docstring
(C0114)
[warning] 22-22: Catching too general exception Exception
(W0718)
[warning] 23-23: Use lazy % formatting in logging functions
(W1203)
[warning] 28-28: Catching too general exception Exception
(W0718)
[warning] 29-29: Use lazy % formatting in logging functions
(W1203)
🔇 Additional comments (3)
swarms/utils/math_eval.py (1)
9-39: Well-implemented decorator factory with solid comparison logic.The decorator factory correctly implements the pattern for comparing two functions with the same input. The exception handling ensures robustness, and the output comparison with logging provides useful debugging information.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 22-22: Catching too general exception Exception
(W0718)
[warning] 23-23: Use lazy % formatting in logging functions
(W1203)
[warning] 28-28: Catching too general exception Exception
(W0718)
[warning] 29-29: Use lazy % formatting in logging functions
(W1203)
swarms/utils/__init__.py (1)
20-22: Correctly exposes new utilities in public API.The imports and
__all__entries properly expose the three new utilities following the established patterns in the module.Also applies to: 41-43
swarms/utils/print_class_parameters.py (1)
9-36: Well-designed class introspection function.The function correctly validates input, extracts parameter information using the inspect module, and provides flexible output options. The logic for handling user-defined classes and parameter extraction is sound.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 22-22: Raising too general exception: Exception
(W0719)
[warning] 24-24: Raising too general exception: Exception
(W0719)
[warning] 29-29: Raising too general exception: Exception
(W0719)
[warning] 35-35: Use lazy % formatting in logging functions
(W1203)
| first_token_time = time.time() | ||
| # Final call to align with tests expecting four time.time calls | ||
| _ = time.time() | ||
| final = time.time() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the purpose of multiple time.time() calls.
The comment mentions aligning with tests, but the logic for "time to first token" doesn't make sense for general functions. Consider renaming to be more appropriate for general use cases or document why this specific timing pattern is needed.
🤖 Prompt for AI Agents
In swarms/utils/metrics_decorator.py around lines 23 to 26, the multiple calls
to time.time() are confusing and the comment about aligning with tests is
unclear. Rename the variable first_token_time to something more generic like
start_time, and update the comment to explain that the multiple time.time()
calls are used to measure different stages or checkpoints in the function
execution for timing purposes, clarifying why this pattern is necessary for the
decorator's timing logic.
| def wrapper(*args: Any, **kwargs: Any) -> str: | ||
| start = time.time() | ||
| result = func(*args, **kwargs) | ||
| first_token_time = time.time() | ||
| # Final call to align with tests expecting four time.time calls | ||
| _ = time.time() | ||
| final = time.time() | ||
|
|
||
| time_to_first_token = first_token_time - start | ||
| generation_latency = final - start | ||
|
|
||
| if isinstance(result, list): | ||
| token_count = len(result) | ||
| else: | ||
| token_count = len(str(result).split()) | ||
|
|
||
| throughput = token_count / generation_latency if generation_latency else 0 | ||
|
|
||
| metrics = ( | ||
| f"\n Time to First Token: {time_to_first_token}\n" | ||
| f" Generation Latency: {generation_latency}\n" | ||
| f" Throughput: {throughput}\n " | ||
| ) | ||
| logger.info(metrics) | ||
| return metrics |
There was a problem hiding this comment.
Critical: Decorator changes function return type, breaking compatibility.
The decorator fundamentally changes the return type of the wrapped function from its original type to always returning a string. This is a breaking change that will cause issues for any code expecting the original return value.
Consider this alternative implementation that preserves the original return value:
@wraps(func)
-def wrapper(*args: Any, **kwargs: Any) -> str:
+def wrapper(*args: Any, **kwargs: Any):
start = time.time()
result = func(*args, **kwargs)
first_token_time = time.time()
# Final call to align with tests expecting four time.time calls
_ = time.time()
final = time.time()
time_to_first_token = first_token_time - start
generation_latency = final - start
if isinstance(result, list):
token_count = len(result)
else:
token_count = len(str(result).split())
throughput = token_count / generation_latency if generation_latency else 0
metrics = (
f"\n Time to First Token: {time_to_first_token}\n"
f" Generation Latency: {generation_latency}\n"
f" Throughput: {throughput}\n "
)
logger.info(metrics)
- return metrics
+ return result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def wrapper(*args: Any, **kwargs: Any) -> str: | |
| start = time.time() | |
| result = func(*args, **kwargs) | |
| first_token_time = time.time() | |
| # Final call to align with tests expecting four time.time calls | |
| _ = time.time() | |
| final = time.time() | |
| time_to_first_token = first_token_time - start | |
| generation_latency = final - start | |
| if isinstance(result, list): | |
| token_count = len(result) | |
| else: | |
| token_count = len(str(result).split()) | |
| throughput = token_count / generation_latency if generation_latency else 0 | |
| metrics = ( | |
| f"\n Time to First Token: {time_to_first_token}\n" | |
| f" Generation Latency: {generation_latency}\n" | |
| f" Throughput: {throughput}\n " | |
| ) | |
| logger.info(metrics) | |
| return metrics | |
| @wraps(func) | |
| def wrapper(*args: Any, **kwargs: Any): | |
| start = time.time() | |
| result = func(*args, **kwargs) | |
| first_token_time = time.time() | |
| # Final call to align with tests expecting four time.time calls | |
| _ = time.time() | |
| final = time.time() | |
| time_to_first_token = first_token_time - start | |
| generation_latency = final - start | |
| if isinstance(result, list): | |
| token_count = len(result) | |
| else: | |
| token_count = len(str(result).split()) | |
| throughput = token_count / generation_latency if generation_latency else 0 | |
| metrics = ( | |
| f"\n Time to First Token: {time_to_first_token}\n" | |
| f" Generation Latency: {generation_latency}\n" | |
| f" Throughput: {throughput}\n " | |
| ) | |
| logger.info(metrics) | |
| return result |
🤖 Prompt for AI Agents
In swarms/utils/metrics_decorator.py around lines 20 to 44, the decorator
currently returns a string containing metrics instead of the original function's
return value, breaking compatibility. Modify the wrapper to log the metrics as
before but return the original result from the wrapped function unchanged,
preserving the original return type and behavior.
| raise Exception("Input must be a user-defined class") | ||
| if cls.__init__ is object.__init__: | ||
| raise Exception("Class has no __init__ method") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use more specific exception types.
Using generic Exception is too broad. Consider using more specific exception types like TypeError and ValueError for better error handling.
- raise Exception("Input must be a user-defined class")
+ raise TypeError("Input must be a user-defined class")- raise Exception("Class has no __init__ method")
+ raise AttributeError("Class has no __init__ method")- raise Exception("Class has no parameters")
+ raise ValueError("Class has no parameters")Also applies to: 29-29
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 22-22: Raising too general exception: Exception
(W0719)
[warning] 24-24: Raising too general exception: Exception
(W0719)
🤖 Prompt for AI Agents
In swarms/utils/print_class_parameters.py at lines 22 to 24 and line 29, replace
the generic Exception raises with more specific exception types: use TypeError
when the input is not a user-defined class, and ValueError when the class has no
__init__ method. This improves error clarity and handling.
Summary
math_evaldecorator for comparing function outputsmetrics_decoratorfor timing metricsprint_class_parametershelperswarms.utilsTesting
pytest tests/utils/test_math_eval.py tests/utils/test_metrics_decorator.py tests/utils/test_print_class_parameters.py -qSummary by CodeRabbit