Barracuda inference for hybrid actions #4611

dongruoping · 2020-10-29T01:13:35Z

Proposed change(s)

Barracuda inference for hybrid actions.

Major changes:

1. The input argument lastActions to TensorApplier.IApplier is now an ActionBuffers instead of float[].

2. Changes to model output nodes

To support hybrid actions, deprecated old action output nodes which only work for single-type actions and added new output nodes:

Deprecated action in favor of continuous_actions and discrete_actions
Similarly, deprecated action_output_shape in favor of continuous_action_output_shape and discrete_action_output_shape
is_continuous_control is also deprecated

Now the expectation of the model outputs is either set of:

(old case) action, action_output_shape, is_continuous_control
(new case) continuous_actions, discrete_actions, continuous_action_output_shape, discrete_action_output_shape

Running inference will use the new nodes by default, and fall back to old ones if they doesn't exist.
Deprecated nodes are still supported and the code is able to run old models.

Version compatibility:
old model, old C# - current behavior, use old output nodes
new model, old C# - use old output nodes just like current behavior, new nodes are ignored
old model, new C# - detect that new output nodes do not exist in the model and fall back to old ones
new model, new C# - use new output nodes

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

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-10-29T01:16:13Z

com.unity.ml-agents/Runtime/Inference/ApplierImpl.cs

                    {
-                        actionValue = new float[actionSize];
-                        lastActions[agentId] = actionValue;
+                        actionBuffer = new ActionBuffers(new float[m_ActionSpec.NumContinuousActions],


What I don't like about this is ContinuousActionOutputApplier has to initialize new ActionBuffers based on discrete actions branchSizes.

Maybe make another ActionBuffers constructor (or static method) that takes an ActionSpec?

Yes I can do that.
But even with that I'm still not able to initialize continuous/discrete field separately and still need both sizes to initialize the new ActionBuffers here, due to ActionBuffers's read-only properties. I wonder if it's reasonable to change that.

I don't think initializing them separately is a requirement; you have all the information you need in the ActionSpec. Encapsulating it in an ActionBuffers static method seems fine to me.

You could also move the logic to a ModelRunner method, and call it in ModelRunner.DecideBatch(), just before the call to m_TensorApplier.ApplyTensors.

I think it is also the case that the DiscreteActionOutputApplier initializes ActionBuffers. Maybe we could have a InitializeBuffers method that gets called by both ContinuousActionOutputApplier and DiscreteActionOutputApplier if the ActionBuffers does not yet exist,.

com.unity.ml-agents/Runtime/Inference/ApplierImpl.cs

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

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

com.unity.ml-agents/Runtime/Inference/TensorNames.cs

vincentpierre · 2020-10-30T17:49:05Z

com.unity.ml-agents/Runtime/Inference/ApplierImpl.cs

                    {
-                        actionValue = new float[actionSize];
-                        lastActions[agentId] = actionValue;
+                        actionBuffer = new ActionBuffers(new float[m_ActionSpec.NumContinuousActions],


I think it is also the case that the DiscreteActionOutputApplier initializes ActionBuffers. Maybe we could have a InitializeBuffers method that gets called by both ContinuousActionOutputApplier and DiscreteActionOutputApplier if the ActionBuffers does not yet exist,.

vincentpierre · 2020-10-30T17:50:18Z

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

@@ -16,7 +16,7 @@ internal class BarracudaModelParamLoader
    {
        enum ModelActionType
        {
-            Unknown,
+            Hybrid,


I think we should still leave Unknown to be 0, and make Hybrid the third option

vincentpierre · 2020-10-30T17:51:52Z

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

-            names.Add(TensorNames.ActionOutput);
+            if (!model.outputs.Contains(TensorNames.ContinuousActionOutput) && !model.outputs.Contains(TensorNames.DiscreteActionOutput))
+            {
+                names.Add(TensorNames.ActionOutputDeprecated);


I like the change, I think this is a very complex part of the codebase that we do not modify often, so do not hesitate to add some code comments.

vincentpierre · 2020-10-30T17:53:18Z

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

@@ -164,9 +178,6 @@ public static string[] GetOutputNames(Model model)

            var modelApiVersion = (int)model.GetTensorByName(TensorNames.VersionNumber)[0];
            var memorySize = (int)model.GetTensorByName(TensorNames.MemorySize)[0];
-            var isContinuousInt = (int)model.GetTensorByName(TensorNames.IsContinuousControl)[0];


You are modifying the expectation on the constants and outputs of the model. I think you should summarize in the PR description the overall changes made and how older models will work with the new code (and vice versa)

updated in the description

vincentpierre · 2020-10-30T17:55:58Z

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

+            }
+            else
+            {
+                var isContinuous = model.GetTensorByName(TensorNames.IsContinuousControl)[0] > 0;


Is IsContinuousControl deprecated? If it is the case, rename to IsContinuousControlDeprecated

yes, changed it as suggested

vincentpierre · 2020-10-30T17:56:18Z

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

+                modelActionType = GetActionType(modelContinuousActionSize > 0, modelDiscreteActionSize > 0);
+            }
+            else
+            {


Suggested change

{

{

// For backwards compatibility

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

vincentpierre · 2020-10-30T17:59:49Z

com.unity.ml-agents/Runtime/Inference/TensorNames.cs

@@ -23,12 +23,16 @@ internal static class TensorNames
        public const string MemorySize = "memory_size";
        public const string VersionNumber = "version_number";
        public const string IsContinuousControl = "is_continuous_control";
-        public const string ActionOutputShape = "action_output_shape";
-        public const string ActionOutput = "action";
+        public const string ActionOutputShapeDeprecated = "action_output_shape";


Separate the Deprecated section from the rest with a blank line for clarity

Move ActionOutputShapeDeprecated at the bottom with ActionOutputDeprecated

vincentpierre · 2020-11-03T18:20:39Z

com.unity.ml-agents/Tests/Editor/ParameterLoaderTest.cs

@@ -157,7 +157,7 @@ public void TestGetOutputTensors1()
        {
            var model = ModelLoader.Load(continuous2vis8vec2actionModel);


I think you made a lot of improvements to the model loading logic. I think you should add more tests to check your logic (loading an old model and a new model and see if they behave properly)

More tests with new models added

com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs

chriselion · 2020-11-04T01:38:58Z

com.unity.ml-agents/Runtime/Inference/TensorApplier.cs

            if (actionSpec.NumContinuousActions > 0)
            {
-                m_Dict[TensorNames.ActionOutput] = new ContinuousActionOutputApplier();
+                var tensorName = useDeprecated ? TensorNames.ActionOutputDeprecated : TensorNames.ContinuousActionOutput;


model.ContinuousOutputName()?

Looks like it's possible that the model is null here

chriselion · 2020-11-04T01:45:18Z

com.unity.ml-agents/Runtime/Inference/TensorApplier.cs

-            actionSpec.CheckNotHybrid();
-
+            bool useDeprecated = false;
+            if (barracudaModel != null)


Can we just early out if the model is null? Might make some of the code cleaner. (same for TensorGenerator below)

chriselion · 2020-11-05T01:42:06Z

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

@@ -72,12 +65,9 @@ public static int GetNumVisualInputs(Model model)

            foreach (var input in model.inputs)
            {
-                if (input.shape.Length == 4)
+                if (input.name.StartsWith(TensorNames.VisualObservationPlaceholderPrefix))


Doesn't have to be in this PR, but you might want to consider moving GetNumVisualInputs(), GetOutputNames(), etc to the extension methods too.

chriselion · 2020-11-05T02:03:34Z

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs

@@ -152,21 +149,13 @@ public static string[] GetOutputNames(Model model)
                return failedModelChecks;
            }

-            foreach (var constantName in TensorNames.RequiredConstants)
+            var modelApiVersionTensor = model.GetTensorByName(TensorNames.VersionNumber);
+            if (modelApiVersionTensor == null)


Again, doesn't need to be in this PR, but I feel like we lost some of the checks that the tensors we need are in the Model. It's never an issue for model files that we create, but a frequent source of forum/github posts would be people trying to train their own models, then add them for inference and get null reference errors because the assumed the tensors were there without checking.

I need to look a little more closely, but I think things like HasContinuousOutputs() would throw if not of the expected tensors are there.

We could add something like

bool CheckExpectedTensors(List<string> errorMessagesOut) {}

to the extension methods, call it once here, and then assume everything is present after that.

We should also have a test that CheckModel() with a basically empty model doesn't throw and returns some error strings.

Feel free to just log a jira as a reminder to come back to this, since I think it's a small regression in usability.

I think I took care of that by the order of the checks, but I agree we should check it explicitly for safety.
Made a JIRA ticket.

…echnologies/ml-agents into develop-hybrid-inference

dongruoping · 2020-11-05T19:59:37Z

Some tests are failing because since this PR is not self-contained, and it needs some changes on python. Leave those to python PR will check when merge.

* Add hybrid action capability flag (#4576) * Change BrainParametersProto to support ActionSpec (#4579) * Assign new BrainParametersProto fields based on capabilities (#4581) * ActionBuffer with hybrid actions for RemotePolicy (#4592) * Barracuda inference for hybrid actions (#4611) * Refactor BarracudaModel loader checks (#4629) * Export separate nodes for continuous/discrete actions (#4655) * Separate continuous/discrete actions in AgentActionProto (#4698) * Force different nodes for new and deprecated action output (#4705)

barracuda inference for hybrid actions

bdc3ef4

dongruoping requested a review from chriselion October 29, 2020 01:13

dongruoping commented Oct 29, 2020

View reviewed changes

dongruoping requested a review from vincentpierre October 29, 2020 01:16

chriselion reviewed Oct 29, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Inference/ApplierImpl.cs Outdated Show resolved Hide resolved

chriselion reviewed Oct 29, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Policies/BarracudaPolicy.cs Outdated Show resolved Hide resolved

chriselion reviewed Oct 29, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs Outdated Show resolved Hide resolved

chriselion reviewed Oct 29, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs Outdated Show resolved Hide resolved

chriselion reviewed Oct 29, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs Outdated Show resolved Hide resolved

chriselion reviewed Oct 29, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs Outdated Show resolved Hide resolved

chriselion reviewed Oct 29, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Inference/TensorNames.cs Outdated Show resolved Hide resolved

vincentpierre reviewed Oct 30, 2020

View reviewed changes

dongruoping added 5 commits November 2, 2020 15:16

Add barracuda model extension methods to handle deprecated action output

1a9ec75

add method summaries

303bb94

replace deprecated checks with extension method

fa7a4a8

fix tests

8bdece5

move deprecated fields

8a15f3b

vincentpierre self-requested a review November 3, 2020 18:11

vincentpierre reviewed Nov 3, 2020

View reviewed changes

vincentpierre approved these changes Nov 3, 2020

View reviewed changes

chriselion reviewed Nov 4, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs Show resolved Hide resolved

chriselion reviewed Nov 4, 2020

View reviewed changes

dongruoping added 2 commits November 3, 2020 21:59

fix bug

3761d64

Add new model and tests to ParameterLoaderTest

cd899a2

dongruoping marked this pull request as ready for review November 4, 2020 19:33

dongruoping added 3 commits November 4, 2020 12:07

add comments

1f78b1b

early out in tensorApplier if model is null

a16a2d0

remove unused

79a92ef

dongruoping requested a review from chriselion November 4, 2020 23:55

chriselion reviewed Nov 5, 2020

View reviewed changes

yamato triggers - run on non-master PRs

b4bbd27

chriselion approved these changes Nov 5, 2020

View reviewed changes

dongruoping added 2 commits November 5, 2020 09:30

fix tests

5de688b

Merge branch 'develop-hybrid-inference' of https://github.com/Unity-T…

99d4247

…echnologies/ml-agents into develop-hybrid-inference

dongruoping changed the title ~~[WIP] Barracuda inference for hybrid actions~~ Barracuda inference for hybrid actions Nov 5, 2020

dongruoping merged commit fbbff02 into develop-hybrid-actions-csharp Nov 5, 2020

delete-merged-branch bot deleted the develop-hybrid-inference branch November 5, 2020 21:02

github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Barracuda inference for hybrid actions #4611

Barracuda inference for hybrid actions #4611

dongruoping commented Oct 29, 2020 •

edited

Loading

dongruoping Oct 29, 2020

chriselion Oct 29, 2020

dongruoping Oct 30, 2020

chriselion Oct 30, 2020

vincentpierre Oct 30, 2020

vincentpierre Oct 30, 2020

vincentpierre Oct 30, 2020

vincentpierre Oct 30, 2020

vincentpierre Oct 30, 2020

dongruoping Nov 3, 2020

vincentpierre Oct 30, 2020

dongruoping Nov 3, 2020

vincentpierre Oct 30, 2020

vincentpierre Oct 30, 2020

vincentpierre Nov 3, 2020

vincentpierre Nov 3, 2020

dongruoping Nov 4, 2020

chriselion Nov 4, 2020

dongruoping Nov 4, 2020

chriselion Nov 4, 2020

dongruoping Nov 4, 2020

chriselion Nov 5, 2020 •

edited

Loading

chriselion Nov 5, 2020

dongruoping Nov 5, 2020

dongruoping commented Nov 5, 2020

		@@ -157,7 +157,7 @@ public void TestGetOutputTensors1()
		{
		var model = ModelLoader.Load(continuous2vis8vec2actionModel);

Barracuda inference for hybrid actions #4611

Barracuda inference for hybrid actions #4611

Conversation

dongruoping commented Oct 29, 2020 • edited Loading

Proposed change(s)

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

Types of change(s)

Checklist

Other comments

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriselion Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongruoping commented Nov 5, 2020

dongruoping commented Oct 29, 2020 •

edited

Loading

chriselion Nov 5, 2020 •

edited

Loading