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

Automate and enforce unique unique_id in Agents #2213

Closed
EwoutH opened this issue Aug 15, 2024 · 14 comments · Fixed by #2260
Closed

Automate and enforce unique unique_id in Agents #2213

EwoutH opened this issue Aug 15, 2024 · 14 comments · Fixed by #2260
Labels
breaking Release notes label
Milestone

Comments

@EwoutH
Copy link
Member

EwoutH commented Aug 15, 2024

Currently, Agent's unique_id values are assigned manually on Agent creation, and not guaranteed to be unique. This issue:

  • Automatically assign agents an unique_id value on creation.
  • Guarantee that all unique_id values are actually unique.
@GittyHarsha
Copy link

GittyHarsha commented Aug 19, 2024

Hi @EwoutH here is my initial proposal:
change the __init__ function of Agent class to the below:

def __init__(self, model: Model) -> None:
        """
        Create a new agent.

        Args:
            model (Model): The model instance in which the agent exists.
        """
        self.unique_id = model.next_id()
        self.model = model
        self.pos: Position | None = None

        # register agent
        try:
            self.model.agents_[type(self)][self] = None
        except AttributeError as err:
            # model super has not been called
            raise RuntimeError(
                "The Mesa Model class was not initialized. You must explicitly initialize the Model by calling super().__init__() on initialization."
            ) from err

Basically, I used next_id() member of Model class as it maintains current_id: A counter for assigning unique IDs to agents.

@EwoutH
Copy link
Member Author

EwoutH commented Aug 19, 2024

I think this is quite clean!

We could even make this less breaking by making model.next_id a property, and then setting that as the default value for agent.unique_id, so you can overwrite it if you want to, but also it doesn't break current builds.

@quaquel @rht another thought of me was maybe handeling this as an Agent Class attribute. What's the way you would implement this?

@quaquel
Copy link
Member

quaquel commented Aug 19, 2024

In my view, the counter stuff should be in the Agent Class not the Model class. There are some metaprogramming things possible depending on how we want to handle it. Do we want a single counter across all Agent subclasses or count seperately for different subclasses? I don't think a property is required nor is users overwriting this desirable since it might brake other parts of MESA that depend on unique_id (e.g., some future version of data collection).

@GittyHarsha
Copy link

@quaquel

In my view, the counter stuff should be in the Agent Class not the Model class

in this case, how can the agent IDs be unique in a particular model?

@EwoutH EwoutH added breaking Release notes label and removed good first issue labels Aug 19, 2024
@EwoutH
Copy link
Member Author

EwoutH commented Aug 19, 2024

Do we want a single counter across all Agent subclasses or count seperately for different subclasses

I was also thinking about this, I'm not sure what's better. I'm nudging towards the former.

users overwriting this desirable

Agreed, users can define other things if they want.


I don't want to break every Mesa model currently in existence in a hard way.

There are two cases we have to look out for:

  • Agents defined with positional arguments (MyAgent(i, self))
  • Agents defined with keyword arguments (MyAgent(unique_id=i, model=self))

Both should be let down gracefully (a warning for now), but also with a clear migration path to what to do next.

@GittyHarsha sorry, this issue is probably a bit more complicated then I thought.

@EwoutH
Copy link
Member Author

EwoutH commented Aug 19, 2024

Actually I think it’s quite doable, with some type checking on the input arguments. @GittyHarsha do you want to give it a shot with your current proposal? If so, feel free to open a draft PR!

Even if we go in another direction it will be useful to have figured out how to handle warnings for current models.

@quaquel
Copy link
Member

quaquel commented Aug 19, 2024

I was also thinking about this, I'm not sure what's better. I'm nudging towards the former.

me too

I don't want to break every Mesa model currently in existence in a hard way.

The simplest solution is to issue a warning once in the next release of MESA 2, while properly rerigging everything in MESA 3.

@EwoutH
Copy link
Member Author

EwoutH commented Aug 19, 2024

I was going for type checking to check if the first argument in Agent() is a Model instance or not. If not give a warning and handle gracefully, but note that it's deprecated. Then in Mesa 3.1 change the warning to an error, in 3.2 remove the code path.

@GittyHarsha
Copy link

@GittyHarsha do you want to give it a shot with your current proposal? If so, feel free to open a draft PR!

Sure! I'm grateful for the oppurtunity

@quaquel
Copy link
Member

quaquel commented Aug 19, 2024

I was going for type checking to check if the first argument in Agent() is a Model instance or not. If not give a warning and handle gracefully, but note that it's deprecated. Then in Mesa 3.1 change the warning to an error, in 3.2 remove the code path.

Why create such a long lasting impact on MESA 3? In my perception, with MESA 3 we can start with a clean sheat and brake stuff to make it better.

@EwoutH
Copy link
Member Author

EwoutH commented Aug 19, 2024

You know by now I like to break things if that helps move us forward. I've broken enough to prove that (ask @rht).

But to break literately every mesa model built ever in existence is even for me a bit fanatic. If we can circumvent that with a few lines of code and a properly written warning, I think we should do that.

So if possible, there should be at least one version where the old one and the new both work, and where a warning is given how to move to the new one. The next one 3.0 should be that one. Because we don't support it in 3.0, we tolerate it with a warning to don't expect that we tolerate it much longer.

GittyHarsha added a commit to GittyHarsha/mesa that referenced this issue Aug 20, 2024
@GittyHarsha
Copy link

GittyHarsha commented Aug 20, 2024

@EwoutH I have opened a draft PR

Follow-ups

  1. Should I make next_item as a property of the model?
  2. should I add a warning when a user doesn't use automatic unique_id?
  3. Could you give pointers for adding tests and documentation for the changes introduced?

@quaquel
Copy link
Member

quaquel commented Aug 20, 2024

So if possible, there should be at least one version where the old one and the new both work, and where a warning is given how to move to the new one.

Which was why I was suggeting another MESA 2 release with this (and perhaps other?) warnings in preperation for MESA 3.

@EwoutH EwoutH added this to the v3.0 milestone Sep 3, 2024
@EwoutH
Copy link
Member Author

EwoutH commented Sep 4, 2024

Implemented in #2260.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Release notes label
Projects
None yet
3 participants