Skip to content

better logging for ports and versions #3048

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
Merged
merged 1 commit into from
Dec 10, 2019
Merged

better logging for ports and versions #3048

merged 1 commit into from
Dec 10, 2019

Conversation

chriselion
Copy link
Contributor

  • Clean up (IMHO) the port decision logic on C# side
  • Always print the version information on python
  • Print the port number when waiting for editor connection
  • If editor doesn't connect, print the port and API version.

@chriselion
Copy link
Contributor Author

One thing I didn't realize earlier is that we lost some of the exception information coming from here

ml-agents/UnitySDK/Assets/ML-Agents/Scripts/Grpc/RpcCommunicator.cs

Lines 92 to 103 in 22520fe

var exceptionMessage = "The Communicator was unable to connect. Please make sure the External " +
"process is ready to accept communication with Unity.";
// Check for common error condition and add details to the exception message.
var httpProxy = Environment.GetEnvironmentVariable("HTTP_PROXY");
var httpsProxy = Environment.GetEnvironmentVariable("HTTPS_PROXY");
if (httpProxy != null || httpsProxy != null)
{
exceptionMessage += " Try removing HTTP_PROXY and HTTPS_PROXY from the" +
"environment variables and try again.";
}
throw new UnityAgentsException(exceptionMessage);

because this is the expected path when doing inference. Not the end of the world though.

chriselion
chriselion commented Dec 7, 2019
View reviewed changes
UnitySDK/Assets/ML-Agents/Scripts/Academy.cs
@@ -217,6 +225,10 @@ void InitializeEnvironment()
}
catch
{
Debug.Log($"" +
$"Couldn't connect to trainer on port {port} using API version {k_ApiVersion}. " +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope this is OK, I want some information to make sure we're using the same port as python, but also want to make it clear that this is expected sometimes (when doing inference).

chriselion
chriselion commented Dec 7, 2019
View reviewed changes
UnitySDK/Assets/ML-Agents/Scripts/Academy.cs
{
Communicator = new RpcCommunicator(
new CommunicatorInitParameters
{
port = ReadArgs()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The try/catch here was really for the ReadArgs(); RpcCommunicator never throws.

chriselion
chriselion commented Dec 7, 2019
View reviewed changes
ml-agents/mlagents/trainers/learn.py
TensorFlow: {tf_utils.tf.__version__}
"""
return f""" Version information:
ml-agents: {mlagents.trainers.__version__},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just trimmed up the whitespace here.

chriselion
chriselion commented Dec 7, 2019
View reviewed changes
ml-agents/mlagents/trainers/learn.py
@@ -170,7 +169,7 @@ def parse_command_line(argv: Optional[List[str]] = None) -> CommandLineOptions:
"--cpu", default=False, action="store_true", help="Run with CPU only"
)

parser.add_argument("--version", action="version", version=get_version_string())
parser.add_argument("--version", action="version", version="")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

--version still works, but we always print the version anyway. I think it's worth keeping since that's a standard argument for a CLI.

@chriselion chriselion merged commit ff0edb4 into master Dec 10, 2019
@delete-merged-branch delete-merged-branch bot deleted the develop-debugging-help branch December 10, 2019 01:49
chriselion pushed a commit that referenced this pull request Dec 10, 2019
better logging for ports and versions (#3048)
839d24d
@chriselion chriselion mentioned this pull request Dec 10, 2019
Merged
chriselion pushed a commit that referenced this pull request Dec 10, 2019
better logging for ports and versions (#3048) (#3069)
557488c
chriselion pushed a commit that referenced this pull request Dec 12, 2019
Release 0.12.1 (#3078)
69c086e 
* cherry pick PR#3032 (#3066)

* better logging for ports and versions (#3048) (#3069)

* Release 0.12.1 doc fixes (#3070)

* add env.step() (#3068)

* bump version strings
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vincentpierre vincentpierre vincentpierre approved these changes

@surfnerd surfnerd Awaiting requested review from surfnerd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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