Dev/minimal flow #16
Dev/minimal flow #16
Conversation
| """ | ||
| verify_type("api_token", api_token, str) | ||
| verify_type("family", family, str) | ||
| verify_type("run_id", run_id, str) | ||
| verify_type("max_queue_size", max_queue_size, int) | ||
| verify_type("max_queue_size_exceeded_callback", max_queue_size_exceeded_callback, (Callable, type(None))) | ||
|
|
There was a problem hiding this comment.
we could check for > 1 here or add verify_int() with eg. positive=True argument
| @@ -51,6 +51,7 @@ def __init__( | |||
|
|
|||
| def enqueue(self, *, operation: RunOperation) -> None: | |||
| try: | |||
| # TODO: This lock could be moved to the Run class | |||
| with self._lock: | |||
There was a problem hiding this comment.
Or just have OpeartionsQueue own the lock? It doesn't seem Run is using it, apart from passing to the OperationsQueue constructor.
There was a problem hiding this comment.
In the following PRs we're making sure that no one is putting any new operations once we're operating on SyncProcess so this cannot be a part of OperationsQueue
| verify_type("as_experiment", as_experiment, (str, type(None))) | ||
| verify_type("creation_time", creation_time, (datetime, type(None))) | ||
| verify_type("from_run_id", from_run_id, (str, type(None))) | ||
| verify_type("from_step", from_step, (int, float, type(None))) |
There was a problem hiding this comment.
Shouldn't we make from_step obligatory when from_run_id is provided, and the other way around? Otherwise _create_run will silently ignore forking
There was a problem hiding this comment.
Isin't it covered by line 128?
if (from_run_id is not None and from_step is None) or (from_run_id is None and from_step is not None):
raise ValueError("`from_run_id` and `from_step` must be used together.")| new_size = size + pb_key_size(key) + proto_value.ByteSize() + 6 | ||
|
|
There was a problem hiding this comment.
Where does the +6 come from?
There was a problem hiding this comment.
It was based on our internal previous script, an overhead for type and length definitions I think.
| @@ -33,3 +62,8 @@ def make_step(number: float | int, raise_on_step_precision_loss: bool = False) - | |||
| micro = micro % m | |||
|
|
|||
| return Step(whole=whole, micro=micro) | |||
|
|
|||
|
|
|||
| def pb_key_size(key: str) -> int: | |||
There was a problem hiding this comment.
A short explanation of how this is calculated and why would be great
There was a problem hiding this comment.
It was from our previous script but I think it comes from max length assumption (10k if I remember, so 2 bytes at most for varint representation) + type definition overhead.
tests/unit/test_metadata_splitter.py
Outdated
| result = list(builder) | ||
|
|
||
| # then | ||
| assert len(result) > 0 |
There was a problem hiding this comment.
Detail: assert len(result) > 1 would make sure we're actually generating multiple results for entries that are too large. There's another one below as well
No description provided.