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

Feature: Models v2 #543

Merged
merged 53 commits into from
May 30, 2018
Merged

Feature: Models v2 #543

merged 53 commits into from
May 30, 2018

Conversation

timotheeguerin
Copy link
Member

@timotheeguerin timotheeguerin commented May 7, 2018

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

class UserState(Enum):
    Creating = "creating"
    Ready = "ready"
    Deleting = "deleting"

class UserInfo(Model):
    name = fields.String()
    age = fields.Integer()

class User(Model):
    info = fields.Model(UserInfo)
    enabled = fields.Boolean(default=True)
    state = fields.Enum(UserState, default=UserState.Ready)

# This is a valid way to create(from dict)
user = User(
        info=dict(
            name="Highlander",
            age=800,
        ),
        enabled=False,
        state="deleting",
    )

Todo:

  • Handle list merging(Allow append, replace options)
  • Handle nested object merging(Allow merge, replace options)
  • Handle list parsing and validation correctly
  • Apply on existing models using configuration base
  • update writing model docs

@@ -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

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?

Copy link
Member Author

@timotheeguerin timotheeguerin May 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting is to have option to run/debug the test one by one in vscode instead of using the cli, this is just for development
image

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

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

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

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?

Copy link
Member Author

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

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

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?

cluster_id = fields.String()
toolkit = fields.Model(Toolkit)
size = fields.Integer(default=0)
size_low_pri = fields.Integer(default=0)
Copy link
Member

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")
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 is a little confusing. Maybe we should point to docs on how to format secrets instead?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid keys in yaml files should result in an error instead of passive failure
3 participants