Integrate BrainParameters with ActionSpec, update BrainParametersDrawer #4718

dongruoping · 2020-12-08T00:30:18Z

Proposed change(s)

Add an ActionSpec in BrainParameters, replace old fields in BrainParameters with corresponding fields in ActionSpec and deprecate the old fields
Add serialization callbacks to propagate ActionSpec fields from the old fields
Update BrainParametersDrawer to allow specifying hybrid actions in editor
ActionSpec is modified to be not read-only

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

JIRA: MLA-1320

Types of change(s)

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

dongruoping · 2020-12-08T18:33:34Z

This change can go into hybrid staging or directly into master if staging is too huge

chriselion · 2020-12-08T23:55:05Z

com.unity.ml-agents/Runtime/Policies/BrainParameters.cs

-        /// The size of the action space.
+        /// The specification of the Action space for the BrainParameters.
+        /// </summary>
+        public ActionSpec VectorActionSpec = new ActionSpec(0, 0, new int[] { });


Can we just call this "ActionSpec"? https://unity.slack.com/archives/C01G1362C23/p1607471246002900

I kept this simply because public ActionSpec ActionSpec = new ActionSpec(); looks a bit confusing.

Is it okay to add new public fields to BrainParameters?

Yes, you can always add new public fields and methods.

We do it in a few other places like this:

ml-agents/com.unity.ml-agents/Runtime/Actuators/ActuatorComponent.cs

Line 22 in e3990b5

public abstract ActionSpec ActionSpec { get; }

and

ml-agents/com.unity.ml-agents/Runtime/Actuators/VectorActuator.cs

Line 75 in e3990b5

public ActionSpec ActionSpec { get; }

I think it's better to have a slightly confusing declaration and simplify how people use it.

com.unity.ml-agents/Editor/BrainParametersDrawer.cs

com.unity.ml-agents/Runtime/Actuators/ActionSpec.cs

vincentpierre · 2020-12-09T19:03:42Z

com.unity.ml-agents/Runtime/Actuators/ActionSpec.cs


        /// <summary>
        /// Get the total number of Discrete Actions that can be taken by calculating the Sum
        /// of all of the Discrete Action branch sizes.
        /// </summary>
-        public int SumOfDiscreteBranchSizes { get; }
+        public int SumOfDiscreteBranchSizes { get { return m_SumOfDiscreteBranchSizes; } set { m_SumOfDiscreteBranchSizes = value; } }


Probably out of scope, but why is this public and not internal? I feel like ActionSpec should be only nContinuous and DiscreteBranchSizes...

Removed m_SumOfDiscreteBranchSizes and m_NumDiscreteActions, and derive them from branch sizes.

vincentpierre · 2020-12-09T19:04:15Z

com.unity.ml-agents/Runtime/Actuators/ActionSpec.cs

+        /// <param name="numDiscreteActions">Number of Discrete Actions</param>
+        /// <param name="branchSizes">The array of branch sizes for the discrete action space.  Each index
+        /// contains the number of actions available for that branch.</param>
+        public void SetDiscrete(int numDiscreteActions, int[] branchSizes)


Can SetDiscrete only take a single branchSizes argument ?

not sure I understand this question.

I think he means public void SetDiscrete(int[] branchSizes).
This makes sense to me; only m_NumContinuousActions and m_DiscreteBranchSizes is necessary in ActionSpec for all information.

vincentpierre · 2020-12-09T19:06:49Z

com.unity.ml-agents/Runtime/Policies/BrainParameters.cs

-        /// The size of the action space.
+        /// The specification of the Action space for the BrainParameters.
+        /// </summary>
+        public ActionSpec VectorActionSpec = new ActionSpec(0, 0, new int[] { });


Is it okay to add new public fields to BrainParameters?

com.unity.ml-agents/Editor/BrainParametersDrawer.cs

surfnerd · 2020-12-09T19:13:07Z

com.unity.ml-agents/Editor/DemonstrationDrawer.cs

@@ -72,16 +72,17 @@ static string BuildIntArrayLabel(SerializedProperty actionSizeProperty)
        /// </summary>
        void MakeActionsProperty(SerializedProperty property)
        {
-            var actSizeProperty = property.FindPropertyRelative("VectorActionSize");
-            var actSpaceTypeProp = property.FindPropertyRelative("VectorActionSpaceType");
+            var actSpecProperty = property.FindPropertyRelative("VectorActionSpec");


Should we move all of our strings to constants? Feels ripe for typo bugs.

Moved them all to const strings

surfnerd · 2020-12-09T19:23:07Z

please run some yamato tests on this branch before merging.

com.unity.ml-agents/Runtime/Actuators/ActionSpec.cs

chriselion · 2020-12-09T22:23:37Z

com.unity.ml-agents/Runtime/Actuators/ActionSpec.cs

+        /// Set action size fields related to continuous actions.
+        /// </summary>
+        /// <param name="numContinuousActions">Number of Continuous Actions</param>
+        public void SetContinuous(int numContinuousActions)


internal?

If you keep this public, I think it should m_BranchSizes to null. As long it's only used in the one or two places that it currently is, it's probably OK as is.

Made it internal.

Add: Originally this is for safely setting discrete fields since there're multiple fields related to discrete spec. Now that there's only one (branch sizes) it's not necessary anymore. I might just remove it.

chriselion · 2020-12-09T22:23:51Z

com.unity.ml-agents/Runtime/Actuators/ActionSpec.cs

+        /// <param name="numDiscreteActions">Number of Discrete Actions</param>
+        /// <param name="branchSizes">The array of branch sizes for the discrete action space.  Each index
+        /// contains the number of actions available for that branch.</param>
+        public void SetDiscrete(int[] branchSizes)


Same comments as SetContinuous

…t format

chriselion · 2020-12-09T22:39:44Z

com.unity.ml-agents/Runtime/Policies/BrainParameters.cs

@@ -61,7 +68,8 @@ public class BrainParameters
        /// For the discrete action space: the number of branches in the action space.
        /// </value>
        [FormerlySerializedAs("vectorActionSize")]
-        public int[] VectorActionSize = new[] { 1 };
+        [FormerlySerializedAs("VectorActionSize")]
+        public int[] VectorActionSizeDeprecated = new[] { 1 };


These are public fields on a public class, so we can't rename them.

I think (@surfnerd correct me if I'm wrong) we can do something like

[FormerlySerializedAs("vectorActionSize")] [FormerlySerializedAs("VectorActionSize")] private int[] m_VectorActionSizeDeprecated; // or maybe internal int[] VectorActionSize { get { /* do something with ActionSpec */ } set { /* do something with ActionSpec */ } }

(and similar for VectorActionSpaceType). The make sure to use the m_* fields (not the properties) in the OnBeforeSerialize methods.

chriselion · 2020-12-09T22:40:23Z

com.unity.ml-agents/Runtime/Policies/BrainParameters.cs

        /// </summary>
-        public int NumActions
+        public int NumActionsDeprecated


Can't rename. We can mark as [Obsolete] to discourage access though.

chriselion · 2020-12-09T22:41:59Z

com.unity.ml-agents/Runtime/Policies/BrainParameters.cs

+        /// <summary>
+        /// Called by Unity immediately before serializing this object.
+        /// </summary>
+        /// <remarks>


Agent has a lot of boilerplate here since it's the main class and users will inherit from it. That shouldn't be the case for BrainParameters, so I think we can trim this down here.

chriselion · 2020-12-09T22:42:52Z

com.unity.ml-agents/Runtime/Policies/BrainParameters.cs

+        /// </example>
+        public void OnAfterDeserialize()
+        {
+            if (!hasUpgradedBrainParametersWithActionSpec)


Same comment on the docstring.

Can we move the logic here and in OnBeforeSerialize to a private UpdateToActionSpec() method?

chriselion · 2020-12-09T22:45:31Z

com.unity.ml-agents/Tests/Editor/Actuators/VectorActuatorTests.cs

@@ -32,24 +32,24 @@ public void WriteDiscreteActionMask(IDiscreteActionMask actionMask)
        public void TestConstruct()
        {
            var ar = new TestActionReceiver();
-            var va = new VectorActuator(ar, new[] { 1, 2, 3 }, SpaceType.Discrete, "name");
+            var va = new VectorActuator(ar, ActionSpec.MakeDiscrete(new int[] { 1, 2, 3 }), "name");


Suggested change

var va = new VectorActuator(ar, ActionSpec.MakeDiscrete(new int[] { 1, 2, 3 }), "name");

var va = new VectorActuator(ar, ActionSpec.MakeDiscrete(1, 2, 3), "name");

MakeDiscrete uses [params](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/params) so you can skip the new int[] and just pass them like multiple args. Not a requirement, just some syntactic sugar.

chriselion · 2020-12-09T23:09:57Z

ml-agents/mlagents/trainers/sac/optimizer_torch.py

@@ -423,7 +426,7 @@ def sac_entropy_loss(
                )
            # We update all the _cont_ent_coef as one block
            entropy_loss += -1 * ModelUtils.masked_mean(
-                _cont_ent_coef * target_current_diff, loss_masks
+                torch.mean(_cont_ent_coef) * target_current_diff, loss_masks


Bad merge? Looks like this is undoing #4725

chriselion · 2020-12-11T22:01:26Z

com.unity.ml-agents/Runtime/Actuators/VectorActuator.cs

-                        "Unknown enum value.");
-            }
+            ActionSpec = actionSpec;
+            string suffix = $"-Continuous-{actionSpec.NumContinuousActions}-Discrete-{actionSpec.NumDiscreteActions}";


This is technically a behavior change. Not sure if there's any observable difference, though - maybe in how the actuators are sorted.

changed it back and only use that with hybrid action

chriselion · 2020-12-11T22:17:16Z

com.unity.ml-agents/Runtime/Policies/BrainParameters.cs

+        /// </summary>
+        public ActionSpec ActionSpec
+        {
+            get { return new ActionSpec(m_NumContinuousActions, m_DiscreteBranchSizes); }


I think this means that someone could do something like

myBrainParams.ActionSpec.NumContinuousActions = 4;

which would compile are run, but it would set NumContinuousActions on the new ActionSpec that gets returned here, but it wouldn't propagate to m_NumContinuousActions.

@surfnerd is there a better pattern for this?

ActionSpec is a value type, so 'new' is redundant here I believe.

without 'new' there will be compile error

Now that the deprecated fields can be synced with corresponding new field, it's actually easier to just bring back ActionSpec and sync with ActionSpec fields to solve this problem.

chriselion · 2020-12-11T23:52:50Z

com.unity.ml-agents/Runtime/Policies/BrainParameters.cs

+            {
+                m_ActionSpec.NumContinuousActions = value.NumContinuousActions;
+                m_ActionSpec.BranchSizes = value.BranchSizes;
+                if (m_ActionSpec.NumContinuousActions == 0)


nit: can this block be replaced with SyncDeprecatedActionFields()?

chriselion · 2020-12-12T00:00:00Z

com.unity.ml-agents/CHANGELOG.md

@@ -10,11 +10,10 @@ and this project adheres to
 ## [Unreleased]
 ### Major Changes
 #### com.unity.ml-agents (C#)
+- Agent with both continuous and discrete actions is now supported. You can specify


nit: This is still a minor change from semantic versioning point of view. We'll list it as "major" for the release notes though.

Also, please keep the old PR# (#4702) and add this one.

was following the python changelog. Is the python changes also minor?

dongruoping mentioned this pull request Dec 8, 2020

[WIP] Integrate BrainParameters with ActionSpec, update BrainParametersDrawer #4700

Closed

10 tasks

dongruoping changed the title ~~[WIP] Integrate BrainParameters with ActionSpec, update BrainParametersDrawer~~ Integrate BrainParameters with ActionSpec, update BrainParametersDrawer Dec 8, 2020

dongruoping requested review from chriselion, surfnerd and vincentpierre December 8, 2020 18:33

chriselion reviewed Dec 8, 2020

View reviewed changes

vincentpierre reviewed Dec 9, 2020

View reviewed changes

surfnerd reviewed Dec 9, 2020

View reviewed changes

com.unity.ml-agents/Editor/BrainParametersDrawer.cs Outdated Show resolved Hide resolved

surfnerd reviewed Dec 9, 2020

View reviewed changes

surfnerd approved these changes Dec 9, 2020

View reviewed changes

delete-merged-branch bot deleted the branch master December 9, 2020 22:13

chriselion reviewed Dec 9, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Actuators/ActionSpec.cs Show resolved Hide resolved

chriselion reviewed Dec 9, 2020

View reviewed changes

Ruo-Ping Dong added 14 commits December 9, 2020 14:28

make actionSpec not read-only

e9b9af6

add actionSpec in BrainParameters and update BrainParameters Drawer

4840d09

Integrate BrainParameters with ActionSpec, deprecate some fields

d7080be

add serialization callbacks

a1c81e9

add function to set ActionSpec

178e095

update BrainParameters when changed in editor

accf0d5

fix tests

05a3c6b

enable hybrid ParameterLoaderTest

3e3ebaa

fix tests

cc227fd

fix demonstration drawer

351a479

add comments

6864686

rename method

397fcf1

check both continuous/discrete output shape regardless of model outpu…

0bf75e3

…t format

simplify actionspec. put property names in const string

91d4125

Ruo-Ping Dong added 2 commits December 9, 2020 15:05

rename VectorActionSpec to ActionSpec

ddde653

remove unused import

b42a740

chriselion reviewed Dec 9, 2020

View reviewed changes

Ruo-Ping Dong added 8 commits December 9, 2020 15:18

fix bad merge

f9336eb

pass multiple args instead of new array to makeDiscrete()

2d0f9da

change back public fields' name

ca778e3

change back public fields' name

8e9977b

fix clone brainParameter

a5b7302

remove setContinuous and setDiscrete

ede61b2

Merge branch 'master' into develop-hybrid-brainparameters2

4f32b56

update changelog

59285bd

dongruoping closed this Dec 10, 2020

dongruoping reopened this Dec 10, 2020

Ruo-Ping Dong added 4 commits December 9, 2020 17:47

fix internal field serialization

b24c013

revert breaking change

51587de

sync deprecated fields

b6c62d8

fix BrainParameterToProto

cd28e02

chriselion reviewed Dec 11, 2020

View reviewed changes

Ruo-Ping Dong added 3 commits December 11, 2020 14:44

change back VectorActuator suffix

b1d04b5

add back ActionSpec

4219e5c

set deprecated to null

aa3d799

chriselion reviewed Dec 11, 2020

View reviewed changes

replace with SyncDeprecatedActionFields

b64d85a

chriselion reviewed Dec 12, 2020

View reviewed changes

chriselion approved these changes Dec 12, 2020

View reviewed changes

update changelog

d7c8298

dongruoping merged commit 4a8de9e into master Dec 12, 2020

delete-merged-branch bot deleted the develop-hybrid-brainparameters2 branch December 12, 2020 07:21

github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2021

	var va = new VectorActuator(ar, ActionSpec.MakeDiscrete(new int[] { 1, 2, 3 }), "name");
	var va = new VectorActuator(ar, ActionSpec.MakeDiscrete(1, 2, 3), "name");

Integrate BrainParameters with ActionSpec, update BrainParametersDrawer #4718

Integrate BrainParameters with ActionSpec, update BrainParametersDrawer #4718

Uh oh!

Conversation

dongruoping commented Dec 8, 2020

Proposed change(s)

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

Types of change(s)

Checklist

Other comments

Uh oh!

dongruoping commented Dec 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surfnerd commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongruoping Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

surfnerd commented Dec 9, 2020 •

edited

Loading

dongruoping Dec 9, 2020 •

edited

Loading