Use PeerID exclusively to address MoE experts #479
Conversation
Codecov Report
@@ Coverage Diff @@
## master #479 +/- ##
==========================================
- Coverage 83.02% 82.95% -0.08%
==========================================
Files 83 83
Lines 8177 8177
==========================================
- Hits 6789 6783 -6
- Misses 1388 1394 +6
|
hivemind/moe/client/beam_search.py
Outdated
| if not best_active_pairs: | ||
| break |
There was a problem hiding this comment.
If I understand everything right, then without this if we would also break the cycle, because eventually would get empty beam on L330 and reach break at L333. However on L332 we have log.warning, which telling us that this situation should get some attention.
TLDR : shouldn't we do log.warning here?
There was a problem hiding this comment.
Yes, I think that it should at least require an explanation via a comment/log entry
There was a problem hiding this comment.
good catch, this if is not necessary
gonna remove it
There was a problem hiding this comment.
Well, in fact this if gives some profit. We wont do a lot of calls just to find out that there is nothing to search for. As I understand, there will be 0 net communications in either way, be there will be some small time save.
Anyway, any change is acceptable: to add logging or to remove this if at all.
| experts[i] = RemoteExpertInfo(uid, PeerInfo.from_tuple(expert_info_for_uid.value)) | ||
| server_peer_id = found[uid] | ||
| if server_peer_id is not None and isinstance(server_peer_id.value, str): | ||
| experts[i] = ExpertInfo(uid, PeerID.from_base58(server_peer_id.value)) |
There was a problem hiding this comment.
This construction (involving PeerID.from_base58) occurs at least 3 times in the codebase, perhaps it's worth changing ExpertInfo into a dataclass with an additional classmethod from_binary for simplicity?
hivemind/moe/client/beam_search.py
Outdated
| and isinstance(match, ValueWithExpiration) | ||
| and isinstance(match.value, tuple) | ||
| and len(match.value) == 2 | ||
| and is_valid_uid(match.value[0]) | ||
| and isinstance(match.value[1], str) |
There was a problem hiding this comment.
Maybe extract this 5-line validation into a function?
hivemind/moe/client/beam_search.py
Outdated
| if not best_active_pairs: | ||
| break |
There was a problem hiding this comment.
Yes, I think that it should at least require an explanation via a comment/log entry
|
the SLOC balance is finally negative :) |
borzunov
changed the title
This PR changes declare_experts / RemoteExpert to use only p2p peer ID, not the whole multiaddress.
This slightly reduces the code complexity and gives you an easier time sharing experts with dynamic IP.
It also fixes one DHT edge case i've discovered when working on it.
Minor changes: