Skip to content
This repository has been archived by the owner on Feb 3, 2021. It is now read-only.

Internal: Cluster data helpers and upload_node_script into cluster_data module #401

Merged
merged 104 commits into from
Mar 8, 2018

Conversation

timotheeguerin
Copy link
Member

@timotheeguerin timotheeguerin commented Feb 13, 2018

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).

@jafreck jafreck added this to the v0.7.0 milestone Feb 26, 2018
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:
Copy link
Member

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:
Copy link
Member

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:
Copy link
Member

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?

Copy link
Member Author

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):
Copy link
Member

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?

Copy link
Member Author

@timotheeguerin timotheeguerin Mar 6, 2018

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

@timotheeguerin timotheeguerin merged commit 2bed496 into master Mar 8, 2018
@timotheeguerin timotheeguerin deleted the internal/cluster-data branch March 8, 2018 18:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants