Develop side channel #2956
Develop side channel #2956
Conversation
UnitySDK/Assets/ML-Agents/Scripts/SideChannel/RawBytesChannel.cs
Outdated
Show resolved
Hide resolved
* Added the side channel for the Engine Configuration. Note that this change does not require modifying a lot of files : - Adding a sender in Python - Adding a receiver in C# - subscribe the receiver to the communicator (here is a one liner in the Academy) - Add the side channel to the Python UnityEnvironment (not represented here) Adding the side channel to the environment would look like such : ```python from mlagents.envs.environment import UnityEnvironment from mlagents.envs.side_channel.raw_bytes_channel import RawBytesChannel from mlagents.envs.side_channel.engine_configuration_channel import EngineConfigurationChannel channel0 = RawBytesChannel() channel1 = EngineConfigurationChannel() env = UnityEnvironment(base_port = 5004, side_channels = [channel0, channel1]) ``` * renamings * addressing comments
UnitySDK/Assets/ML-Agents/Scripts/SideChannel/RawBytesChannel.cs
Outdated
Show resolved
Hide resolved
| var message = binaryReader.ReadBytes(messageLength); | ||
| if (sideChannels.ContainsKey(channelType)) | ||
| { | ||
| sideChannels[channelType].OnMessageReceived(message); |
There was a problem hiding this comment.
I still think a separate try/catch around here is a good idea; it will help distinguish between errors in the message stream (which we're handling below) and errors in the OnMessageReceived() implementations.
There was a problem hiding this comment.
I would not want to mask an error a user has in its own implementation of a side channel.
| + "compatible with the Python version." | ||
| ) | ||
| if channel_type in self.side_channels_dict: | ||
| self.side_channels_dict[channel_type].on_message_received(message_data) |
There was a problem hiding this comment.
Same comment as C# side, wrap this in a try/except.
There was a problem hiding this comment.
Same comment, I think we need to raise these errors as they are for the user that made its own side channels.
|
Overall looks good, could just use some python unit tests. |
|
This PR contains code additions for side channels, tests and code comments but does not have documentation and are not being used by training (the old system is still working) |
UnitySDK/Assets/ML-Agents/Scripts/SideChannel/FloatPropertiesChannel.cs
Outdated
Show resolved
Hide resolved
| QueueMessageToSend(SerializeMessage(key, defaultValue)); | ||
| m_FloatProperties[key] = defaultValue; |
There was a problem hiding this comment.
This would surprise me as a user - Get* shouldn't generally have a side effect. I think you should have a SetProperty that calls QueueMessageToSend and sets the Dict value
There was a problem hiding this comment.
(sorry if I missed that in the other PR)
| public enum SideChannelType | ||
| { | ||
| // Reserved for the FloatPropertiesChannel. | ||
| FloatProperties = 1, |
There was a problem hiding this comment.
Should we have a 0 value for "invalid"?
| if key in self._float_properties: | ||
| return self._float_properties[key] | ||
| else: | ||
| return None |
There was a problem hiding this comment.
You can just do self._float_properties.get(key) - that returns None if the key isn't in the dict.
|
Looks great. Definitely OK to merge this, and hook it in later. |
Co-Authored-By: Chris Elion <chris.elion@unity3d.com>
Co-Authored-By: Chris Elion <chris.elion@unity3d.com>
…ologies/ml-agents into develop-side-channel
This is the first draft for what is in this document
This PR contains code additions for side channels, tests and code comments but does not have documentation and are not being used by training (the old system is still working)
I would like to get this PR in and then make another with deprecation of the current way to do reset parameters and engine config and the documentation changes.