-
Notifications
You must be signed in to change notification settings - Fork 66
Conversation
@@ -16,5 +16,6 @@ | |||
"--style=.style.yapf" | |||
], | |||
"python.venvPath": "${workspaceFolder}/.venv/", | |||
"python.pythonPath": "${workspaceFolder}/.venv/Scripts/python.exe" | |||
"python.pythonPath": "${workspaceFolder}/.venv/Scripts/python.exe", | |||
"python.unitTest.pyTestEnabled": true |
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.
is true the production value?
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.
aztk/models/models.py
Outdated
self.storage_account_key = storage_account_key | ||
self.file_share_path = file_share_path | ||
self.mount_path = mount_path | ||
import azure.batch.models as batch_models | ||
|
||
class File: |
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.
why are some models left in this file? shouldn't this be in models/file_share.py
now?
|
||
for k, v in signature.parameters.items(): | ||
args[k] = PluginArgument(k, default=v.default, required=v.default is inspect.Parameter.empty) | ||
for k, param in signature.parameters.items(): |
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.
Not sure if this applies to this PR or if it needs another, but shouldn't this file exist in the aztk.spark.models
namespace since this has all of the spark
plugins specified?
if value is None: | ||
value = [] | ||
|
||
if self.merge_strategy == ListMergeStrategy.Append: |
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.
Shouldn't we have implementations for all defined types of ListMergeStrategy
?
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.
The other one is replace which is the default
def __init__(self, *args, **kwargs): | ||
if 'vm_count' in kwargs: | ||
deprecate("vm_count is deprecated for ClusterConfiguration please use size instead") | ||
kwargs['size'] = kwargs.pop('vm_count') |
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.
for cases like this, should we be handling for a KeyError
and returning something nicer if vm_count
doesn't exist, for example?
|
||
for k, v in signature.parameters.items(): | ||
args[k] = PluginArgument(k, default=v.default, required=v.default is inspect.Parameter.empty) | ||
for k, param in signature.parameters.items(): |
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.
Also, can we change k
to key
?
aztk/models/cluster_configuration.py
Outdated
cluster_id = fields.String() | ||
toolkit = fields.Model(Toolkit) | ||
size = fields.Integer(default=0) | ||
size_low_pri = fields.Integer(default=0) |
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.
what happens if size
is set to 0 and size_low_pri
is set to None
?
secrets.ssh_priv_key = default_config.get('ssh_priv_key') | ||
secrets.ssh_pub_key = default_config.get('ssh_pub_key') | ||
if 'default' in secrets_config: | ||
deprecate("default key in secrets.yaml is deprecated. Place all child parameters directly at the root") |
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 is a little confusing. Maybe we should point to docs on how to format secrets instead?
fix #162 (Unkown keys in config files will raise error)
Define a new Model base class and fields to be able to define models as below
This will handle most of the Validation, merging, parsing from dict automatically
Todo: