-
Notifications
You must be signed in to change notification settings - Fork 66
Internal: Cluster data helpers and upload_node_script into cluster_data module #401
Conversation
…internal/cluster-data
aztk/client.py
Outdated
@@ -27,6 +27,13 @@ def __init__(self, secrets_config: models.SecretsConfiguration): | |||
self.blob_client = azure_api.make_blob_client(secrets_config) | |||
|
|||
|
|||
|
|||
def get_cluster_data(self, cluster_id: str) -> cluster_data.ClusterData: |
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.
This is meant to be for internal use only, right? So maybe make the function _get_cluster_data
instead.
self.blob_client = blob_client | ||
|
||
|
||
def as_resource_file(self, dest: str = None) -> batch_models.ResourceFile: |
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 feel like this should be to_resource_file
.
self.blob_client.create_blob_from_path(self.cluster_id, blob_path, local_path) | ||
return BlobData(self.blob_client, self.cluster_id, blob_path) | ||
|
||
def upload_cluster_file(self, blob_path: str, local_path: str) -> BlobData: |
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.
Does this need to exist? Looks like it's only called by upload_node_data
below, they could probably be combined. Or are we anticipating that files other than node_scripts will be put in the cluster
dir?
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.
yeah was having this one in case there is any other files that would need to be uploaded. Feel like it doesn't hurt to have it here
file_utils.ensure_dir(self.zip_path) | ||
self.zipf = zipfile.ZipFile(self.zip_path, "w", zipfile.ZIP_DEFLATED) | ||
|
||
def __enter__(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.
This is nice, and we might want to add it a few other places, but it doesn't look like you used it at all for NodeData?
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.
yeah I actually didn't ended up using for node data so could remove it here but there is a few place where this could be useful to implement
helpers and upload_node_script was getting quite messy.
Created a new module
cluster_data
which goal is to manage all the cluster data(Blob container).