Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

time: Use weak references to agents in schedulers #1910

Closed
wants to merge 1 commit into from

Conversation

EwoutH
Copy link
Contributor

@EwoutH EwoutH commented Dec 17, 2023

This PR builts on the agents defaultdict introduced in #1894. See 89b61ec for the code changes proposed in this PR.

This PR modifies the schedulers to use weak references for agent management. The update aims to improve memory efficiency and garbage collection, especially in scenarios where agents are dynamically added and removed during model execution.

It also ensures that the Agent remove() method (introduced in #1894) also removes the Agent from the schedule.

Key changes

  • Agents within schedulers are now stored as weak references (weakref.ref[Agent]), reducing the memory footprint and allowing for more efficient garbage collection.
  • The add method in BaseScheduler and derived classes has been updated to store agents as weak references.
  • The remove method has been modified to handle both direct agent objects and weak references, improving flexibility and robustness.
  • Updated do_each method to iterate over a copy of agent keys and check the existence of agents before calling methods, ensuring stability during dynamic agent removal.
  • Updated the agents property to return a list of live agent instances, maintaining backward compatibility for user code that iterates over agents.
  • Ensured all existing tests pass, confirming the stability and correctness of the changes.

The changes result in a more memory-efficient scheduler implementation in Mesa, while preserving existing functionality and ensuring backward compatibility.

All 27 current tests pass without any modifications, so there shouldn't be changes breaking compatibility.

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

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

Comparison is base (9495a5a) 77.53% compared to head (26823e3) 77.47%.

Files Patch % Lines
mesa/time.py 86.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1910      +/-   ##
==========================================
- Coverage   77.53%   77.47%   -0.07%     
==========================================
  Files          15       15              
  Lines        1015     1021       +6     
  Branches      221      224       +3     
==========================================
+ Hits          787      791       +4     
  Misses        197      197              
- Partials       31       33       +2     

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

This commit modifies the schedulers to use weak references for agent management. The update aims to improve memory efficiency and garbage collection, especially in scenarios where agents are dynamically added and removed during model execution.

It also ensures that the Agent remove() method also removes the Agent from a schedule.

Key Changes:
- Agents within schedulers are now stored as weak references (`weakref.ref[Agent]`), reducing the memory footprint and allowing for more efficient garbage collection.
- The `add` method in `BaseScheduler` and derived classes has been updated to store agents as weak references.
- The `remove` method has been modified to handle both direct agent objects and weak references, improving flexibility and robustness.
- Updated `do_each` method to iterate over a copy of agent keys and check the existence of agents before calling methods, ensuring stability during dynamic agent removal.
- Added a `agents` property to return a list of live agent instances, maintaining backward compatibility for user code that iterates over agents.
- Ensured all existing tests pass, confirming the stability and correctness of the changes.

The changes result in a more memory-efficient scheduler implementation in Mesa, while preserving existing functionality and ensuring backward compatibility.

All 27 current tests pass without any modifications, so there shouldn't be changes breaking compatibility.
@EwoutH
Copy link
Contributor Author

EwoutH commented Dec 18, 2023

Rebased on main, now that #1894 is merged.

@quaquel
Copy link
Contributor

quaquel commented Dec 18, 2023

I think you can simply the code a bit by using WeakrefValueDictionary directly. This will make sure that items in the dict are automagically removed if referenced value no longer exists. So, some of the None checks you now have would become redundant because this is handled within the data structure for you.

@EwoutH
Copy link
Contributor Author

EwoutH commented Dec 18, 2023

Thanks! If we go this direction, I will certainly look into it.

Right now I would like to propose a different direction:

Because this just looks way too good to ignore.

class MyModel(Model):
    def step(self):
        self.do_each("move")
        self.do_each("eat")
        self.do_each("sleep")

@quaquel
Copy link
Contributor

quaquel commented Dec 18, 2023

My point was primarily about the fact that there are some useful collections in weakref that you want to use as much as possible. This point applies also when using these in spaces. It is always better to use a data structure that handles an issue (refs to None in this case) rather than solving it in your code (as you did in this draft).

The scheduler discussion is interesting and I am still thinking about it.

@EwoutH
Copy link
Contributor Author

EwoutH commented Dec 18, 2023

Fair point, thanks.

@quaquel
Copy link
Contributor

quaquel commented Dec 22, 2023

How do you want to proceed with this now that #1916 is ready to be merged?

@EwoutH
Copy link
Contributor Author

EwoutH commented Dec 22, 2023

Probably close it. The next effort should be to refactor the schedulers using AgentSet functionality, and possibly deprecate them.

@quaquel
Copy link
Contributor

quaquel commented Dec 22, 2023

Ok, if I get to it, I'll open a separate PR reimplementing the schedulers on top of AgentSet. I already had a quick look. It can be done in a backward compatible way while raising some warnings here and there.

@EwoutH
Copy link
Contributor Author

EwoutH commented Dec 22, 2023

Yeah it should be pretty easy, all the examples all already in #1916.

I have another deal: If you handle my email I will do this implementation 😇.

@EwoutH
Copy link
Contributor Author

EwoutH commented Jan 6, 2024

Closing this, succeeded by #1926.

@EwoutH EwoutH closed this Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants