Allow user models to assign Model.agents for now, but add warning

[quaquel]

Fix in case the user is using model.agents.

The fix consists of 3 things

1. I added a setter for model.agents. If used, this triggers a warning. Whatever the user wants to assign is assigned to model._agents
2. I moved model._agents to model.agents_. I deem it unlikely that this label is being used by anyone.
3. I modified the getter for model.agents. It checks if model._agents exists in which case the user is using model.agents.

I have not included full name mangling, but we can do so if we want to be extra sure that there are no classes with user code. But this code works as intended. All current tests pass and I tested with the example models to see what happens if I define model.agents myself.

There are 2 corner cases left. First, if the user is using descriptors for model.agents this won't work. Second, if the user is using model._agents and/or model._agents. The latter could be fixed by full name mangling. The first is not fixable.

[codecov]

Codecov Report

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

Comparison is base (877d552) 79.27% compared to head (12fe852) 79.03%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
mesa/model.py	66.66%	3 Missing and 1 partial ⚠️
mesa/agent.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1976      +/-   ##
==========================================
- Coverage   79.27%   79.03%   -0.24%     
==========================================
  Files          16       16              
  Lines        1298     1307       +9     
  Branches      291      293       +2     
==========================================
+ Hits         1029     1033       +4     
- Misses        230      233       +3     
- Partials       39       41       +2     

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

[Corvince]

Thanks, this looks good, just 2 comments and a failing pre-commit ci

[quaquel]

pre-commit.ci autofix

[EwoutH]

Thanks a lot for this effort. I will leave the code review to others, but conceptually, this looks really good.

There are 2 corner cases left. First, if the user is using descriptors for model.agents this won't work.

If an user is advanced enough to do that, they will check the source code on problems, and know how to find us. So not a problem.

Second, if the user is using model._agents and/or model._agents. The latter could be fixed by full name mangling.

Also not a problem. _name means private, you shouldn't use that in a library. Also the chance is a lot smaller than the really common .agents.

Actually, maybe we can add that as a policy. model._anything and agent._anything are reserved by Mesa to use for whatever we want without breaking API).

So LGTM conceptually.

[Corvince]

Actually, maybe we can add that as a policy. model._anything and agent._anything are reserved by Mesa to use for whatever we want without breaking API).

Thats an excellent idea! If we just document this well enough this should prevent some headaches

[EwoutH]

Thanks! Will open a discussion or PR for it somewhere.

If this can be validated on multiple custom models that currently define Model.agents themselves, I would like to tag a bug fix release with this fix.

@rht I would also like a party not currently involved in this PR to validate it, could you do that?

[EwoutH]

Hi @NickGotts! This PR should have solved your issue migrating your model from Mesa 2.1 to 2.2. Could you maybe test if the model you had for 2.1.5 now works on the latest Mesa version? You can install the latest development version of Mesa that includes this PR with:

pip install -U -e git+https://github.com/projectmesa/mesa@main#egg=mesa

[quaquel]

Actually, maybe we can add that as a policy. model._anything and agent._anything are reserved by Mesa to use for whatever we want without breaking API).

Thats an excellent idea! If we just document this well enough this should prevent some headaches

I would be careful with the wording on this. It means that any custom properties, which often involve the use of underscore variable names can freely be broken by MESA. To be clear, I am fine with the basic idea. What matters is how to communicate it in a clear way.

[Corvince]

Yes communication will be key here. We shouldn't introduce this before 3.0, but I think the restriction should be fine. Users can still use custom properties, just not private ones.

Alternatively, and I am just thinking about this now, we could just have a single __mesa_internal__ attribute which is just a dict with all the attributes we use internally. So its just a single attribute name that users can't use (so _random, steps, agents would all go there). I wouldn't even see this as a breaking change, the changes of a name collision are too low.

[Corvince]

Oh I think my suggested name could trigger some name mangling? I am not up to date on this topic, maybe this would be good or it would be bad, but it wasn't intentional (albeit maybe subconsciously intended)

[rht]

A bit too late for me to comment, but self.agents_ would be considered a public API, given a convention that private API always starts with _. If it were self._agents_ it would have been clear that it is non-public and at the same time avoids collision with existing self._agents.

[rht]

given a convention that private API always starts with _.

Even if you were to make an exception and declare self.agents_ private (i.e. anything that starts or ends with _ is private), I'd be among the confused users who would have considered it public, before reading the documentation to learn that it is private.

[quaquel]

The use of a trailing underscore can have several meanings. In PEP8 it is suggested as a way of avoiding naming conflicts with Python keywords. In scikit_learn, it is used to denote attributes that are only available after a model has been fitted. So there is precedence in Python to using underscorers (leading and trailing) to signal to the user that there is something going on that you want to be careful with.

Moreover, this PR is really meant as a temporary fix to help users migrate their code away from using model.agents. In my view, this entire PR should be removed again for 2.3 or whatever is the next release.

[EwoutH]

With this merged we can do the 2.2.2 release now right? Anything else we want to have cherry picked?

[EwoutH]

As an example the "how to merge" discussion, this PR would have been easier to cherry pick to a maintenance branch if it was squashed to a single commit. Almost no information would have been lost, since that's all here in the PR discussion anyway.

• Establish PR merge method policy #1963