pass sensor name through to ObservationSpec #5036
Conversation
There was a problem hiding this comment.
I must admit that I am not a fan of exposing the sensor name to python because we do not have a use case for it. It is also an invitation for the user to try to communicate information with the name of the sensor, which could be a problem.
But these are not good reasons, if you are confident about this, let's do it.
Is there maybe a way to test it end to end ?
|
I'm actually in support of this in the long run. In the future we can go as far as e.g. configuring different encoders for different sensors in the YAML. Also in demos we could use it to, for instance, record all the sensor but only train using some subset of them, and save people from re-recording demos over and over. |
|
@ervteng I don't think we should be picking encoders based on the string; we should add flags to explicitly control this (which is what we've been doing with e.g. dimension properties and ObservationType). This is intended to help with debugging, but I think it might make sense for the demonstration filtering too. @vincentpierre Please see the linked issue - this was a request from a user of the LLAPI to help them debug their sensors+observations.
I'll a check in the LLAPI test that the name is set to something non-empty |
| @@ -427,6 +427,11 @@ public static ObservationProto GetObservationProto(this ISensor sensor, Observat | |||
| } | |||
| } | |||
| observationProto.Shape.AddRange(shape); | |||
| var sensorName = sensor.GetName(); | |||
| if (string.IsNullOrEmpty(sensorName)) | |||
There was a problem hiding this comment.
This is why we needed an end-to-end test.
Proposed change(s)
Add a
namefield to the Observation proto, set this in C# when creating the proto, and pass it to the ObservationSpec on python.I'll also combine this into #5030 (depending on which finishes first) to add the sensor name in the error message.
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
#5011
Types of change(s)
Checklist