Feature: SDK refactor #622

jafreck · 2018-07-02T20:42:11Z

Docstrings for ALL public facing fucntions
Update sdk_example.py

timotheeguerin · 2018-07-09T22:40:37Z

aztk/client/base/helpers/create_user_on_node.py

+def __create_user(self, pool_id: str, node_id: str, username: str, password: str = None, ssh_key: str = None) -> str:
+    """
+        Create a pool user
+        :param pool: the pool to add the user to


Wrong format of args

these are the old deprecated methods, I'm going to leave the docstrings as is (since they will be removed soon).

All of the new user-facing functions have docstrings in the proper format.

timotheeguerin · 2018-07-09T22:48:07Z

aztk/client/base/helpers/get_application_log.py

+from aztk import models
+from aztk.utils import constants, helpers
+
+output_file = constants.TASK_WORKING_DIR + \


I'm actually thinking we shouldn't do path.join since this path is a Linux style path (evaluated on the node, not the client) regardless of what client runs it. So putting "/" explicitly seems better.

How about then we create a utility for that. I think this is root to many errors in duplicating /

yeah, sure. created a separate issue for that: #630

timotheeguerin · 2018-07-17T17:01:04Z

.vscode/settings.json

@@ -14,5 +14,6 @@
  "python.formatting.provider": "yapf",
  "python.venvPath": "${workspaceFolder}/.venv/",
  "python.pythonPath": "${workspaceFolder}/.venv/Scripts/python.exe",
-  "python.unitTest.pyTestEnabled": true
+  "python.unitTest.pyTestEnabled": true,
+  // "editor.formatOnSave": true,


I can delete this for now. This will autoformat on save, so we only commit formatted code. I didn't enable it here since there were already so many changes.

timotheeguerin · 2018-07-17T17:01:19Z

aztk/client/base/base_operations.py

+from aztk.internal import cluster_data
+from aztk.utils import ssh as ssh_lib
+
+from .helpers import (create_user_on_cluster, create_user_on_node, delete_user_on_cluster, delete_user_on_node,


separate on each line?

This is what VSCode's "sort imports" spits out. Unfortunately, yapf doesn't do import formatting (or comment/docstring formatting).

I think we should align to some standard import formatter, but not sure the vscode python extension one is the right tool.

timotheeguerin · 2018-07-17T17:01:49Z

aztk/client/base/base_operations.py

+            internal (:obj:`bool`, optional): if True, this will connect to the node using its internal IP.
+                Only use this if running within the same VNET as the cluster. Defaults to False.
+        Returns:
+            ClusterConfiguration: Object representing the cluster's configuration


shouldn't that also be a type?

what do you mean?

:objClusterConfiguration``

these actually weren't even being published, but I changed that so now they are fixed and are being published

timotheeguerin · 2018-07-17T17:04:06Z

aztk/spark/client/base/helpers/generate_application_task.py

+
+
+def generate_application_task(spark_client, container_id, application, remote=False):
+    resource_files = []


can you split this method in multiples smaller ones

timotheeguerin · 2018-07-17T17:05:47Z

aztk/spark/client/cluster/helpers/create.py

+
+        software_metadata_key = "spark"
+
+        vm_image = models.VmImage(publisher='Canonical', offer='UbuntuServer', sku='16.04')


should we save this as a constant?

timotheeguerin · 2018-07-17T17:06:33Z

aztk/spark/client/cluster/helpers/get.py

+
+def get_cluster(spark_cluster_operations, cluster_id: str):
+    try:
+        pool, nodes = super(type(spark_cluster_operations), spark_cluster_operations).get(cluster_id)


do you need to do that super(type thing?

If not it fell into infinite recursion, but maybe there is a better way.

spark_cluster_operations.get() would get an infinite loop?

timotheeguerin · 2018-07-17T17:06:43Z

aztk/spark/client/cluster/helpers/list.py

@@ -0,0 +1,14 @@
+import azure.batch.models.batch_error as batch_error
+
+import aztk.models    # TODO: get rid of this import and use aztk.spark.models


timotheeguerin · 2018-07-17T17:07:56Z

aztk/spark/client/job/helpers/submit.py

+def __app_cmd():
+    docker_exec = CommandBuilder("sudo docker exec")
+    docker_exec.add_argument("-i")
+    docker_exec.add_option("-e", "AZ_BATCH_TASK_WORKING_DIR=$AZ_BATCH_TASK_WORKING_DIR")


i think you can just do -e MY_ENV and docker will do the same

Yeah -- this code has just been reshuffled, not updated. That is an easy refactor though.

Actually, it looks like changing the -e env=$env to -e env does not work here. Possibly that syntax only works for the docker run, not docker exec. Not sure, but for now I'll just leave it as it works as is.

timotheeguerin · 2018-07-17T17:09:02Z

docs/aztk.models.rst

@@ -6,3 +6,4 @@ aztk.models package
    :members:
    :show-inheritance:
    :imported-members:
+    :undoc-members:


doesn't this adds too much noise?

timotheeguerin · 2018-07-17T17:10:00Z

aztk/client/base/helpers/get_application_log.py

+        task = batch_client.task.get(cluster_id, application_name)
+
+        if task.state is batch_models.TaskState.active or task.state is batch_models.TaskState.preparing:
+            # TODO: log


This is just reshuffled code I believe. We don't currently have a logger in the SDK (we should add one though) so this is outside the scope for this PR I think.

timotheeguerin · 2018-07-17T17:10:24Z

aztk/client/base/helpers/delete_user_on_node.py

@@ -0,0 +1,9 @@
+def delete_user(self, pool_id: str, node_id: str, username: str) -> str:
+    """


doesn't looks like to be too much of those left. Could change them all now to the new format

This docstring is not published. I can update it, but it will only be discoverable in the source.

timotheeguerin · 2018-07-30T21:32:15Z

aztk/spark/client/cluster/helpers/create.py

+        node_data = NodeData(cluster_conf).add_core().done()
+        zip_resource_files = cluster_data.upload_node_data(node_data).to_resource_file()
+
+        start_task = spark_cluster_operations._generate_cluster_start_task(core_cluster_operations, zip_resource_files, cluster_conf.cluster_id,


hhm shouldn't you not be calling spark_cluster_operations._generate_cluster_start_task

timotheeguerin · 2018-07-31T19:45:50Z

aztk/spark/client/client.py

+        return self.cluster.submit(id=cluster_id, application=application, remote=remote, wait=wait)
+
+    @deprecated("0.10.0")
+    def submit_all_applications(self, cluster_id: str, applications):    # NOT IMPLEMENTED


NOT IMPLEMENTED?

This is just a reminder to us that this function doesn't exist in the new API (i.e. I didn't just forget to do it, it's intentionally not there).

timotheeguerin · 2018-07-31T19:46:08Z

aztk/spark/client/cluster/helpers/list.py

+
+def list_clusters(core_cluster_operations):
+    try:
+        software_metadata_key = "spark"


isn't that a constant?

yea -- looks like a couple places that needed to change to models.Software.spark

jafreck added 13 commits June 22, 2018 17:48

start refactor

a88f087

continue refactor for cluster and job functions

35da215

fix imports

0da3f73

fixes

1a821b7

fixes

5f1542d

refactor integration test secrets management

f1faed9

fix cluster create, add new test

202591d

add tests for new sdk api and fix bugs

4fa150e

fix naming and bugs

a47ab07

update job operations naming, bug fixes

457adfa

fix cluster tests

880e66d

fix joboperations and tests

1d07be9

update cli and fix some bugs

8c3b289

jafreck added the work in progress label Jul 2, 2018

jafreck added 2 commits July 3, 2018 15:42

start fixes

bd16653

fix pylint errors, bugs

adb0e0a

jafreck removed the work in progress label Jul 9, 2018

add deprecated warning checks, rename tests

6830614

timotheeguerin reviewed Jul 9, 2018

View reviewed changes

jafreck added 11 commits July 10, 2018 11:00

add docstrings for baseoperations

52d27f9

add docstrings

581f9c8

docstrings, add back compat for coreclient, fix init for spark client

b159c61

whitespace

c6da487

docstrings, whitespace

4aa8774

docstrings, fixes

8cf4667

docstrings, fixes

b4c13e1

fix the sdk documentation, bugs

5769e71

fix method call

2342326

pool_id->id

b3af9ec

rename ids

9b18ac5

jafreck added 7 commits July 11, 2018 17:53

add todo

15111aa

fixes

e585751

add some todos

dd2b6ac

rename pool to cluster, add todo for nodes params

a737237

add todos for nodes param removal

7042be9

update functions names

32c9d2c

remove deprecated fucntion calls

cfdf132

timotheeguerin reviewed Jul 17, 2018

View reviewed changes

jafreck added 5 commits July 17, 2018 13:44

update docs and docstrings

d48f7e5

update docstrings

c2de9b3

get rid of TODOs, fix docstrings

d6760c1

remove unused setting

a418a37

inheritance -> composition

4749017

jafreck mentioned this pull request Jul 25, 2018

update aztk/examples/sdk/sdk_example.py #627

Closed

jafreck added 6 commits July 30, 2018 10:35

fix models bugs

2d0700c

fix create_user bug

be31eac

update sdk_example.py

f179b0d

fix create user argument issue

19865af

update sdk_example.py

96f46ee

update doc

0a734a5

timotheeguerin reviewed Jul 30, 2018

View reviewed changes

timotheeguerin approved these changes Jul 31, 2018

View reviewed changes

jafreck added 5 commits August 1, 2018 17:32

use Software model instead of string

159566a

add job wait flag, add cluster application wait functions

42fed0f

add docs for wait, update tests

ecfda50

fix bug

a27e371

add clientrequesterror catch to fix tests

6c6767c

jafreck merged commit b18eb69 into Azure:master Aug 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: SDK refactor #622

Feature: SDK refactor #622

jafreck commented Jul 2, 2018 •

edited

Loading

timotheeguerin Jul 9, 2018

jafreck Jul 10, 2018

timotheeguerin Jul 9, 2018

jafreck Jul 30, 2018

timotheeguerin Jul 30, 2018

jafreck Jul 30, 2018

timotheeguerin Jul 17, 2018

jafreck Jul 17, 2018

timotheeguerin Jul 17, 2018

jafreck Jul 17, 2018

timotheeguerin Jul 17, 2018

jafreck Jul 17, 2018

timotheeguerin Jul 17, 2018

jafreck Jul 17, 2018

timotheeguerin Jul 17, 2018

timotheeguerin Jul 17, 2018

timotheeguerin Jul 17, 2018

jafreck Jul 17, 2018

timotheeguerin Jul 17, 2018

timotheeguerin Jul 17, 2018

timotheeguerin Jul 17, 2018

jafreck Jul 17, 2018

jafreck Jul 30, 2018

timotheeguerin Jul 17, 2018

timotheeguerin Jul 17, 2018

jafreck Jul 18, 2018

timotheeguerin Jul 17, 2018

jafreck Jul 17, 2018

timotheeguerin Jul 30, 2018

timotheeguerin Jul 31, 2018

jafreck Jul 31, 2018

timotheeguerin Jul 31, 2018

jafreck Jul 31, 2018



		def generate_application_task(spark_client, container_id, application, remote=False):
		resource_files = []


		software_metadata_key = "spark"

		vm_image = models.VmImage(publisher='Canonical', offer='UbuntuServer', sku='16.04')

		@@ -0,0 +1,14 @@
		import azure.batch.models.batch_error as batch_error

		import aztk.models # TODO: get rid of this import and use aztk.spark.models

		@@ -0,0 +1,9 @@
		def delete_user(self, pool_id: str, node_id: str, username: str) -> str:
		"""

Feature: SDK refactor #622

Feature: SDK refactor #622

Conversation

jafreck commented Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NOT IMPLEMENTED?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafreck commented Jul 2, 2018 •

edited

Loading