Reimplement schedulers to use AgentSet by quaquel · Pull Request #1926 · mesa/mesa

quaquel · 2023-12-23T19:52:12Z

This PR builds on #1916 and updates the schedulers to use the new AgentSet class and/or weakrefs. It also adds a new feature: schedulers take an optional agents keyword argument in __init__ so you can spawn a scheduler with all agents.

My aim with this PR is to be fully backward compatible but raise warnings when methods that have become redundant are being used. The current code passes all unit tests.

Still to do:

codecov · 2023-12-23T19:53:01Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (7bd42e7) 79.14% compared to head (37bdb57) 80.21%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
mesa/time.py	94.54%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1926      +/-   ##
==========================================
+ Coverage   79.14%   80.21%   +1.06%     
==========================================
  Files          15       15              
  Lines        1098     1137      +39     
  Branches      236      245       +9     
==========================================
+ Hits          869      912      +43     
+ Misses        198      195       -3     
+ Partials       31       30       -1

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

EwoutH · 2023-12-24T09:19:17Z

Looks like a nice start!

I think a big conceptual design decision we need to make is if we want to support multiple schedulers. Did we do so in the past?

If we only have one scheduler, we could simplify and assume a lot, for example that all agents are in the scheduler. We don't have to explicitly add or remove them, or even keep track of them in the scheduler. There would also be one single step/time clock. It would probably allow deprecating all schedulers, and moving a lot of the requesting of behavior to AgentSet.do(). It would be a flexible approach with lot possibilities, like described in #1912.

However, if we do want to support multiple schedulers, we need to leave a lot of stuff in the scheduler, at least the time of that scheduler and which agents are part of it. Personally, I can't try of that many cases you can't solve with the AgentSet approach, but there might be any.

Also curious what @jackiekazil @tpike3 and @Corvince think on this matter.

quaquel · 2023-12-24T10:22:58Z

I have used multiple schedulers in the past (pre RandomActivationByType), and more generally, I like its flexibility. Also, this PR's only aim is to keep the current Schedulers up to date by using AgentSet. Discussing the future of schedulers is beyond the scope of this PR.

As indicated in #1912, one possible direction could be to shift to a central EventList. This creates a central understanding of time. All existing functionality of the various schedulers could be rebuilt on such an event list.

EwoutH · 2023-12-24T10:36:58Z

mesa/time.py

+        if not self._remove_warning_given:
+            self._remove_warning_given = True
+        warnings.warn(
+            "Because of the shift to using weakrefs, it is no longer needed to explicitly remove"


I think this holds up as long a you use a single scheduler, and assume agents in model == agents in schedulers. However, as you let that assumption go you might want to remove agents from a scheduler but not from a model.

However, I find it weird you still have to explicitly add agents to the scheduler, but don't explicitly have to remove them.

This comes back to having one or more than one scheduler. If you have only one, then yes, there is no need to add or remove them. But that would be a backward incompatible change and thus is outside the scope of this PR.

@EwoutH

Adresses @EwoutH comment

@EwoutH

further tweaks based on feedback from @EwoutH

quaquel · 2023-12-24T11:49:32Z

I have reviewed the code several times, fixed the docs, and tested against various examples. This is ready for a proper check. @EwoutH, @Corvince, @jackiekazil, or @tpike3 do you have a chance to check this?

Corvince · 2023-12-24T11:50:55Z

Probably not in the coming days, but I really look forward to review this!

EwoutH · 2023-12-24T12:33:12Z

Will try to review it later today!

EwoutH

Looks good in general, and agree with the intend of this PR.

However, I think that we only should make minimal changes until we decide which direction to go with the schedulers (None, one per model, or multiple). So while this PR is good, I think the next step is a broad discussion about how we see the future of activating agents to do things.

mesa/time.py

EwoutH · 2023-12-24T13:49:07Z

mesa/time.py

+                stacklevel=2,
+            )
+
+        agent_keys = [agent.unique_id for agent in self._agents]


Agreed on deprecating in this case.

However, while in this way it's probably fine, we're technically deprecating something for which the replacement (AgentSet) is still experimental. Might be something to keep in mind.

Again, this is outside the scope of this PR, but I want to do a separate discussion on unique_id. Now that it is no longer needed for the schedulers, its only function is to identify agents when gathering statistics.

Agree - outside of this pr - suggested place for the discussion?

Given that #1933 also relies on unique_id, we can perhaps discuss it there. Alternatively, we can have a broader discussion on the future of data collection and make it part of that.

mesa/time.py

mesa/agent.py

EwoutH · 2024-01-01T21:41:02Z

@quaquel What's your guidance on this regarding the next release? Would you recommend having this in the 2.2.0 release, or to release 2.2.0 first and merge this on the 2.3 development branch?

jackiekazil · 2024-01-02T04:10:18Z

@quaquel

RE: Release -- would like to hear your thoughts on the release integration mentioned by @EwoutH.
Re: scheduler - +1 to what you said -- multiple schedulers is a requirements.

RE: Review -- I looked over the code this evening. I will do a test run tomorrow evening.

quaquel · 2024-01-02T08:21:26Z

@EwoutH : Ideally, make it part of 2.2
@jackiekazil: thanks for the clarity on having multiple schedulers as a requirement. This does, however, leads into a next question: what tracks the global simulation time?

tpike3

LGTM! Thanks at @quaquel I think this really highlights a new and positive path for Mesa.

EwoutH · 2024-01-26T11:25:51Z

Unfortunately, this PR, together with an implementation error in #1916, broke RandomActivation, and more important, went completely unnoticed for multiple weeks, until an user reported it in #2006. We should evaluate very critically how such a major unintended behavior could slip through the cracks.

quaquel · 2024-01-26T12:02:55Z

we did not test AgentSet.shuffle properly.
We moved the schedulers to AgenSet for 2.2. We should have waited with this for 2.3 (although I doubt we would have seen these bugs before then)

EwoutH · 2024-01-26T12:25:42Z

(and thus the next questions are, should we alter our review process, testing standards or something else, and if so, how)

quaquel · 2024-01-26T13:02:44Z

See #2007. The fix can be found there.

quaquel added 2 commits December 23, 2023 18:06

Move schedulers to use AgentSet

3983264

devsscheduler now uses weakrefs and fix for circular imports

a06b099

codespell and black fixes

33d978f

quaquel marked this pull request as draft December 23, 2023 19:57

quaquel added 2 commits December 24, 2023 10:45

unit tests and ruff

e5f14b5

additional test for RandomActivationByType

ed194b1

EwoutH reviewed Dec 24, 2023

View reviewed changes

quaquel added 3 commits December 24, 2023 12:15

Fix for remove_warning

d1e0060

Adresses @EwoutH comment

remove added back in

6e4c9ba

further tweaks based on feedback from @EwoutH

Update time.py

802c386

EwoutH reviewed Dec 24, 2023

View reviewed changes

update __init__ docstrings to reflect agents kwarg

37bdb57

EwoutH approved these changes Dec 25, 2023

View reviewed changes

quaquel changed the title ~~[WIP] Reimplement schedulers to use AgentSet~~ Reimplement schedulers to use AgentSet Dec 29, 2023

quaquel marked this pull request as ready for review December 29, 2023 12:55

EwoutH reviewed Dec 29, 2023

View reviewed changes

mesa/agent.py Show resolved Hide resolved

tpike3 mentioned this pull request Jan 2, 2024

update history for 2.2.0 release #1917

Closed

tpike3 approved these changes Jan 3, 2024

View reviewed changes

tpike3 merged commit 811b605 into mesa:main Jan 3, 2024

EwoutH mentioned this pull request Jan 6, 2024

time: Use weak references to agents in schedulers #1910

Closed

EwoutH added the enhancement Release notes label label Jan 9, 2024

EwoutH mentioned this pull request Sep 26, 2024

tests: Resolve warnings by removing scheduler and updating arguments #2329

Merged

quaquel deleted the schedulers branch November 4, 2024 19:35

Uh oh!

Conversation

quaquel commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

EwoutH commented Dec 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quaquel commented Dec 24, 2023

Uh oh!

EwoutH Dec 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EwoutH Dec 24, 2023

Choose a reason for hiding this comment

Uh oh!

quaquel Dec 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quaquel commented Dec 24, 2023

Uh oh!

Corvince commented Dec 24, 2023

Uh oh!

EwoutH commented Dec 24, 2023

Uh oh!

EwoutH left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EwoutH Dec 24, 2023

Choose a reason for hiding this comment

Uh oh!

quaquel Dec 25, 2023

Choose a reason for hiding this comment

Uh oh!

jackiekazil Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

quaquel Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EwoutH commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackiekazil commented Jan 2, 2024

Uh oh!

quaquel commented Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tpike3 left a comment

Choose a reason for hiding this comment

Uh oh!

EwoutH commented Jan 26, 2024

Uh oh!

quaquel commented Jan 26, 2024

Uh oh!

EwoutH commented Jan 26, 2024

Uh oh!

quaquel commented Jan 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

quaquel commented Dec 23, 2023 •

edited

Loading

codecov bot commented Dec 23, 2023 •

edited

Loading

EwoutH commented Dec 24, 2023 •

edited

Loading

EwoutH Dec 24, 2023 •

edited

Loading

quaquel Dec 24, 2023 •

edited

Loading

quaquel Jan 2, 2024 •

edited

Loading

EwoutH commented Jan 1, 2024 •

edited

Loading

quaquel commented Jan 2, 2024 •

edited

Loading