Increase communicator version for concatenated PNGs. #4462
Conversation
chriselion
changed the title
| var trainerCanHandle = Academy.Instance.TrainerCapabilities == null || Academy.Instance.TrainerCapabilities.m_ConcatenatedPngObservations; | ||
| if (!trainerCanHandle) | ||
| { | ||
| throw new UnityAgentsException("Attached trainer doesn't support multiple PNGs. Upgrade to a version with communicator API >= 1.1.0"); |
There was a problem hiding this comment.
We could also just "downgrade" to uncompressed observations in this case (if we move the check earlier).
There was a problem hiding this comment.
Went with this instead.
| @@ -289,6 +289,17 @@ public static ObservationProto GetObservationProto(this ISensor sensor, Observat | |||
| } | |||
| else | |||
| { | |||
| // Check capabilities if we need to concatenate PNGs | |||
| if (sensor.GetCompressionType() == SensorCompressionType.PNG && shape.Length >= 3 && | |||
| shape[shape.Length - 1] > 3) | |||
There was a problem hiding this comment.
Are we assuming any shape >=3 will work? I am not sure Python will support. I would say shape.Length == 3
There was a problem hiding this comment.
We can't handle higher dimensional tensors in training, or in barracuda. Theoretically if you had your own trainer and weren't using inference, it could work. Then again, you wouldn't use PNG compression, so I'll simplify this.
| @@ -5,6 +5,7 @@ namespace Unity.MLAgents | |||
| internal class UnityRLCapabilities | |||
| { | |||
| internal bool m_BaseRLCapabilities; | |||
| internal bool m_ConcatenatedPngObservations; | |||
There was a problem hiding this comment.
I am not sure that internal variables need the m_ prefix.
There was a problem hiding this comment.
I'm not sure either. I can just make them public since it's an internal class.
There was a problem hiding this comment.
I think it is better yes.
| var trainerCanHandle = Academy.Instance.TrainerCapabilities == null || Academy.Instance.TrainerCapabilities.ConcatenatedPngObservations; | ||
| if (!trainerCanHandle) | ||
| { | ||
| Debug.LogWarning("Attached trainer doesn't support multiple PNGs. Switching to uncompressed observations."); |
There was a problem hiding this comment.
I fear this warning will be thrown a lot throughout the simulation. Should we set the compression type to None in the sensor?
There was a problem hiding this comment.
Good call. I'll make sure it only fires once.
Should we set the compression type to None in the sensor?
We don't currently have an interface for setting the compression type. I didn't want to make the sensor implementations check the compatibility, since they might forget.
delete-merged-branch
bot
deleted the
MLA-1348-concat-PNGs-communicator-version
branch
Proposed change(s)
Increase the communicator version to 1.1.0 to reflect the fact that concatenated PNG observations are not supported on older trainers.
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments