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

Side Channel Design Changes #3807

Merged
merged 24 commits into from
Apr 23, 2020
Merged

Side Channel Design Changes #3807

merged 24 commits into from
Apr 23, 2020

Conversation

mmattar
Copy link

@mmattar mmattar commented Apr 19, 2020 •
edited
Loading

Proposed change(s)

See https://docs.google.com/document/d/1a0Ua_P1D8VUz8r_YMtiBa1swFISshJaaXslTncl_lmo/edit

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

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

Marwan Mattar added 9 commits April 19, 2020 08:58
Make EnvironmentParameters a first-class citizen in the API
963a60d 
Missing: Python conterparts and testing.
Minor comment fix to Engine Parameters
ce1525e
A second minor fix.
62c868b
Make EngineConfigChannel Internal and add a singleton/sealed accessor
53f9850
Make StatsSideChannel Internal and add a singleton/sealed accessor
fc28824
Changes to SideChannelUtils
12e88f9 
- Disallow two sidechannels of the same type to be added
- Remove GetSideChannels that return a list as that is now unnecessary
- Make most methods except (register/unregister) internal to limit users impacting the “system-level” side channels
- Add an improved comment to SideChannel.cs
Added Dispose methods to system-level sidechannel wrappers
cbe21ad 
- Specifically to StatsRecorder, EnvironmentParameters and EngineParameters.
- Updated Academy.Dispose to take advantage of these.
- Updated Editor tests to cover all three “system-level” side channels.

Kudos to Unit Tests (TestAcademy / TestAcademyDispose) for catching these.
Removed debub log.
194cea4
Back-up commit.
f81e835
Marwan Mattar added 8 commits April 21, 2020 18:30
mmattar
mmattar commented Apr 22, 2020
View reviewed changes
Copy link
Author

@mmattar mmattar left a comment •
edited by vincentpierre
Loading

Choose a reason for hiding this comment

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

@vincentpierre - the branch is ready for you to take over and make the Python changes. This should include:

  • Updating the engine config side channel to include capture framerate as the last field. This will require adding a new CLI option that has a default value of 60.
  • Creating EnvironmentParameters side channel that is uni-directional Python to Unity with the message format of: string, int, block. The int represents an enum value that is equivalent to the EnvironmentDataTypes enum in EnvironmentParametersChannel.cs
  • Switch from using floatproperties to the new environment parameters side channel

com.unity.ml-agents/Runtime/SideChannels/EnvironmentParametersChannel.cs Show resolved Hide resolved
chriselion
chriselion reviewed Apr 22, 2020
View reviewed changes
com.unity.ml-agents/Runtime/SideChannels/EnvironmentParametersChannel.cs Outdated
/// <param name="key">Parameter key.</param>
/// <param name="defaultValue">Default value to return.</param>
/// <returns></returns>
public float GetParameterWithDefault(string key, float defaultValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Maybe "GetWithDefault" or "GetValueWithDefault" or (if for some reason you want to be like Java.HashMap) "GetOrDefault"?

Copy link
Author

Choose a reason for hiding this comment

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

I like this shorter notation. I don't want to be like Java, but GetOrDefault is clearer, no?

But, if we make this change, should we also make these for consistency?

  • floatproperties: GetPropertyWithDefault --> GetWithDefault
  • floatproperties: SetProperty --> Set
  • rawbytes: GetAndClearReceivedMessages --> GetAndClear
  • rawbytes: GetReceivedMessages --> Get
  • rawbytes: SendRawBytes --> Send

Copy link
Contributor

Choose a reason for hiding this comment

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

GetPropertyWithDefault is the one I'd like to change the most, since I feel like "parameter"/"property" doesn't really add any value.

I'd rather leave GetAndClearReceivedMessages to keep it clear (pun intended) that it's a destructive action.

Copy link
Author

Choose a reason for hiding this comment

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

@vincentpierre - one more change if we think its worth it.

@chriselion
Copy link
Contributor

Don't forget to increase the communication protocol version!

@vincentpierre vincentpierre mentioned this pull request Apr 22, 2020
Merged
10 tasks
chriselion
chriselion reviewed Apr 22, 2020
View reviewed changes
com.unity.ml-agents/Runtime/SideChannels/EnvironmentParametersChannel.cs Outdated
}
else
{
throw new UnityAgentsException("EnvironmentParametersChannel only supports floats.");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should either silently ignore unhandled types here, or at most log a warning (no exception).

Copy link
Author

Choose a reason for hiding this comment

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

@vincentpierre

@vincentpierre
Python side changes to the SideChannel redesign (#3826)
ab9dc93 
* Modified the EngineConfig to send one message per field

* Created the Python Environment Parameters Channel and hooked it in

* Make OnMessageReceived protected

* addressing comments
@vincentpierre vincentpierre mentioned this pull request Apr 23, 2020
Merged
10 tasks
@mmattar
Copy link
Author

mmattar commented Apr 23, 2020

Don't forget to increase the communication protocol version!

@vincentpierre

@vincentpierre
[Side Channels] Edited the documenation and renamed a few things (#3833)
be42950 
* Edited the documetation and renamed a few things

* addressing comments

* Update docs/Python-API.md

Co-Authored-By: Chris Elion <chris.elion@unity3d.com>

* Update com.unity.ml-agents/CHANGELOG.md

Co-Authored-By: Chris Elion <chris.elion@unity3d.com>

* Removing unecessary migrating line

Co-authored-by: Chris Elion <chris.elion@unity3d.com>
@vincentpierre vincentpierre self-assigned this Apr 23, 2020
chriselion
chriselion reviewed Apr 23, 2020
View reviewed changes
com.unity.ml-agents/Runtime/EnvironmentParameters.cs Outdated
/// <param name="key">The parameter key</param>
/// <param name="defaultValue">Default value for this parameter.</param>
/// <returns></returns>
public float GetParameterWithDefault(string key, float defaultValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I might have commented on the wrong thing before. But this is the method I was suggesting we shorten to GetWithDefault()

chriselion
chriselion reviewed Apr 23, 2020
View reviewed changes
com.unity.ml-agents/Runtime/SideChannels/FloatPropertiesChannel.cs Outdated
/// Returns a list of all the string identifiers of the properties currently present.
/// </summary>
/// <returns> The list of string identifiers </returns>
public IList<string> List()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we just call this Keys()?

chriselion
chriselion reviewed Apr 23, 2020
View reviewed changes
com.unity.ml-agents/Runtime/EngineParameters.cs Outdated
/// that if the Engine settings are directly changed anywhere in your Project, then the values
/// returned here may not be reflective of the actual Engine settings.
/// </summary>
internal sealed class EngineParameters
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like this class has no purpose now, since it's internal. Should we just remove it, and move the EngineConfigurationChannel/RegisterSideChannel setup to Academy?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can do that yes. This is without change to the API so we can hold it off or I can do it now. Any preference ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then again, it's internal so we can remove it if we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I was going to say do it later, but looks like you already did.

Copy link
Author

@mmattar mmattar Apr 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The purpose of EngineParameters was just to contain the registering/unregistering of the side channel.

Copy link
Author

Choose a reason for hiding this comment

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

Fine to not have it, just wanted to clarify intentions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it had a purpose originally, but we removed so much that it wasn't worth keeping around just to handle registration.

@vincentpierre vincentpierre merged commit e4826b6 into master Apr 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-mm-env-params-unity branch April 23, 2020 23:47
@chriselion chriselion changed the title [WIP] Side Channel Design Changes Side Channel Design Changes Apr 28, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chriselion chriselion chriselion approved these changes

@vincentpierre vincentpierre vincentpierre left review comments

Assignees

@vincentpierre vincentpierre

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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