-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Make the DecisionRequester public and customizable. #3716
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
surfnerd
requested review from
vincentpierre and
chriselion
and removed request for
vincentpierre
April 1, 2020 21:27
chriselion
reviewed
Apr 1, 2020
chriselion
reviewed
Apr 1, 2020
Looks good, just needs a test. |
surfnerd
changed the title
WIP: Make the DecisionRequester public and customizable.
Make the DecisionRequester public and customizable.
Apr 7, 2020
chriselion
reviewed
Apr 7, 2020
chriselion
approved these changes
Apr 7, 2020
surfnerd
force-pushed
the
master-decision-requester-api
branch
2 times, most recently
from
April 8, 2020 06:29
41fa8ae
to
b405630
Compare
… to allow users to customize it's behavior. - Rename Academy.AgentSetStatus to Academy.AgentPreStep and make it public. - Fix Unity library cache issues for backwards compatibility tests. - Collect standalone build and logs to artifacts for standalone build jobs. - cat standalone build log if the build fails. - Default verbose to False for standalone build test.
surfnerd
force-pushed
the
master-decision-requester-api
branch
from
April 8, 2020 18:43
f0df718
to
ddd206f
Compare
chriselion
approved these changes
Apr 8, 2020
vincentpierre
approved these changes
Apr 8, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change(s)
Make the DecisionRequest public and add a delegate to its API to allow for logic customization in calling
RequestDecision
and/orRequestAction
.I chose to use a delegate instead of a virtual method to make it easier for users to implement Agent subclasses. With the delegate, they can keep all of their Agent logic in one class, and easily hook into the EnvironmentStep loop in order to ensure that their calls to RequestAction or RequestDecision are handled in the appropriate step.
This pull request also changes how we handle logging our standalone build. We now collect the standalone build log as an artifact and will cat the log if the build fails or if the verbose flag was set to
true
.Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
MLA-764
Types of change(s)
- [ ] Bug fix- [ ] New feature- [ ] Breaking change- [ ] Documentation update- [ ] Other (please describe)Checklist
Other comments