Conversation
| logger.debug(f"let's select {extra} ends for round {round}") | ||
| # round = channel_props["round"] if "round" in channel_props else 0 | ||
| if not agg_version_state or agg_version_state[0] is not None: | ||
| model_version = agg_version_state[0] |
There was a problem hiding this comment.
add an else case as fallback?
There was a problem hiding this comment.
would that else have a logger warning or a logger error?
| # DG: Removed old check for first round This indicates the | ||
| # first round, where no end's utility has been measured; | ||
| # Then, perform random selection | ||
| if round == 0: |
There was a problem hiding this comment.
we can use model_version here and it should work just fine too
if model version is available here
There was a problem hiding this comment.
why are we checking against exploitation_len? I think the previous comment of model_version or round being 0, implying that stat_utility values haven't been received, and thus resulting in a random selection makes sense. But what about this current condition? I think it could lead to other scenarios where it could do a random selection?
There was a problem hiding this comment.
Readability wise, the logic seems a bit off or difficult to understand. In the new code, we are first checking exploration/ exploitation. if that criteria doesnt match, then we are doing random selection? And right now it is not in sync with the old comments. It is okay to stick to this new logic if it is correct and makes sense- but the comments should be updated too
dhruvsgarg
left a comment
dhruvsgarg
left a comment
There was a problem hiding this comment.
Please address the comments
| end_last_selected_round = ends[end_id].get_property( | ||
| PROP_LAST_SELECTED_ROUND | ||
| ) | ||
| ) # Misnomer: This is actually PROP_LAST_SELECTED_MODEL_VERSION |
There was a problem hiding this comment.
Since consistently using model_version instead of round fixes it for both FL and FFL, can you add a TODO to replace LAST_SELECTED_ROUND with LAST_SELECTED_MODEL_VERSION?
| utility_list[utility_idx][PROP_UTILITY] = curr_end_utility | ||
|
|
||
| logger.debug( | ||
| f"{curr_end_utility}, {temporal_uncertainty}, {global_system_utility}, {utility_list[utility_idx][PROP_UTILITY]}, {utility_list[utility_idx][PROP_END_ID]}" |
There was a problem hiding this comment.
Clip value was missed from being logged (but it is expected based on the header at the top)
| logger.debug(f"let's select {extra} ends for round {round}") | ||
| # round = channel_props["round"] if "round" in channel_props else 0 | ||
| if not agg_version_state or agg_version_state[0] is not None: | ||
| model_version = agg_version_state[0] |
There was a problem hiding this comment.
would that else have a logger warning or a logger error?
| # diverged a lot in this time | ||
| and round - ends[end_id].get_property(PROP_LAST_EVAL_ROUND) >= 35 | ||
| and model_version - ends[end_id].get_property(PROP_LAST_EVAL_ROUND) | ||
| >= 35 |
There was a problem hiding this comment.
should check if this 35 limit is from OORT itself?
| # DG: Removed old check for first round This indicates the | ||
| # first round, where no end's utility has been measured; | ||
| # Then, perform random selection | ||
| if round == 0: |
There was a problem hiding this comment.
why are we checking against exploitation_len? I think the previous comment of model_version or round being 0, implying that stat_utility values haven't been received, and thus resulting in a random selection makes sense. But what about this current condition? I think it could lead to other scenarios where it could do a random selection?
| # DG: Removed old check for first round This indicates the | ||
| # first round, where no end's utility has been measured; | ||
| # Then, perform random selection | ||
| if round == 0: |
There was a problem hiding this comment.
Readability wise, the logic seems a bit off or difficult to understand. In the new code, we are first checking exploration/ exploitation. if that criteria doesnt match, then we are doing random selection? And right now it is not in sync with the old comments. It is okay to stick to this new logic if it is correct and makes sense- but the comments should be updated too
…Async (We just need to select 1 end)
…ack to random choose between exploration & exploitation in AsyncOORT when choosing just one end (It actually move completely to exploitation pretty quickly)
3 participants
No description provided.