move PerformanceEMA to utils, TrainingAverager to optim, update utils #405
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
Member
justheuristic
commented
Nov 7, 2021
•
justheuristic
commented
- implement and test async wrapper for ContextManager (used in DecentralizedAverager and ProgressTracker)
- implement .reset_timer in PerformanceEMA (used when progress was reset, e.g. with fp16 gradient overflow, which should not affect samples per second)
- move PerformanceEMA to hivemind.utils (rationale: will be used in hivemind.moe in @mryab 's pipelining exps)
- move TrainingAverager to hivemind.optim (for compliance with hivemind.Optimizer and future deprecation in favour of Training StateAverager)
- fix process-wide RSA keys in the validator
justheuristic
commented
Nov 7, 2021
| async def enter_asynchronously(context: AbstractContextManager): | ||
| """Wrap a non-async context so that it can be entered asynchronously""" | ||
| async with _AsyncContextWrapper(context) as ret_value: | ||
| yield ret_value |
There was a problem hiding this comment.
note: we can't simply
try:
yield await loop.run_in_executor(context.__enter__(...))
finally:
context.__exit__(None, None, None)
b/c this option does not correctly propagate exceptions into the inner context manager
Codecov Report
@@ Coverage Diff @@
## master #405 +/- ##
==========================================
- Coverage 83.73% 83.50% -0.24%
==========================================
Files 73 73
Lines 6678 6687 +9
==========================================
- Hits 5592 5584 -8
- Misses 1086 1103 +17
|
mryab
reviewed
Nov 7, 2021
| @@ -453,7 +461,7 @@ async def _run_allreduce(self, group_info: GroupInfo, min_vector_size: int, **kw | |||
| None, load_balance_peers, self.total_size, download_bandwidths, min_vector_size | |||
| ) | |||
|
|
|||
| async with self.get_tensors_async() as local_tensors: | |||
| async with enter_asynchronously(self.get_tensors()) as local_tensors: | |||
There was a problem hiding this comment.
This no longer seems to acquire lock_averaged_tensors (which should really be averaged_tensors_lock BTW), is this intended?
There was a problem hiding this comment.
It does, inside the get_tensors
| @@ -37,15 +37,19 @@ def update(self, task_size: float, interval: Optional[float] = None) -> float: | |||
| self.samples_per_second = 1 / max(adjusted_seconds_per_sample, self.eps) | |||
| return self.samples_per_second | |||
|
|
|||
| def reset_timer(self): | |||
There was a problem hiding this comment.
Right now it appears this method has only one usage in the same class. Is it going to have more usages outside of the class? If not, maybe you can have it as a private method or simply keep it inlined
| @@ -33,6 +33,9 @@ def event_loop(): | |||
| def cleanup_children(): | |||
| yield | |||
|
|
|||
| with RSAPrivateKey._process_wide_key_lock: | |||
| RSAPrivateKey._process_wide_key = None | |||
There was a problem hiding this comment.
Didn't see this change in the description and right now it seems to have no effect, do we need this?
There was a problem hiding this comment.
This has no effect, but it will if you run existing tests in different order.
TL;DR here is how it breaks things:
- you create something that instantiates RSAPrivateKey.process_wide
- and then all subsequent tests use the same key
- crucially, if you create two DHT instances with RSA validator AFTER instantiating private key, everything breaks because they both inherit your key
Future PRs introduce tests that instantiate validators before the sensitive tests and break everything in bizarre ways.
mryab
approved these changes
Nov 7, 2021
justheuristic
changed the title
justheuristic
changed the title
justheuristic
changed the title
justheuristic
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.