Repository: projectmesa/mesa
Pull request #2605 (2025-01-10 07:47:25)

Enhance DataCollector to validate model_reporters functions
### Summary
This PR enhances the DataCollector class by adding a comprehensive validation system for model reporters. The main goal is to provide clearer feedback when reporters are misconfigured, while maintaining backward compatibility.


### Motive
The original DataCollector implementation had several limitations:
1. No validation for different types of model reporters
2. Silent failures or unclear error messages when reporters were misconfigured
3. Inconsistent handling of method calls and function parameters
4. No early warning system for potential issues

These issues made debugging difficult and led to confusion when setting up model reporters.

### Implementation
1. New Validation Method:
```python
def _validate_model_reporter(self, name, reporter, model):
    """Validates four types of model reporters:
    1. Lambda functions
    2. Method references (as strings)
    3. Attribute strings
    4. Function lists with parameters
    """
```
2. Modified Collection Logic:
```python
def collect(self, model):
    if self.model_reporters:
        for var, reporter in self.model_reporters.items():
            # Add validation before collection
            self._validate_model_reporter(var, reporter, model)
            # Existing collection logic continues...
```
3. Warning System:
- Replaced hard errors with warning messages
- Added specific warnings for each reporter type
- Included examples in warning messages


### Usage Examples
1. Lambda Functions:
```python
# Valid usage
model_reporters = {
    "Agent Count": lambda m: len(m.agents)
}

# Invalid usage (will show warning)
model_reporters = {
    "Bad Lambda": lambda m: m.nonexistent_attr
}
```
2. Method References:
```python
# Valid usage
model_reporters = {
    "Grid Size": "get_grid_size"  # As string
}

# Invalid usage (will show warning)
model_reporters = {
    "Grid Size": self.get_grid_size  # Direct reference
}
```
3. Attribute Strings:
```python
# Valid usage
model_reporters = {
    "Total Wealth": "total_wealth"
}

# Invalid usage (will show warning)
model_reporters = {
    "Status": "nonexistent_attribute"
}
```
4. Function Lists:
```python
# Valid usage
model_reporters = {
    "Custom": [calculate_metric, [model, param]]
}

# Invalid usage (will show warning)
model_reporters = {
    "Bad Function": ["not_callable", [1, 2]]
}
```

### Additional Notes

1. Test Coverage:
- Added comprehensive test suite in `test_model_reporters.py`
- Tests cover both valid and invalid configurations
- Includes edge cases and error conditions
2. Backward Compatibility:
- All existing code continues to work
- Warnings can be suppressed if desired
- No breaking changes to the API
3. Documentation:
- Updated docstrings with clear examples
- Added warning messages with helpful suggestions
- Included migration guide for existing code
4. Files Modified:
- `datacollection.py`: the only one py code changed in mesa offical repository.
5. Dependencies:
- No new dependencies added
- Uses standard Python `warnings` module

[datacollection_new.zip](https://github.com/user-attachments/files/18372344/datacollection_new.zip)


EwoutH: Thanks for the PR, sounds interesting.

What's the performance overhead?
peter-kinger: > Thanks for the PR, sounds interesting.
> 
> What's the performance overhead?

Thanks for your attention to the PR. I apologize for the delayed response as I was unfamiliar with Mesa's testing logistics.

Regarding the overhead:

1. Are you referring to the execution time measured by `global_benchmark.py`?

2. As "quaquel" mentioned, the current version of `_validate_model_reporter` performs validation during data collection. Our performance benchmarks show some noticeable overhead.

Future considerations:

1. I've been working on a new version of the validation logic using `_validate_reporter_format` within the `DataCollector.__init__` function. However, this approach presents challenges, particularly with Type 4 reporters (function with parameters), as we can't fully validate them without the `model` parameter.

2. I would greatly appreciate your suggestions on how to proceed, as this has been a challenging issue to resolve.


quaquel: I suggest running the validation logic only on the first-ever call to `DataCollector.collect`. This solves both the overhead problem and your issue with type 4 reporters. 
peter-kinger: > I suggest running the validation logic only on the first-ever call to `DataCollector.collect`. This solves both the overhead problem and your issue with type 4 reporters.

Thanks for the suggestions for the position where the "validate function" puts. I have completed the v2, version two, codes of it:

1. Improve: Due the essential input parameter "model", I put the `**_validate_model_reporter**` within the `collect` function. But it will be validate once.  (put the change of signal firstly avoid losing efficacy in `_validate_model_reporter`)
2.  overhead: add the bool signal of "`self._validated`" in `_init_` to make sure there will be once to be validated in the progress of datacollecting.


In addition, I want to clarify the problems in checks:

1. [run-benchmarks](https://github.com/projectmesa/mesa/actions/runs/12705394103/job/35416342272?pr=2605#logs): The benchmark failure (ModuleNotFoundError: No module named 'scipy') appears to be a pre-existing issue with the test environment's dependencies, unrelated to my changes in `datacollector.py`.
2. examples: new version codes have passed the unit test of pytest. The previous fail of unnecessary positional argument had solved.
3. Codecov: I have added corresponding unit tests to cover the new components introduced in `_validate_model_reporter`, ensuring adequate test coverage.

I don't know if my modification is qualified, thank you for your review.
quaquel: Thanks for the update. I'll try to review the code later today. I did update your branch by merging the main back into it, so everything is up to date again.

1. Yes, scipy was an issue that has, in the meantime, been resolved. You can also see that all tests have now been passed.
2. The one outstanding check that is failing is the codecoverage one. Ideally, we want PRs to be as close as reasonably possible to 100% coverage. The reason your coverage is lower than desired is that you haven't checked the raise exception clauses in your test code. You can do this with `pytest.raises`. It would be great if you can explicitly add tests for this.

github-actions[bot]: Performance benchmarks:

| Model           | Size   | Init time [95% CI]      | Run time [95% CI]       |
|-----------------|--------|-------------------------|-------------------------|
| BoltzmannWealth | small  | 🔵 +2.6% [+1.1%, +4.0%] | 🔵 +1.9% [+1.3%, +2.5%] |
| BoltzmannWealth | large  | 🔵 +0.2% [-1.1%, +1.6%] | 🔵 +1.0% [-0.3%, +2.0%] |
| Schelling       | small  | 🔵 +2.8% [+1.9%, +3.6%] | 🔵 -0.4% [-0.9%, +0.1%] |
| Schelling       | large  | 🔵 -0.9% [-8.8%, +3.9%] | 🔵 +0.3% [-0.2%, +0.9%] |
| WolfSheep       | small  | 🔵 +3.9% [+3.0%, +4.8%] | 🔵 -0.2% [-0.7%, +0.3%] |
| WolfSheep       | large  | 🔵 +3.9% [+3.0%, +5.0%] | 🔵 -0.1% [-0.7%, +0.5%] |
| BoidFlockers    | small  | 🔵 -0.7% [-2.6%, +1.0%] | 🔵 -0.9% [-1.3%, -0.5%] |
| BoidFlockers    | large  | 🔵 -2.0% [-3.8%, -0.6%] | 🔵 -0.5% [-1.1%, +0.1%] |

github-actions[bot]: Performance benchmarks:

| Model           | Size   | Init time [95% CI]      | Run time [95% CI]       |
|-----------------|--------|-------------------------|-------------------------|
| BoltzmannWealth | small  | 🔵 +1.7% [-0.6%, +4.0%] | 🔵 -1.1% [-1.6%, -0.7%] |
| BoltzmannWealth | large  | 🔵 -0.5% [-1.3%, +0.1%] | 🔵 +1.3% [+0.3%, +2.2%] |
| Schelling       | small  | 🔵 +1.7% [+1.4%, +2.1%] | 🔵 -0.2% [-0.3%, -0.1%] |
| Schelling       | large  | 🔵 +1.4% [+0.8%, +2.1%] | 🔵 -0.7% [-1.5%, +0.0%] |
| WolfSheep       | small  | 🔴 +3.6% [+3.2%, +4.0%] | 🔵 +0.4% [+0.2%, +0.5%] |
| WolfSheep       | large  | 🔴 +4.1% [+3.1%, +5.1%] | 🔵 +1.2% [-0.1%, +2.6%] |
| BoidFlockers    | small  | 🔵 -0.6% [-1.2%, -0.1%] | 🔵 -0.5% [-0.7%, -0.4%] |
| BoidFlockers    | large  | 🔵 -0.1% [-0.5%, +0.2%] | 🔵 +0.1% [-0.1%, +0.3%] |

peter-kinger: Thank you very much for taking the time to review all the code revisions in detail. I sincerely apologize for the delayed response due to the Chinese New Year holiday.

I greatly appreciate your suggestion regarding the use of `pytest.raises`. I will add the corresponding test cases in the coming days to ensure 100% test coverage.
github-actions[bot]: Performance benchmarks:

| Model           | Size   | Init time [95% CI]      | Run time [95% CI]       |
|-----------------|--------|-------------------------|-------------------------|
| BoltzmannWealth | small  | 🔵 +0.1% [-1.0%, +1.2%] | 🔵 -0.0% [-0.1%, +0.1%] |
| BoltzmannWealth | large  | 🔵 -1.1% [-1.6%, -0.6%] | 🔵 -0.6% [-2.1%, +0.6%] |
| Schelling       | small  | 🔵 +1.8% [+1.5%, +2.1%] | 🔵 -0.4% [-0.5%, -0.3%] |
| Schelling       | large  | 🔵 +2.2% [+1.9%, +2.7%] | 🔵 -1.3% [-2.0%, -0.6%] |
| WolfSheep       | small  | 🔵 +2.6% [+2.3%, +2.9%] | 🔵 +0.0% [-0.1%, +0.2%] |
| WolfSheep       | large  | 🔵 +2.8% [+1.9%, +3.9%] | 🔵 +0.3% [-0.4%, +1.1%] |
| BoidFlockers    | small  | 🔵 +0.7% [+0.2%, +1.2%] | 🔵 -0.2% [-0.5%, -0.0%] |
| BoidFlockers    | large  | 🔵 +0.5% [-0.1%, +1.1%] | 🔵 -0.3% [-0.4%, -0.1%] |

quaquel: This looks ready to go. @peter-kinger, is there any outstanding issue, or can I do a final review and merge this?


peter-kinger: Hi  @quaquel, based on my observation, there aren't any outstanding issues. I'm not familiar with the performance benchmarks provided by GitHub Actions. If the benchmarks pass, I think we can merge it

In addtion, issues/2606(https://github.com/projectmesa/mesa/issues/2606) should be closed if the PR finishs. Thanks for the reviewing!

Patch:
diff --git a/mesa/datacollection.py b/mesa/datacollection.py
index 309d56d8..63c0e7f0 100644
--- a/mesa/datacollection.py
+++ b/mesa/datacollection.py
@@ -117,6 +117,9 @@ class DataCollector:
         self._agenttype_records = {}
         self.tables = {}
 
+        # add the signal of the validation of model reporter
+        self._validated = False
+
         if model_reporters is not None:
             for name, reporter in model_reporters.items():
                 self._new_model_reporter(name, reporter)
@@ -134,13 +137,66 @@ class DataCollector:
             for name, columns in tables.items():
                 self._new_table(name, columns)
 
+    def _validate_model_reporter(self, name, reporter, model):
+        """Validate model reporter and handle validation results appropriately.
+
+        Args:
+            name: Name of the reporter
+            reporter: Reporter definition (lambda/method/attribute/function list)
+            model: Model instance
+
+        Raises:
+            ValueError: If reporter is None or has invalid format
+            AttributeError: If model attribute doesn't exist
+            TypeError: If reporter type is not supported
+            RuntimeError: If reporter execution fails
+        """
+        self._validated = True  # put the change of signal firstly avoid losing efficacy
+
+        # Type 1: Lambda function
+        if isinstance(reporter, types.LambdaType):
+            try:
+                reporter(model)
+            except Exception as e:
+                raise RuntimeError(
+                    f"Lambda reporter '{name}' failed validation: {e!s}\n"
+                    f"Example: lambda m: len(m.agents)"
+                ) from e
+
+        # Type 2: Method of class/instance
+        if not callable(reporter) and not isinstance(reporter, types.LambdaType):
+            pass
+
+        # Type 3: Model attribute (string)
+        if isinstance(reporter, str):
+            try:
+                if not hasattr(model, reporter):
+                    raise AttributeError(
+                        f"Model reporter '{name}' references non-existent attribute '{reporter}'\n"
+                    )
+                getattr(model, reporter)  # 验证属性是否可访问
+            except AttributeError as e:
+                raise AttributeError(
+                    f"Model reporter '{name}' attribute validation failed: {e!s}\n"
+                ) from e
+
+        # Type 4: Function with parameters in list
+        if isinstance(reporter, list) and (not reporter or not callable(reporter[0])):
+            raise ValueError(
+                f"Invalid function list format for reporter '{name}'\n"
+                f"Expected: [function, [param1, param2]], got: {reporter}"
+            )
+
     def _new_model_reporter(self, name, reporter):
         """Add a new model-level reporter to collect.
 
         Args:
             name: Name of the model-level variable to collect.
-            reporter: Attribute string, or function object that returns the
-                      variable when given a model instance.
+            reporter: Can be one of four types:
+                1. Attribute name (str): "attribute_name"
+                2. Lambda function: lambda m: len(m.agents)
+                3. Method: model.get_count or Model.get_count
+                4. List of [function, [parameters]]
         """
         self.model_reporters[name] = reporter
         self.model_vars[name] = []
@@ -262,6 +318,10 @@ class DataCollector:
     def collect(self, model):
         """Collect all the data for the given model object."""
         if self.model_reporters:
+            if not self._validated:
+                for name, reporter in self.model_reporters.items():
+                    self._validate_model_reporter(name, reporter, model)
+
             for var, reporter in self.model_reporters.items():
                 # Check if lambda or partial function
                 if isinstance(reporter, types.LambdaType | partial):


Test patch:
diff --git a/tests/test_datacollector.py b/tests/test_datacollector.py
index 0b5153d3..7f7de853 100644
--- a/tests/test_datacollector.py
+++ b/tests/test_datacollector.py
@@ -352,5 +352,67 @@ class TestDataCollectorWithAgentTypes(unittest.TestCase):
         self.assertTrue(super_data.equals(agent_data))
 
 
+class MockModelForErrors(Model):
+    """Test model for error handling."""
+
+    def __init__(self):
+        """Initialize the test model for error handling."""
+        super().__init__()
+        self.num_agents = 10
+        self.valid_attribute = "test"
+
+    def valid_method(self):
+        """Valid method for testing."""
+        return self.num_agents
+
+
+def helper_function(model, param1):
+    """Test function with parameters."""
+    return model.num_agents * param1
+
+
+class TestDataCollectorErrorHandling(unittest.TestCase):
+    """Test error handling in DataCollector."""
+
+    def setUp(self):
+        """Set up test cases."""
+        self.model = MockModelForErrors()
+
+    def test_lambda_error(self):
+        """Test error when lambda tries to access non-existent attribute."""
+        dc_lambda = DataCollector(
+            model_reporters={"bad_lambda": lambda m: m.nonexistent_attr}
+        )
+        with self.assertRaises(RuntimeError):
+            dc_lambda.collect(self.model)
+
+    def test_method_error(self):
+        """Test error when accessing non-existent method."""
+
+        def bad_method(model):
+            raise RuntimeError("This method is not valid.")
+
+        dc_method = DataCollector(model_reporters={"test": bad_method})
+
+        with self.assertRaises(RuntimeError):
+            dc_method.collect(self.model)
+
+    def test_attribute_error(self):
+        """Test error when accessing non-existent attribute."""
+        dc_attribute = DataCollector(
+            model_reporters={"bad_attribute": "nonexistent_attribute"}
+        )
+        with self.assertRaises(Exception):
+            dc_attribute.collect(self.model)
+
+    def test_function_error(self):
+        """Test error when function list is not callable."""
+        dc_function = DataCollector(
+            model_reporters={"bad_function": ["not_callable", [1, 2]]}
+        )
+        with self.assertRaises(ValueError):
+            dc_function.collect(self.model)
+
+
 if __name__ == "__main__":
     unittest.main()