model: Move random seed and random to __init__

[rht]

Given that super().__init__() is now necessary for user's model class __init__(), the main motivation: this simplifies the Model construct.
And model.random can be initialized with other RNG objects (np.random.default_rng(...), or any other RNG).

Prior discussion #1938.

This is a breaking change and should wait until Mesa 3.0.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0bd4429) 79.87% compared to head (ed9b5aa) 79.82%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1940      +/-   ##
==========================================
- Coverage   79.87%   79.82%   -0.05%     
==========================================
  Files          15       15              
  Lines        1267     1264       -3     
  Branches      277      277              
==========================================
- Hits         1012     1009       -3     
  Misses        216      216              
  Partials       39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[quaquel]

Why would this have to wait for 3.0? I am not sure I understand how this is a breaking change?

[rht]

Because this PR requires mandatory model super().__init__, which is an API breaking change. Mesa 2.2.x recommends having model super().__init__(), but is not necessary.

I forgot that #2026 hasn't been released as a patch release. cc: @EwoutH we need #2026 in a patch release so that 2.2.x is backward compatible with 2.1.x.

[EwoutH]

2.3 is released, so we can start with 3.0 development work, including moving this PR forward!

[catherinedevlin]

@jackiekazil This PR looks good; the ruff failure appears to be outside the code change and I assume it will go away once main is merged into it.

[EwoutH]

@rht would you like to rebase this PR and see if we can move it forward, or would you like me to do it?

[rht]

Rebased.

[rht]

Fixing the tests.

[rht]

Unfortunately, the seed param is non-optional in the visualization. The caveat of merging this PR is that users will have to explicitly pass seed to the model super().__init__() in order for visualization to work.

[quaquel]

why wrap everything into kwargs, instead of having seed=None explicit?

[rht]

That makes sense, I'm changing it to that regardless of whether it fixes the visualization situation or not.

[quaquel]

Unfortunately, the seed param is non-optional in the visualization. The caveat of merging this PR is that users will have to explicitly pass seed to the model super().__init__() in order for visualization to work.

I am not intimately familiar with the visualization side of things. Can you explain the problem a bit more (and see my code comment, which might fix this also?).

[rht]

All the context you need is in

mesa/mesa/visualization/solara_viz.py

Lines 128 to 135 in a48dd80

	def make_model():
	"""Create a new model instance with current parameters and seed."""
	model = model_class.__new__(
	model_class, **model_parameters, seed=reactive_seed.value
	)
	model.__init__(**model_parameters)
	current_step.value = 0
	return model

. The seed is explicit in the visualization now, so that the result is reproducible.

Actually, if the __init__ with seed doesn't work, I can also explicitly set the seed after the model object has been created.

[rht]

Actually, if the init with seed doesn't work, I can also explicitly set the seed after the model object has been created.

I implemented exactly this way in the 3rd commit. So we're all good now regarding the move to remove __new__ from Model.

It seems to be the build cache that the CI fails. The test_time.py passed locally on my machine.

[quaquel]

I have resolved the conflicts with main. I can confirm that the tests pass fine locally, but still fail on CI. Not sure why or how to move this forward, but it would be great if we can merge this soon.

[EwoutH]

You found a nice rabbit hole? Seems some of the schedulers depend in some way on the new method?

[quaquel]

yes this is a real rabbit hole. I have established that the problem occurs only if you instantiate MockModel with keyword arguments inside unittest.TestCase instances. My current idea is just to move all tests over to pytest and see if that fixes the issue. Removing e.g., caches does not solve it. I will be flying tomorrow so then I'll have time to fix this.

[EwoutH]

Very particular.

My current idea is just to move all tests over to pytest

Would be good anyways.

[quaquel]

This is turning into an insane rabbit hole. I have been running many tests in #2295, but still no luck. A short summary

1. I have moved all tests from unittest to pytest. The error persists
2. I have implemented an alternative MockModel. Still no luck
3. I have changed the signature of Model in various ways no luck
4. It instantiates just fine if I instantiate MockModel in time_test.py outside of a test_x function. Inside a test_x function, it errors with a TypeError related to __new__
5. If I add a __new__ method back to Model which does nothing special. it all works again.
6. If I add a simple class hierarchy inside time_test.py with the exact signature as Model and let MockModel inherit from this, it instantiates just fine.
7. Examples, which all subclass Model run fine
8. locally, everything runs fine.

My last resort is eliminating MockModel inside test_time.py and testing all schedulers directly. This is a better test practice anyway. However, I would still love to figure out what the .... is going on here.

[quaquel]

Another update: If I move Model in its entirety over to test_time, it all works fine. This does suggest some kind of caching, but I don't understand how. I have checked if the correct version of the code is being used by changing mesa.__version__ and printing this after catching a TypeError. So the correct code is being used.

Anyone any idea?

[rht]

To test whether it is the cache problem, the cache can be explicitly deleted at https://github.com/projectmesa/mesa/actions/caches.

Another pattern I noticed is that in test_datacollector.py and test_batch_run.py, MockModel is initialized without argument. It might have something to do with passing extra arguments, that preceeds the class itself, to the object class's __new__ (https://stackoverflow.com/questions/59217884/new-method-giving-error-object-new-takes-exactly-one-argument-the-typ).

This is a problem that the user might encounter, so I think it is worth checking the bottom of it instead of sweeping it under the rug.

[quaquel]

To test whether it is the cache problem, the cache can be explicitly deleted at https://github.com/projectmesa/mesa/actions/caches.

I forgot to mention this, but yes, that is the first thing I tested, and no luck.

Another pattern I noticed is that in test_datacollector.py and test_batch_run.py, MockModel is initialized without argument.

Yes, but without arguments, it works. The moment keyword arguments are passed to MockModel, the type error occurs. And only if you do this inside a test_{} function, not anywhere else in the same test file. Moreover, if I copy Model in its entirety to test_time.py instead of importing it, everything suddenly works fine again.

This is a problem that the user might encounter, so I think it is worth checking the bottom of it instead of sweeping it under the rug.

I would like to get to the bottom of it, but I am quite confident it is not something a user will encounter. Locally, all tests pass, no problem. All examples also pass, no problem. So it really seems a problem related to testing and the CI runners.

[EwoutH]

Could be an OS thing, or an version thing. You’re on MacOS right? Which Python version? And Pytest and NumPy?

[quaquel]

Yes, I am on macos which of course we don't test. I have an ubuntu and windows VM, so I can test tomorrow.

Python version does not matter, all versions in CI error in the same way. Numpy is not in the picture for the error.

[EwoutH]

Yes, I am on macos which of course we don't test.

We do have a macOS run in the CI right? Or do you mean something different?

[quaquel]

We do have a macOS run in the CI right? Or do you mean something different?

You are right, I overlooked it. MacOs with python 3.12. I think I am on 3.11 but I highly doubt that is the problem.

[rht]

I was finally able to reproduce this locally. Instead of just pytest tests/test_time.py, you have to run the entire tests, pytest tests.

[quaquel]

I have run all tests from within my IDE without a problem. Are you using the command line, because it would be great if I can also reproduce it locally.

[rht]

I was using the command line, and also made sure that pytest and pytest-mock are up to date on my machine.
I have narrowed down the tests to be pytest tests/test_solara_viz.py tests/test_time.py. Somewhere in test_solara_viz.py, a global property of the Model class probably got tampered.

[quaquel]

That is in line with my latest test. I copied Model inside model.py, gave it a different name, and used this in test_time.py This solved the type error, so yes, at last, we are on the right path to solving this.

[quaquel]

my guess it is this line in test_solara_vis.py: mocker.patch.object(mesa.Model, "__new__", return_value=model). It patches the __new__ method.

[rht]

This seems to be the explanation. This PR removes the need of the SolaraViz code to use __new__ anyway, that we should be avoid the problem.

[rht]

The CI passed, and running solara run app.py on Boltzmann wealth model experimental and changing the seeds on the go also worked.