-
Notifications
You must be signed in to change notification settings - Fork 488
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
Optimize RemoteSequenceManager #106
Conversation
@@ -0,0 +1 @@ | |||
"""Client-side functions responsible for choosing the best server, """ |
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.
"""Client-side functions responsible for choosing the best server, """ | |
"""Client-side functions responsible for choosing the best server.""" |
start=True, | ||
) | ||
|
||
def trigger_update(self): |
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.
Let's rename:
- (optional)
trigger_update()
->update()
(maybe withwait=True/False
) update_()
->_update()
(it's a private method, a user can't call it because it's not thread-safe)
logger.debug(f"{self.__class__.__name__} is shutting down") | ||
break | ||
|
||
if not self.trigger.is_set() and time.perf_counter() - self.last_update_time >= self.update_period: |
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.
if not self.trigger.is_set() and time.perf_counter() - self.last_update_time >= self.update_period: | |
if not self.trigger.is_set(): |
Simplify
finally: | ||
del update_manager | ||
|
||
logger.info(f"{self.__class__.__name__} thread exited") |
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.
logger.info(f"{self.__class__.__name__} thread exited") | |
logger.debug(f"{self.__class__.__name__} thread exited") |
(not useful to the end user)
def update_(self): | ||
"""Perform an immediate and synchronous refresh, may take time""" | ||
for attempt_no in itertools.count(): | ||
new_block_infos = petals.dht_utils.get_remote_module_infos( |
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.
Please wrap this in try-except, so retry happens in except
E.g., if the client has a network outage, get_remote_module_infos
will fail but we still want to retry
self.dht, self.block_uids, expiration_time=float("inf"), frozen=False | ||
) | ||
with self.lock_changes: | ||
self.sequence_info.update_(new_block_infos) |
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'm worried that it is not updated with OFFLINE
s
# Conflicts: # src/petals/client/remote_sequential.py # src/petals/client/sequence_manager.py # src/petals/client/sequential_autograd.py
Fix an issue in span selection that was introduced in #106
By the time this text is copied to commit message, this PR will have
traceback
4review: