Skip to content

Release v0.7 disable gRPC on non supported platforms #1743

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.

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

Merged
merged 7 commits into from
Feb 21, 2019

Conversation

vincentpierre
Copy link
Contributor

This would be a quick fix for disabling training on unsupported platforms.
Might still need to document that if the user wants to recompile the protobuf objects, they will need to edit the generated files.

I tested it on UWP

@@ -2,6 +2,9 @@
// Generated by the protocol buffer compiler. DO NOT EDIT!
// source: mlagents/envs/communicator_objects/unity_to_external.proto
// </auto-generated>

# if UNITY_EDITOR || UNITY_STANDALONE_WINDOWSWINDOWS || UNITY_STANDALONE_OSX || UNITY_STANDALONE_LINUX
Copy link
Contributor

Choose a reason for hiding this comment

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

UNITY_STANDALONE_WINDOWSWINDOWS looks like a typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix

@Unity-Technologies Unity-Technologies deleted a comment Feb 21, 2019
@Unity-Technologies Unity-Technologies deleted a comment Feb 21, 2019
@@ -26,3 +26,14 @@ On Windows & Linux: [See here](https://github.com/google/protobuf/blob/master/sr
1. Install pre-requisites.
2. Un-comment line 4 in `make.bat`, and set to correct Grpc.Tools sub-directory.
3. Run `make.bat`
4. In the generated `UnityToExternalGrpc.cs` file in the `CommunicatorObjects` folder, you will need to add on top of the file the line
Copy link
Contributor

Choose a reason for hiding this comment

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

Say where CommunicatorObjects folder is (i.e. in UnitySDK).

Also change "to add on top of the file the line"
to
"to add the following to the beginning of the file"

```csharp
# if UNITY_EDITOR || UNITY_STANDALONE_WIN || UNITY_STANDALONE_OSX || UNITY_STANDALONE_LINUX
```
and at the bottom of the file the line
Copy link
Contributor

Choose a reason for hiding this comment

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

"and the following line to the end"

```csharp
#endif
```
This is to make sure the Grpc library is not used on platforms that do not support training.
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain why this is needed.

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 mean more than the current explanation ? What is missing in it ? I say It is to make sure the Grpc library is not used on platforms that do not support training

Copy link
Contributor

@awjuliani awjuliani left a comment

Choose a reason for hiding this comment

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

Added a few comments.

@Unity-Technologies Unity-Technologies deleted a comment Feb 21, 2019
Copy link
Contributor

@mantasp mantasp left a comment

Choose a reason for hiding this comment

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

looks good now

Copy link
Contributor

@awjuliani awjuliani left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@vincentpierre vincentpierre merged commit a4baab2 into release-v0.7 Feb 21, 2019
@vincentpierre vincentpierre deleted the release-v0.7-2dll branch February 21, 2019 23:47
LeSphax pushed a commit to LeSphax/ml-agents-1 that referenced this pull request May 3, 2020
…ies#1743)

* Fix for GRPC, need documentation

* Edits

* typo

* Fixes

* Missing typo

* Modified the documentation

* Updated the documentation
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants