MultiAgentGroup Interface

[dongruoping]

Proposed change(s)

Add MultiAgentGroup Interface that handles Agent groupId and groupReward to support multi-agent training.

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

Types of change(s)

• Bug fix
• New feature
• Code refactor
• Breaking change
• Documentation update
• Other (please describe)

Checklist

• Added tests that prove my fix is effective or that my feature works
• Updated the changelog (if applicable)
• Updated the documentation (if applicable)
• Updated the migration guide (if applicable)

Other comments

[vincentpierre]

So the convention is that -1 is "no team".

[dongruoping]

It actually should be 0 since it's what it would be in proto if using old version of C#.

[vincentpierre]

Is there anything preventing the user from adding team rewards manually? How will these be handled if there is no TeamManager for the Agent ?

[chriselion]

What about making this method internal and only calling it from BaseTeamManager methods instead?

[dongruoping]

How will these be handled if there is no TeamManager for the Agent ?

Team reward will be processed (sum, mean, etc) across the team in python so if it has no TeamManager it will be processed across a team of itself. It's not meaningful in that case but it will run.

Making it internal.

[vincentpierre]

Is the user expected to call SetTeamManager directly or is this hidden from the user?

[dongruoping]

User is expected to call SetTeamManager() to associate an agent with a manager.

[dongruoping]

This has changed after removing cyclic reference between agent and manager. It's now internal and called by TeamManager.RegisterAgent()

[vincentpierre]

ITeamManager holds reference to a list of Agent and Agent has a reference to iTeamManager. This is a cyclic reference I think we should avoid.
We could get away with it if we made Agent implement an interface and have ITeamManager hold a list of objects that implements that new interface.

[chriselion]

It looks like the Agent doesn't really need the TeamManager that much. Maybe it could avoid storing the reference to it altogether; you'd mostly need to

• add an internal method on Agent to set the teamManagerId and have BaseTeamManager call it
• make sure Agent unregisters itself with the teamManager when it gets destroyed. I think we could handle this with a delegate on Agent.

[dongruoping]

Removed TeamManager reference in agent and unregister agent using delegate.

[vincentpierre]

Could we make this class more generic and have Agent also use this for their AgentId?

[dongruoping]

AgentId (or actually Episode ID) uses EpisodeIdCounter which is almost the same thing. It's a static class so I figured I'd make a separate class. Should I try to integrate them?

[chriselion]

I know that Agent currently has both AddReward and SetReward, but that causes some confusion among users. Can we just pick one and stick with it here?

[dongruoping]

Had a discussion around this and we still think it's useful to have both and also keep the parity with agent Add/SetReward.

[chriselion]

Does this need to be called after AgentAct?.Invoke(), or can it possible go on the same delegate? If it needs to go after, can we call it something more generic like PostAgentAct?

[dongruoping]

This is basically replacing the MaxStepReached logic in agent. We will want all agents to finish acting first and then call MaxStepReached so I think it needs to go after. Also it should only be invoked once instead of n_agent times so not in the same delegate as AgentAct.
Will change the name.

[vincentpierre]

I think we want to put this in OnDisable. I think the user would easily make a custom OnDestroy implementation for their Agents (overriding this)

[dongruoping]

I thought about that before but that means the user has to link the agent with the same TeamManager again every time the agent is disabled and re-enabled. So I thought putting it in destroy might be lesser evil. For existing codes that are not using TeamManager, overriding it is fine.
Also if you want to do something to each agent in OnTeamEpisodeBegin() for next episode, the unregistered agent might not be there.

But that's a good point. Is adding documentation telling user to call base.Destroy() enough or you think we should do it in OnDisable?

[vincentpierre]

I don't think it is wise to expect the user will consistently call base.Destroy(. I think we should keep all of our logic in OnDisable (which is also called when the Agent is Destroyed).
I really think we should not introduce any distinction between disabling and destroying the Agent. both should be doing the same thing (as it is the case today). We should not introduce a distinction now.

[dongruoping]

expect the user will consistently call base.Destroy

We already expect that with OnEnable() and OnDisable() don't we?

It's a bit questionable to me about whether we want to introduce the distinction. We already introduce some distinctions to the workflows and we decoupled disabling an agent and ending an episode. Theoretically you can disable and re-enable an agent in the same team episode but not with destroy.
Having to set TeamManager for disabled agent every episode (or even within episode) and getting different results from calling GetRegisteredAgents() at different time is what actually bothers me.

That said, I agree with your concern about consistency and I can change this.

[ervteng]

Some minor doc things but looks solid

[vincentpierre]

Is this event really needed? Can you add a comment to justify why and when the event is called?

[dongruoping]

This is the delegate for agent to trigger removing itself from the group and is added to avoid cyclic reference between agent and group. The agent needs a way to clean itself up from the group when disabled/destroyed/assigned to another group.

I'll add comments.

[chriselion]

This might be better with a more generic name, like OnAgentDisabled.

[dongruoping]

didn't named it OnAgentDisabled since it was also triggered in SetMultiAgentGroup. Now that is removed I changed the name.

[vincentpierre]

Instead of relying on another callback, is there a way to simply check if the Agent is still alive and remove it from the list if it is not ?

[dongruoping]

If we would like the group to constantly check on agents instead of agent triggering it, it will probably needs to be a monobehaviour to do this check in every FixedUpdate (or passively check it when any method is called which sounds buggy). Also this doesn't work if the agent is still alive but assigned to another group.

[vincentpierre]

InterruptGroupEpisode for symmetry with EndGroupEpisode ?

[dongruoping]

I made this for symmetry with Agent.EpisodeInterrupted() in the hope that user can map EndEpisode/EpisodeInterrupted to EndGroupEpisode/GroupEpisodeInterrupted

[chriselion]

m_StepCount and m_GroupMaxStep appear to be unused now?

[chriselion]

I think if you want the agent to unregister from all groups, you should allow passing a null multiAgentGroup here. This would be better as something like

if(multiAgentGroup == null)
{
  m_GroupId == 0;
}
else
{
  var newId = multiAgentGroup.GetId();
  if(m_GroupId == 0 || m_GroupId == newId)
  {
    // Idempotent, so setting the same group has no effect
    m_GroupId = newId;
  }
  else 
  {
    throw new UnityAgentsException("Agent is already registered with a group. Unregister it first.");
  }
}

[dongruoping]

I made it internal and only being called in MultiAgentGroup.RegisterAgent so I don't see there's any cases that it can be passed in a null group. Also it shouldn't be unregistered by just overwriting m_GroupId otherwise there would be delegates left in Agent.UnregisterFromGroup.

I can do throwing an exception if a user trying to assign an agent from one group directly to another group without unregistering from the first, instead of call unregister for them.

[chriselion]

I think you need to do something to the Agent here to reset its group ID (and maybe zero out its team reward). You should either make Agent.SetMultiAgentGroup() take null and set the ID to 0 in that case, or add Agent.ClearMultiAgentGroup() and do it there.

[chriselion]

Basically, I think you should have a test like

IMultiAgentGroup coach = ...
Agent agent = ....

coach.RegisterAgent(agent);
coach.AddTeamReward(1.0);
coach.UnregisterAgent(agent);

Assert.AreEqual(0, agent.groupId);
Assert.AreEqual(0.0f, agent.groupReward); // maybe

[dongruoping]

Update accordingly except the group reward part. I kept the group reward reset cycle the same as the individual reward to avoid it being zeroed out before sending to trainer/policy. Also added tests.

[chriselion]

Please add unit tests for code coverage:

• You can make a simple test implementation of IMultiAgentGroup in com.unity.ml-agents tests and make sure it calls the Agent method and delegates.
• Add a test for BaseMultiAgentGroup in com.unity.ml-agents.extensions and test the "bookkeeping". Make sure adding and removing idempotent, etc.

[chriselion]

Overall looks really good. I like that the new interface and changes to Agent are very minimal. BaseMultiAgentGroup is probably simple enough that we can move into the main package in a release or two.

[chriselion]

virtual too?

[chriselion]

Would be nice to return this as a readonly interface so that the user doesn't mess with it. It looks like IReadOnlyCollection<Agent> might be the best bet: https://stackoverflow.com/a/36923075/224264

[chriselion]

Last small bits of feedback. Looks good though!