-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Runtime selection of fp16/fp32 #1649
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
035fe32
Checkpointing
ihavnoid e9d1e7a
OpenCL half precision is now command-line option, compiled by default
ihavnoid 0122e42
Address code review comments for #1649
ihavnoid c2ab6a9
Update Network self-check
ihavnoid 23d2feb
Addressed code review results (2nd round)
ihavnoid 7735fd0
Merge branch 'next' of http://github.com/gcp/leela-zero into next
ihavnoid File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
How badly would the self-check need to be relaxed for HALF to pass?
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.
The reason I'm asking is that the comment for USE_HALF says "please test before enabling it".
And the user is going to wonder: test what? 😁
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.
I did a bit experimenting and ran the whole thing overnight (with self-checking everything) and I recall that eventually failed even with 100% margins. Most of the cases the error was less than 1% so it was okay.
I even moved the self-check to the final results (from the output of the forward() call) and it still did fail with 100% margins. It didn't seem to yield something too problematic (e.g., policy net moving a probability from 0.05 to 0.1) which isn't something that the tree search cannot fix, though.
I guess if we still want the USE_HALF self-check we have to do something like 'N cases with average error of XXX%' but I am not sure what is the right value/way to do this.
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.
The self-check has a catch that "rounds" all small values to zero (because the relative error gets very big on a very small value). Maybe it's just a matter of slightly pulling that up.
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.
I think Leela Zero Chess has a probabilistic self-check, in that it tolerates the occasional failure. Not sure how that combines with us already doing the self check once every xxx nodes though. At some point you're not going to catch buggy drivers either any more.
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.
Technically what we should test... is the strength of the engine (which one yields better win rate - speed vs. NN accuracy) and that can differ quite a bit depending on which GPU the user uses. The last time I tested, I figured out that high-bandwidth GPUs (e.g., Tesla P100 from Google Cloud) doesn't yield much additional performance hence we were sacrificing accuracy for nothing. In those cases --use-half would be worthless.
Maybe all I can say for now is to delete the comment? :)
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.
One option would be to use KL divergence or some other measure to calculate the error in the self check. KL divergence doesn't care if the low probabilities are little off unlike the current method.
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.
Okay, what I will do is: