Develop remove memories #2795

vincentpierre · 2019-10-24T20:21:33Z

In this pull request :

Memories are no longer being communicated between C# and Python
Removed memories from AgentInfo and AgentAction
Memories are now stored in the python Policy on Python
Memories are now stored in the ModelRunner on C#

…s in proto

ervteng

🚢 🇮🇹 - Okay, except for the failing protobuf test

chriselion · 2019-10-24T21:09:44Z

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/TensorApplier.cs

@@ -56,7 +61,7 @@ public interface IApplier
                m_Dict[TensorNames.ActionOutput] =
                    new DiscreteActionOutputApplier(bp.vectorActionSize, seed, allocator);
            }
-            m_Dict[TensorNames.RecurrentOutput] = new MemoryOutputApplier();
+            // m_Dict[TensorNames.RecurrentOutput] = new MemoryOutputApplier();


chriselion · 2019-10-24T21:14:14Z

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/TensorApplier.cs

+            /// <param name="memories">
+            /// The memories of all the agents
+            /// </param>
+            void Apply(TensorProxy tensorProxy,


Rather than pass this everywhere, can the two classes that actually need it (BarracudaRecurrentInputGenerator and BarracudaMemoryOutputApplier) save a reference to it?

chriselion · 2019-10-24T21:15:32Z

UnitySDK/Assets/ML-Agents/Scripts/Policy/BarracudaPolicy.cs

@@ -18,7 +19,7 @@ public enum InferenceDevice
    public class BarracudaPolicy : IPolicy
    {

-        protected IBatchedDecisionMaker m_BatchedDecisionMaker;
+        protected ModelRunner m_BatchedDecisionMaker;


Rename to m_ModelRunner?

ml-agents/mlagents/trainers/tf_policy.py

chriselion · 2019-10-24T21:21:04Z

ml-agents/mlagents/trainers/tf_policy.py

+    def save_memories(self, agent_ids, memory_matrix):
+        if not isinstance(memory_matrix, np.ndarray):
+            return
+        for index, id in enumerate(agent_ids):


nit: agent_id instead of id (id() is a built-in function)

chriselion · 2019-10-24T21:22:33Z

ml-agents/mlagents/trainers/tf_policy.py

@@ -56,6 +56,7 @@ def __init__(self, seed, brain, trainer_parameters):
        self.seed = seed
        self.brain = brain
        self.use_recurrent = trainer_parameters["use_recurrent"]
+        self.memory_dict = {}


Suggested change

self.memory_dict = {}

self.memory_dict: Dict[int, np.ndarray] = {}

chriselion · 2019-10-24T21:23:59Z

ml-agents/mlagents/trainers/tf_policy.py

@@ -169,6 +177,24 @@ def make_empty_memory(self, num_agents):
        """
        return np.zeros((num_agents, self.m_size))

+    def save_memories(self, agent_ids, memory_matrix):


Suggested change

def save_memories(self, agent_ids, memory_matrix):

def save_memories(self, agent_ids: List[int], memory_matrix: np.ndarray) -> None:

chriselion · 2019-10-24T21:25:24Z

ml-agents/mlagents/trainers/tf_policy.py

+        for index, id in enumerate(agent_ids):
+            self.memory_dict[id] = memory_matrix[index, :]
+
+    def retrieve_memories(self, agent_ids):


Suggested change

def retrieve_memories(self, agent_ids):

def retrieve_memories(self, agent_ids: List[int]):

chriselion · 2019-10-24T21:25:59Z

ml-agents/mlagents/trainers/tf_policy.py

+
+    def retrieve_memories(self, agent_ids):
+        memory_matrix = np.zeros((len(agent_ids), self.m_size))
+        for index, id in enumerate(agent_ids):


Same comment about id

ml-agents/mlagents/trainers/tf_policy.py

chriselion

Some of it's optional, but definite need to fix the id in ... .keys()

ml-agents/mlagents/trainers/tf_policy.py

chriselion · 2019-10-25T16:32:17Z

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/ApplierImpl.cs

@@ -187,7 +193,7 @@ public void Apply(TensorProxy tensorProxy, IEnumerable<Agent> agents)

            foreach (var agent in agents)
            {
-                var memory = agent.GetMemoriesAction();
+                var memory = m_Memories.ContainsKey(agent.Info.id) ? m_Memories[agent.Info.id] : null;


@surfnerd is there a better way to do this (in one lookup)? We were trying m_Memories.ElementAtOrDefault(id).Value but that wasn't behaving as expected.

ElementAtOrDefault takes an index as a parameter. That probably explains the unexpected behavior. You can always use TryGetValue which will set the out value parameter as the default value if it is not in the Dictionary.

🤦‍♂ So

List<float> memories = null; m_Memories.TryGetValue(agent.Info.id, memories);

?

yeah, you could combine it with the if statement down below as well.

List<float> memory = null; if (!m_Memories.TryGetValue(agent.Info.id, out memory) || memory.Count < memorySize * m_MemoriesCount) { ... } ...

chriselion · 2019-10-25T16:36:28Z

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/ModelRunner.cs

-            m_TensorGenerator = new TensorGenerator(brainParameters, seed, m_TensorAllocator, barracudaModel);
-            m_TensorApplier = new TensorApplier(brainParameters, seed, m_TensorAllocator, barracudaModel);
+            m_TensorGenerator = new TensorGenerator(
+                brainParameters, seed, m_TensorAllocator, ref m_Memories, barracudaModel);


Don't think we need ref here now, right?

chriselion · 2019-10-25T16:37:03Z

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/TensorGenerator.cs

@@ -51,16 +55,16 @@ public interface IGenerator
                new SequenceLengthGenerator(allocator);
            m_Dict[TensorNames.VectorObservationPlacholder] =
                new VectorObservationGenerator(allocator);
-            m_Dict[TensorNames.RecurrentInPlaceholder] =
-                new RecurrentInputGenerator(allocator);
+            // m_Dict[TensorNames.RecurrentInPlaceholder] =


nit: dead code

ml-agents/mlagents/trainers/ppo/policy.py

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/GeneratorImpl.cs

surfnerd · 2019-10-25T16:51:17Z

UnitySDK/Assets/ML-Agents/Editor/Tests/EditModeTestInternalBrainTensorApplier.cs

@@ -44,8 +45,8 @@ public void ApplyContinuousActionOutput()
        {
            var inputTensor = new TensorProxy()
            {
-                shape = new long[] {2, 3},
-                data = new Tensor(2, 3, new float[] {1, 2, 3, 4, 5, 6})
+                shape = new long[] { 2, 3 },


gahhh. whitespace noise

UnitySDK/Assets/ML-Agents/Scripts/ICommunicator.cs

chriselion · 2019-10-25T16:53:25Z

ml-agents/mlagents/trainers/tf_policy.py

+            return ActionInfo([], [], [], None)
+
+        self.remove_memories(
+            [


If you wanted to be a bit more pythonic, I think you could do

[ agent for agent, done in zip(brain_info.agents, brain_info.local_done) if done ]

but no worries if you like the current way better.

chriselion

Overall this looks great, just some minor feedback.

It would be good to run a before-and-after timing on a simple training scene like Hallway to so we can brag about any speedups in the release notes (and make sure it didn't somehow get worse).

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/TensorApplier.cs

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/TensorGenerator.cs

UnitySDK/Assets/ML-Agents/Scripts/Policy/RemotePolicy.cs

chriselion · 2019-10-25T17:50:10Z

Looks like bair links are down again. OK to merge with that test failing.

vincentpierre added 8 commits October 23, 2019 13:49

Initial commit removing memories from C# and deprecating memory field…

5624d00

…s in proto

initial changes to Python

33de00e

Adding functionalities

0b8dded

Fixes

ef57029

adding the memories to the dictionary

a0709ff

Fixing bugs

c72751b

tweeks

7a17eb4

Resolving bugs

3604c24

vincentpierre requested review from ervteng and chriselion October 24, 2019 20:21

vincentpierre self-assigned this Oct 24, 2019

ervteng approved these changes Oct 24, 2019

View reviewed changes

chriselion reviewed Oct 24, 2019

View reviewed changes

ml-agents/mlagents/trainers/tf_policy.py Show resolved Hide resolved

chriselion reviewed Oct 24, 2019

View reviewed changes

ml-agents/mlagents/trainers/tf_policy.py Outdated Show resolved Hide resolved

Recreating the proto

d7cf784

chriselion reviewed Oct 24, 2019

View reviewed changes

ml-agents/mlagents/trainers/tf_policy.py Outdated Show resolved Hide resolved

chriselion suggested changes Oct 24, 2019

View reviewed changes

chriselion reviewed Oct 24, 2019

View reviewed changes

ml-agents/mlagents/trainers/tf_policy.py Outdated Show resolved Hide resolved

vincentpierre added 4 commits October 24, 2019 15:05

Addressing comments

5f01e4c

Passing by reference does not work. Do not merge

d9e72b6

Fixing huge bug in Inference

876e30f

Applying patches

6595e76

fixing tests

b1b90f6

vincentpierre requested a review from chriselion October 25, 2019 00:19

chriselion reviewed Oct 25, 2019

View reviewed changes

ml-agents/mlagents/trainers/ppo/policy.py Show resolved Hide resolved

surfnerd reviewed Oct 25, 2019

View reviewed changes

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/GeneratorImpl.cs Outdated Show resolved Hide resolved

surfnerd reviewed Oct 25, 2019

View reviewed changes

UnitySDK/Assets/ML-Agents/Scripts/ICommunicator.cs Show resolved Hide resolved

chriselion reviewed Oct 25, 2019

View reviewed changes

chriselion approved these changes Oct 25, 2019

View reviewed changes

surfnerd reviewed Oct 25, 2019

View reviewed changes

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/TensorApplier.cs Outdated Show resolved Hide resolved

surfnerd reviewed Oct 25, 2019

View reviewed changes

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/TensorGenerator.cs Outdated Show resolved Hide resolved

surfnerd reviewed Oct 25, 2019

View reviewed changes

UnitySDK/Assets/ML-Agents/Scripts/Policy/RemotePolicy.cs Outdated Show resolved Hide resolved

vincentpierre added 3 commits October 25, 2019 10:18

Addressing comments

b6bf28f

Renaming variable to reflect type

d794531

test

78e4a31

surfnerd approved these changes Oct 25, 2019

View reviewed changes

vincentpierre merged commit 58cee7e into develop Oct 25, 2019

delete-merged-branch bot deleted the develop-remove-memories branch October 25, 2019 17:56

github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop remove memories #2795

Develop remove memories #2795

vincentpierre commented Oct 24, 2019

ervteng left a comment •

edited

Loading

chriselion Oct 24, 2019

chriselion Oct 24, 2019

chriselion Oct 24, 2019

chriselion Oct 24, 2019

chriselion Oct 24, 2019

chriselion Oct 24, 2019

chriselion Oct 24, 2019

chriselion Oct 24, 2019

chriselion left a comment

chriselion Oct 25, 2019

surfnerd Oct 25, 2019 •

edited

Loading

chriselion Oct 25, 2019

surfnerd Oct 25, 2019 •

edited

Loading

chriselion Oct 25, 2019

surfnerd Oct 25, 2019

chriselion Oct 25, 2019

surfnerd Oct 25, 2019

chriselion Oct 25, 2019

chriselion left a comment

chriselion commented Oct 25, 2019

	self.memory_dict = {}
	self.memory_dict: Dict[int, np.ndarray] = {}

	def save_memories(self, agent_ids, memory_matrix):
	def save_memories(self, agent_ids: List[int], memory_matrix: np.ndarray) -> None:

	def retrieve_memories(self, agent_ids):
	def retrieve_memories(self, agent_ids: List[int]):

Develop remove memories #2795

Develop remove memories #2795

Conversation

vincentpierre commented Oct 24, 2019

ervteng left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriselion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surfnerd Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surfnerd Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriselion left a comment

Choose a reason for hiding this comment

chriselion commented Oct 25, 2019

ervteng left a comment •

edited

Loading

surfnerd Oct 25, 2019 •

edited

Loading

surfnerd Oct 25, 2019 •

edited

Loading