Feedback on AgentSet

[EwoutH]

We would love feedback on the new AgentSet class!

• Discussion: Deprecate all schedulers and be free to do whatever you want #1912 (comment)
• Pull request (with examples): Introduce AgentSet class #1916

[NickGotts]

My feedback is that it would be good to give more attention to backwards compatibility if you want people outside the small group of developers to use, and continue to use, Mesa. Having succeeded with considerable difficulty in porting my model from version 0.8.8.1 to 2.1.5 (OK, that's a lot of versions to skip over), I saw there was a new version, 2.2.0, and thought I'd try porting to that. But the introduction of AgentSet caused it to crash immediately. As far as I can see, there's no documentation on how a model would need changing to run under 2.2.0 (a long list of changes from 2.1.5 to 2.2.0 doesn't really cut it), and there's a note that the feature is experimental and may be removed in future versions. So I'll stick with 2.1.5, and if other people want to play with my model (I intend to put it in OpenABM) they will need to install Mesa 2.1.5, not whatever the latest version is at the time.

[quaquel]

We have been trying very hard to make sure 2.2.0 is backward compatible. Over the weekend, in fact, we fixed several corner cases where 2.2 was not backward compatible and hope to release these soon. Moreover, we are discussing ways of ensuring that new releases are tested against the available examples. However, this is time-consuming, and everyone volunteers their time.

The specific error shown seems that your model uses model.agents, which has become an attribute used by MESA itself in 2.2.0. I would need to see the code in more detail to help you fix it, but it should be quite straightforward.

[Corvince]

Very helpful feedback @NickGotts . As @quaquel said we added a few tests already and fixed some errors regarding backward-compatibility. We hope to have a bugfix release today. From the looks of it, I suspect your error won't vanish immedieately, but we will try to have another bugfix release that addresses this issue. You are right, normally you should be able from 2.1. to 2.2. without breaking changes.

Regarding your model it might be a good idea anyways to pin the version of your requirements, so whenever other people want to play around with it they get exactly the right version of your dependencies.

[EwoutH]

Thanks a lot for your feedback @NickGotts! You're right and we need to do better in this regard.

For this specific issue, I was afraid this would happen. Is there some smart way we can reserve (by a warning) certain variable names? We have now claimed model.agents for our AgentSet (in #1916) without advance warning. In #1942 we're thinking about claiming model.time and model.steps. It there a way we can add a warning for users that we might going to use those names in the future?

[EwoutH]

@NickGotts for upgrading a project that follows SemVer, it's recommended to update to the latest version of a compatible release, before upgrading to an incompatible one. That way you make sure you get all the latest warnings before they turn into errors. So in your case, it should be 0.8.8 --> 0.8.9 --> 0.9.0 --> 1.2.1 --> 2.1.5.

Breaking your model between 2.1.5 and 2.2.0 is our fault.

[EwoutH]

@NickGotts to adres your specific issue, you are assigning self.agents somewhere in your Model subclass. We're now using Model.agents to track the agents in the model, so you need to rename it to something else (self.agent_list, or something more descriptive (and make sure your rename it everywhere).

[quaquel]

Thanks a lot for your feedback @NickGotts! You're right and we need to do better in this regard.

For this specific issue, I was afraid this would happen. Is there some smart way we can reserve (by a warning) certain variable names? We have now claimed model.agents for our AgentSet (in #1916) without advance warning. In #1942 we're thinking about claiming model.time and model.steps. It there a way we can add a warning for users that we might going to use those names in the future?

I think it is virtually impossible to cover these kinds of situations when adding new elements to the public API. Remember, Semver is about denoting change in the public API. In this particular case, we added something to the API that was not there before. This is a new feature but does not change the existing public API. Is it part of Semver not to add stuff to a public API because the name might be used by someone somewhere using the library?

Technically, it also would be hard to do this. Metaclasses might be partially used to validate class definitions, but given the dynamic nature of python, it is virtually impossible to cover everything.

[EwoutH]

Bit cumbersome, and I don't know what it will do to performance, but functionally it looks like this works:

Given that the steps and time attributes might be updated after the model initialization by built-in functions like step(), it's essential to modify the approach to ensure these updates can occur without triggering warnings. The goal is to allow the framework to update these attributes internally while still warning users if they attempt to set these attributes in their subclasses or instances.

A feasible solution is to use a private flag attribute within the Model class that allows the framework to bypass the warning when setting time and steps. Here’s an updated version of the Model class incorporating this idea:

import warnings

class Model:
    # ... [rest of the existing code] ...

    def __init__(self, *args: Any, **kwargs: Any) -> None:
        self._internal_update = False  # Flag to indicate internal updates
        # ... [rest of the existing __init__ code] ...

    def __setattr__(self, name, value):
        if name in ["time", "steps"] and not self._internal_update:
            warnings.warn(f"'{name}' is a reserved attribute name in the Model class.", RuntimeWarning)
        super().__setattr__(name, value)

    def _set_internal(self, name, value):
        """Internal method to set reserved attributes without triggering a warning."""
        self._internal_update = True
        setattr(self, name, value)
        self._internal_update = False

    # ... [rest of the existing code] ...

With this modification, whenever the framework needs to update the time or steps attributes internally, it should use the _set_internal method. This method temporarily sets the _internal_update flag to True, bypassing the warning mechanism in __setattr__.

For example, in the step method (or any other method that updates time or steps), you would use:

self._set_internal('time', new_time_value)
self._set_internal('steps', new_steps_value)

This approach ensures that the framework can internally manage these attributes without user interference, while still alerting users if they attempt to use these reserved names inappropriately. Again, thorough documentation of these changes is crucial for user awareness and understanding.

[NickGotts]

Thanks for your responses. I'll continue with 2.1.5 for the present. I do already have a requirements.txt file set up for the current model version running under 2.1.5. I appreciate that the developers are all volunteers giving their time free - I'm just pointing out that if you want a significant user community beyond the developers, their needs and experiences must be taken into account.

[Corvince]

Thanks a lot for your feedback @NickGotts! You're right and we need to do better in this regard.

For this specific issue, I was afraid this would happen. Is there some smart way we can reserve (by a warning) certain variable names? We have now claimed model.agents for our AgentSet (in #1916) without advance warning. In #1942 we're thinking about claiming model.time and model.steps. It there a way we can add a warning for users that we might going to use those names in the future?

I think it is virtually impossible to cover these kinds of situations when adding new elements to the public API. Remember, Semver is about denoting change in the public API. In this particular case, we added something to the API that was not there before. This is a new feature but does not change the existing public API. Is it part of Semver not to add stuff to a public API because the name might be used by someone somewhere using the library?

Technically, it also would be hard to do this. Metaclasses might be partially used to validate class definitions, but given the dynamic nature of python, it is virtually impossible to cover everything.

I think we need to differentiate here a bit. I mean yes if we add for example a new function to our Grid we can't make sure the name isn't taken and that shouldn't stop us from developing. But there the name clash is caused by users modifying our implementation.

In this case here we modified a class that is explicitly there for subclassing and adding new attributes and functionalities. In that case I would argue part of the public API are also all the attributes we did not add and are free to use by users.

Going the other attribute we could build Model in a way that disallows adding any new attributes to it. With your definition this wouldn't be a breaking change. But it would still break all existing models. So I think apart from the technical definition of semver the expectation is that upgrading a minor version shouldn't break models - unless you are very unlucky with the names you used.

In this case we all agreed that a model level agent attribute makes a lot of sense as a place to store agents. But this is also very likely also already used by users. So the right way would have been to add a warning that Model.agents will be reserved in Mesa 3.0 and only then do the change.

[quaquel]

unless you are very unlucky with the names you used.

In this case we all agreed that a model level agent attribute makes a lot of sense as a place to store agents. But this is also very likely also already used by users. So the right way would have been to add a warning that Model.agents will be reserved in Mesa 3.0 and only then do the change.

I think this is indeed the crux: model.agents is an obvious attribute that many users might already use, and we should have worked around this. But then, we better find a way to fix it in 2.2.2. A simple solution would be to remove the current agents property from the model and replace this with a temporary get_agents method. We currently use _agents as the attribute name in which the actual hard refs to all agents are stored. If we want to minimize the chance of a name class further, we might consider changing this to __agents_, which will invoke Python's name mangling.

[tpike3]

I agree let's do it, and do it today or tomorrow.

I will also add this is a good problem to have.

We are making significant improvements, getting feedback and responding.

[quaquel]

Ok, I'll try and make the PR after my lunch.

[EwoutH]

Haven't followed the full discussion, but it would be nice users don't have to update their models again after 3.0 if they already updated to 2.2. So ideally can keep using Model.agents. I pointed a few students to Model.agents already, so removing that would be a breaking change.

Model.agents.do() is really powerful in the initialisation by the way, saves a bunch of loops. Really useful stuff!

[quaquel]

This is a fair but tricky point. I'll see if I can come up with a creative solution. It probably requires some careful property trick. Basically, if you try to set model.agents yourself, you get a warning and some name mangling is used. Otherwise, you get the new behavior.

[NickGotts]

Just to note, porting from 2.1.5 to 2.2.0 did turn out to be simple: a global replace of ".agents" by ".agentlist", followed by another of "agentlist_" by "agents_" (my input files use "agents_crowded" and similar) did it.

[Corvince]

AgentSet raises a FutureWarning if first used. To original motivation for this was that early adopters know that the feature is experimental, subject to change and link to this discussion for us to receive feedback.

The issues with this is that now that our Schedulers internally use this feature and the FutureWarning is also triggered if new users just follow along the intro tutorial. I find this problematic, because users don't expect to trigger a warning following an intro tutorial and they also can't have any idea what this feature even is, because they don't use it directly.

I think it would be great if we could resolve this for our next bug fix release, also I have to admit I haven't thought about how to do this cleanly.

[EwoutH]

Agreed. Maybe we can pass an Boolean from the scheduler to the warning message for if it’s used from the scheduler or not.

@quaquel would you have an idea to do this elegantly?

[EwoutH]

In general, I was also thinking about a global switch to disable future warnings to the Model, like just an optional warnings=True you can set to False if you don’t want to see them anymore.

[quaquel]

I am sympathetic to @Corvince point that tutorials should not trigger warnings. However, in practice with many established libraries (seaborn, pandas, matplotlib, numpy), this also does not hold. Admittedly, it is more common to see DepractationWarnings rather than ExperimentalWarnings, but there is at least precedence for keeping it as is.

In this particular case, the simple solution would be to force self.__class__.agentset_experimental_warning_given to True in the BaseScheduler.

I do think we need a generic strategy for handling experimental warnings and how this relates to semver, but let's not discuss that issue here.

[divilian]

Just another vote here for disabling this warning so as not to alarm new users following an intro tutorial. I'm teaching Mesa for an ABM course here at my university, and I just got a dozen emails from students assuming they'd done something wrong following the intro tutorial because this scary warning popped up. Not a way to instill confidence...

[EwoutH]

Thanks for reaching out. It's not ideal that many IDEs put JupyterNotebook warnings in a big red box, the same one as they use for errors.

But indeed, our regular features should not trigger an warning. I think we're far enough that the AgentSet can be stabilized, I don't think we're are going to make huge breaking changes to it. I've opened a PR to stabilize it: #2065.

[OrenBochman]

"We would love to hear what you think about this new feature. If you have any thoughts, share them with us here"

My thoughts On AgentSet:

• Whenever I create a mesa model, I keep getting a very annoying message pointing to this page.
• More so since I never use AgentSet and being advised it may go away at any time, I and others like me will avoid it like the plague.
• Such a cavalier attitude---that features can disappear without warning and lacking proper deprecation policy just tells people avoid Mesa in production environments, and if you already have make a private fork of the projects so you have control of the features.
• After all the talks on this becoming a serious and reusable project, it just feels like it is being developed as someone's throwaway code for a thesis?

On the warning message :

"/home/oren/work/blog/env/lib/python3.10/site-packages/mesa/time.py:82: FutureWarning: The AgentSet is experimental. It may be changed or removed in any and all future releases, including patch releases.
We would love to hear what you think about this new feature. If you have any thoughts, share them with us here: #1919
self._agents: AgentSet = AgentSet(agents, model)"

1. :hair-on-fire: Whoever coded this message into time.py clearly has a misguided view of what users and developers are looking for. Kudos to whoever okeyed the pull request. 👍
2. It indicates the feature is source of future bugs (in production) - unexpected ones since this comes up from internal library code not from actually using AgentSet.
3. It frustrates and breaks testing using testing frameworks that rely on an invariant output from the test. Each time such a message is added or removed this breaks the tests. So at least twice per message that a each developr's time is going to be wasted. The framework is actually the simple part of the ABM developing the model is where true complexity lies. When a framework adds too much overhead in learning, usage or maintenance then users will turn to a more mature alternative, or roll their own code to avoid a flaky framework.
4. It makes debugging client, code very unpleasant experience by flooding the limited console screen real estate with drivel so one must scroll to find the actual trace.
5. Lazt lazy, lazy - Mesa is poorly documented. Instead of creating this kind of trolling message, start adding (minimal) documentation for new features. I noticed servral times that samples and UML explaining the library are hidden in unfindable discussions or in pull requests with examples instead of the official documentation - lazy and counterproductive. Even after I had read them I could not find these at a latter date.
6. If no one can be bothered to document new features this is a clear indicator that they are not a well thought out and therefore premature. Play around in your own private fork or create an incubator with optional install for experimental features.
7. This warning message is tricky enough to suppress - so that it encourges supressing all warnings.

[EwoutH]

Thanks for reaching out and your detailed feedback. I will try to get back to you later In more detail, but we're aware of the issues we caused by handeling it this way (especially the one with the warning popping up from internal calls) and are evaluating how to prevent them in the future.

We stabilised the AgentSet in Mesa 2.3.0 (see #2065) and removed the warning, so updating to this version should at least adress that problem.

We are very open to contributions that improve Mesa - especially documentation!

[EwoutH]

In addition to that, feel free to share your opinion on our experimental feature policy.

• Increasing development speed: Experimental features #1909