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.

Sign up for GitHub

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

Jump to bottom

V2 staging new model version #5080

Merged
merged 24 commits into from
Mar 15, 2021
Merged

V2 staging new model version #5080

merged 24 commits into from
Mar 15, 2021

Conversation

vincentpierre
Copy link
Contributor

@vincentpierre vincentpierre commented Mar 11, 2021 •
edited
Loading

Proposed change(s)

BREAKING: Models trained with the latest Python will no longer work with pre 2.0 Unity package.

  • Remove distinction between visual and vector observations in the onnx model.
  • Allow for running legacy models (models trained with old python will work in 2.0 Unity package).
  • Multinomial will now run in Barracuda (when using a 2.0 python model with 2.0 Unity package)

Need some changes to the changelog

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

Slack messages
JIRA : 1319 and 1626

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 vincentpierre self-assigned this Mar 11, 2021
chriselion
chriselion reviewed Mar 11, 2021
View reviewed changes
com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs
/// <param name="sensors">Attached sensors</param>
/// <param name="observableAttributeTotalSize">Sum of the sizes of all ObservableAttributes.</param>
/// <returns>The list the error messages of the checks that failed</returns>
static IEnumerable<FailedCheck> CheckInputTensorShape(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reuse any of the code between this and CheckInputTensorShapeLegacy? I haven't done a side-by-side diff, but looks like it might be cleaner to make legacy an argument instead.

Same comments apply to CheckInputTensorPresence/CheckInputTensorPresenceLegacy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking reusing code, but I think I still prefer having separate methods (even if it duplicates a lot of the code). My thinking is that is will make it easier to deprecate or isolate this code later and make it easier to test as well.

chriselion
chriselion reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@chriselion chriselion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No major objections, but I think it needs some cleanup.

vincentpierre
vincentpierre commented Mar 11, 2021
View reviewed changes
com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs Outdated
{
if (!tensorTester.ContainsKey(tensor.name))
{
failedModelChecks.Add(FailedCheck.Warning("Model requires an unknown input named : " + tensor.name
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The presence of legacy tensors names will be checked here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the warning message makes sense then. Should it be something like $"Model contains an unexpected input named: {tensor.name}"

@vincentpierre vincentpierre marked this pull request as ready for review March 11, 2021 20:19
dongruoping
dongruoping reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@dongruoping dongruoping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing would be nice to do in PR is to export an array of branch sizes instead of a sum for discrete action sizes, and also update the model loader checks.

@vincentpierre
Copy link
Contributor Author

@dongruoping Please review this PR that will do what you suggested : #5092

chriselion
chriselion reviewed Mar 12, 2021
View reviewed changes
com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs
}
else
{
int result = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a behavior change for legacy models, right? Is it correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not. (Unless I messed it up)
Because discreteOutputShape is currently a tensor of shape 1 ([sum_of_branches]) and will be changed to be ([branh0_size, branch1_size, ...]). So doing the sum does the same thing wether it is legacy or new.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks. Could you leave that as a comment here for the future readers?

chriselion
chriselion reviewed Mar 12, 2021
View reviewed changes
com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs
@@ -58,47 +89,68 @@ internal class BarracudaModelParamLoader
return failedModelChecks;
}

var modelApiVersion = (int)model.GetTensorByName(TensorNames.VersionNumber)[0];
var modelApiVersion = model.GetVersion();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this actually return -1? Doesn't look like it from https://github.com/Unity-Technologies/ml-agents/pull/5080/files#diff-bf1591d220fea33b1512200891dc1d8000f844664308187d2beaaaff8feb657aR49-R52 (unless that's the actual value).
Seems like this could be combined with the one below.

chriselion
chriselion reviewed Mar 12, 2021
View reviewed changes
com.unity.ml-agents/Runtime/Inference/BarracudaModelParamLoader.cs Outdated
return failedModelChecks;
}

var modelVersion = model.GetVersion();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you already have modelApiVersion above.

chriselion
chriselion reviewed Mar 12, 2021
View reviewed changes
com.unity.ml-agents/Tests/Editor/EditModeTestInternalBrainTensorGenerator.cs Outdated
@@ -63,8 +63,7 @@ static List<TestAgent> GetFakeAgents(ObservableAttributeOptions observableAttrib
return agents;
}

[Test]
public void Construction()
public void Construction(int apiVersion)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here? It will no longer be called as a test, and doesn't look like it's called anywhere else (at least not in this file).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, this is unit test was not being used properly, I modified t back so it will run (There was another test in a similar situation that I also changed)

chriselion
chriselion approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@chriselion chriselion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small extra feedback but looks good.

@vincentpierre vincentpierre merged commit 8f2f73f into v2-staging Mar 15, 2021
@delete-merged-branch delete-merged-branch bot deleted the v2-staging-new-model-version branch March 15, 2021 19:28
surfnerd pushed a commit that referenced this pull request Mar 18, 2021
@vincentpierre @surfnerd
V2 staging new model version (#5080)
73658cd 
* Make modelCheck have flavors of error messages

* ONNX exporter v3

* Using a better CheckType and a switch statement

* Removing unused message

* More tests

* Use an enum for valid versions and use GetVersion on model directly

* Maybe the model export version a static constant in Python

* Use static constructor for FailedCheck

* Use static constructor for FailedCheck

* Modifying the docstrings

* renaming LegacyDiscreteActionOutputApplier

* removing testing code

* better warning message

* Nest the CheckTypeEnum into the FailedCheck class

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

* Adding a line explaining that legacy tensor checks are for versions 1.X only

* Modifying the changelog

* Exporting all the branches size instead of omly the sum (#5092)

* addressing comments

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

Co-authored-by: Chris Elion <chris.elion@unity3d.com>

* readding tests

* Adding a comment around the new DiscreteOutputSize method

* Clearer warning : Model contains unexpected input > Model requires unknown input

* Fixing a bug in the case where the discrete action tensor does not exist

Co-authored-by: Chris Elion <chris.elion@unity3d.com>
surfnerd pushed a commit that referenced this pull request Mar 18, 2021
@vincentpierre @surfnerd
V2 staging new model version (#5080)
f508b96 
* Make modelCheck have flavors of error messages

* ONNX exporter v3

* Using a better CheckType and a switch statement

* Removing unused message

* More tests

* Use an enum for valid versions and use GetVersion on model directly

* Maybe the model export version a static constant in Python

* Use static constructor for FailedCheck

* Use static constructor for FailedCheck

* Modifying the docstrings

* renaming LegacyDiscreteActionOutputApplier

* removing testing code

* better warning message

* Nest the CheckTypeEnum into the FailedCheck class

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

* Adding a line explaining that legacy tensor checks are for versions 1.X only

* Modifying the changelog

* Exporting all the branches size instead of omly the sum (#5092)

* addressing comments

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

Co-authored-by: Chris Elion <chris.elion@unity3d.com>

* readding tests

* Adding a comment around the new DiscreteOutputSize method

* Clearer warning : Model contains unexpected input > Model requires unknown input

* Fixing a bug in the case where the discrete action tensor does not exist

Co-authored-by: Chris Elion <chris.elion@unity3d.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chriselion chriselion chriselion approved these changes

@dongruoping dongruoping dongruoping approved these changes

Assignees

@vincentpierre vincentpierre

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vincentpierre @chriselion @dongruoping