[API] Performance Logging #669
Conversation
scripts/requirements-gateway.txt
Outdated
| @@ -17,6 +17,7 @@ google-cloud-storage | |||
| cachetools | |||
| paramiko | |||
| rich | |||
| typer | |||
There was a problem hiding this comment.
Where does the gateway require typer?
There was a problem hiding this comment.
For setting the usage here
There was a problem hiding this comment.
Since this is only used in the client, use dependency injection like this:
skyplane/skyplane/api/impl/tracker.py
Lines 63 to 64 in 35cb10e
We can remove this dependency from the Docker image then
skyplane/api/impl/transfer_job.py
Outdated
| @@ -56,6 +56,41 @@ def estimate_cost(self): | |||
| # TODO | |||
| raise NotImplementedError | |||
|
|
|||
| def calculate_size(self): | |||
There was a problem hiding this comment.
We should cache this from the Chunker class and reuse it. list_objects is slow so we should only call it once. This may mean we only report usage after a transfer completes which is OK.
skyplane/api/impl/transfer_job.py
Outdated
| @@ -152,6 +187,7 @@ class CopyJob(TransferJob): | |||
| multipart_transfer_list: list = field(default_factory=list) | |||
|
|
|||
| def __post_init__(self): | |||
| self.type = "copy" | |||
There was a problem hiding this comment.
Add a "type" class attribute to the dataclass with a default value. That way, it will serialize cleanly.
skyplane/api/impl/transfer_job.py
Outdated
| @@ -229,6 +265,11 @@ def verify(self): | |||
|
|
|||
| @dataclass | |||
| class SyncJob(CopyJob): | |||
|
|
|||
| def __post_init__(self): | |||
| self.type = "sync" | |||
There was a problem hiding this comment.
Add a "type" class attribute to the dataclass with a default value. That way, it will serialize cleanly.
|
@parasj, any suggestions for the questions I mentioned at the beginning? |
Can we have a usage_enabled bit in TransferConfig? Not sure of a good design here.
Either is fine, whatever is easier. |
skyplane/api/dataplane.py
Outdated
| self.topology = topology | ||
| self.src_region_tag = self.topology.source_region() | ||
| self.dst_region_tag = self.topology.sink_region() | ||
| self.max_instances = int(len(self.topology.gateway_nodes) / 2) | ||
| regions = Counter([node.region.split(":")[1] for node in self.topology.gateway_nodes]) |
There was a problem hiding this comment.
In theory there shouldn't be collisions between clouds but maybe should we index it as ("aws", "us-east-1") instead?
skyplane/api/impl/tracker.py
Outdated
| if cr.chunk.chunk_id in self.job_complete_chunk_ids[job_uuid] | ||
| ] | ||
| ) | ||
| return sum(bytes_total_per_job.values()) / (2**30) |
There was a problem hiding this comment.
skyplane/api/usage/client.py
Outdated
| from rich import print as rprint | ||
| from typing import Optional, Dict | ||
|
|
||
| import skyplane | ||
| import skyplane.cli.usage.definitions | ||
| import skyplane.api.usage.definitions | ||
|
|
||
| # from skyplane.api.client import tmp_log_dir |
Remaining questions:
uuid.getnode()~/.skyplane/configfor API. But setting environment variable still works.