refactor: separate metric definition from metric value and share comp…

[AumPatel1]

This PR addresses the TODO comment in metric.py by refactoring the Metric class to separate concerns and optimize memory usage.
Changes

• Separated metric definition from metric value: Added internal _MetricDefinition class to separate metric type (what it is) from metric value (a measurement)
• Comparator instance sharing: Implemented _get_shared_comparator() with WeakValueDictionary cache to ensure only one comparator object exists per unique (method, target, epsilon) combination
  -Memory optimization: All solutions using the same comparison method now share the same comparator instance, reducing memory usage
• Backward compatibility: Maintained full backward compatibility - no changes to Metric class public API

Resolves the TODO comment about mixing metric and metric value concerns
Reduces memory usage by sharing comparator instances across all solutions
Cleaner separation of concerns (definition vs value)
No breaking changes - all existing code continues to work

[greptile-apps]

Greptile Overview

Greptile Summary

This PR refactors the Metric class to separate metric definition (type) from metric value (measurement) and implements comparator caching to reduce memory usage. While the architectural approach is sound, there are critical bugs in the implementation:

• The _MetricDefinition.__eq__ and __hash__ methods exclude the epsilon field, which is part of the comparator's state and affects comparison behavior. This violates the equality contract and could cause two metrics with different epsilon values to be incorrectly considered equal.
• Line 247 contains redundant logic in the conditional expression.

These issues need to be resolved before merging, as they could lead to incorrect metric comparisons in production.

Confidence Score: 2/5

• This PR has critical logical errors that could cause incorrect behavior in production
• The missing epsilon field in _MetricDefinition.__eq__ and __hash__ methods breaks the equality contract and could lead to incorrect metric comparisons. This is a fundamental bug that affects core functionality.
• Pay close attention to plexe/internal/models/entities/metric.py - the equality and hash methods need to include epsilon

Important Files Changed

File Analysis

Filename	Score	Overview
plexe/internal/models/entities/metric.py	2/5	Refactored metric implementation to separate definition from value and cache comparators, but introduced critical bugs in equality checks missing epsilon field

[greptile-apps]

Additional Comments (1)

1. plexe/internal/models/entities/metric.py, line 247 (link)

   syntax: redundant condition: (self.is_worst and other.is_worst) is unnecessary since self.is_worst already covers this case

_{1 file reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

logic: epsilon is missing from equality check - two metric definitions with different epsilon values will be considered equal, causing incorrect behavior when comparing metrics with different epsilon tolerances

Suggested change

	def __eq__(self, other) -> bool:
	"""Check if two metric definitions are equal."""
	if not isinstance(other, _MetricDefinition):
	return False
	return (
	self.name == other.name
	and self.comparator.comparison_method == other.comparator.comparison_method
	and self.comparator.target == other.comparator.target
	)
	def __eq__(self, other) -> bool:
	"""Check if two metric definitions are equal."""
	if not isinstance(other, _MetricDefinition):
	return False
	return (
	self.name == other.name
	and self.comparator.comparison_method == other.comparator.comparison_method
	and self.comparator.target == other.comparator.target
	and self.comparator.epsilon == other.comparator.epsilon
	)

Prompt To Fix With AI

This is a comment left during a code review.
Path: plexe/internal/models/entities/metric.py
Line: 163:171

Comment:
**logic:** `epsilon` is missing from equality check - two metric definitions with different epsilon values will be considered equal, causing incorrect behavior when comparing metrics with different epsilon tolerances

```suggestion
    def __eq__(self, other) -> bool:
        """Check if two metric definitions are equal."""
        if not isinstance(other, _MetricDefinition):
            return False
        return (
            self.name == other.name
            and self.comparator.comparison_method == other.comparator.comparison_method
            and self.comparator.target == other.comparator.target
            and self.comparator.epsilon == other.comparator.epsilon
        )
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logic: epsilon is missing from hash calculation - must include all fields used in __eq__ to maintain hash contract

Suggested change

	def __hash__(self) -> int:
	"""Hash the metric definition."""
	return hash((self.name, self.comparator.comparison_method, self.comparator.target))
	def __hash__(self) -> int:
	"""Hash the metric definition."""
	return hash((self.name, self.comparator.comparison_method, self.comparator.target, self.comparator.epsilon))

Prompt To Fix With AI

This is a comment left during a code review.
Path: plexe/internal/models/entities/metric.py
Line: 173:175

Comment:
**logic:** `epsilon` is missing from hash calculation - must include all fields used in `__eq__` to maintain hash contract

```suggestion
    def __hash__(self) -> int:
        """Hash the metric definition."""
        return hash((self.name, self.comparator.comparison_method, self.comparator.target, self.comparator.epsilon))
```

How can I resolve this? If you propose a fix, please make it concise.