Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated deeplearning.template #21

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

updated deeplearning.template #21

wants to merge 6 commits into from

Conversation

ajayvohra2005
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@mirocody
Copy link
Contributor

@ajayvohra2005 Can you provide some details about this PR?

@mirocody
Copy link
Contributor

Hi @ajayvohra2005, as we discussed, do you have an ETA to make those features in a customized template and keep the default template as a generic one? Let me know if you need any help.

Also the latest commit in master has been updated to DLAMI v21.0.

@@ -84,6 +143,11 @@
"AllowedValues" : [ "AmazonLinux", "Ubuntu" ],
"ConstraintDescription" : "Amazon Supported Image Type"
},
"AMIOverride" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really great feature that allow users not limited to current version of dlami. Thanks!

"Default": "/dev/sda1",
"Description" : "Ebs device name",
"Type" : "String",
"AllowedValues": [ "/dev/sda1" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is only one allowed value here, should it be default?

@@ -13,19 +72,19 @@
"Default" : "1"
},
"InstanceType" : {
"Description" : "The EC2 instance type for workers.For GPUs choose g3.xx, p2.xx or p3.xx.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use origin line g3.xx, p2.xx or p3.xx. here

"g3.16xlarge",
"g3.8xlarge",
"g3.4xlarge",
"p3dn.24xlarge",
Copy link
Contributor

Choose a reason for hiding this comment

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

g2 are no longer supported by DLAMI so they should be removed.

"p3dn.24xlarge",
"g3.16xlarge",
"g3.8xlarge",
"g3.4xlarge",

Please using these values here.

@@ -103,44 +167,57 @@
"Description" : "The Linux mount point for the EFS volume",
"Type": "String",
"MinLength": "1",
"Default": "myEFSvolume"
"Default": "efs"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to keep as myEFSvolume as current README file and auto script commands are running based on this string. Is there a reason it should be changed to efs?

"Default": "true",
"AllowedValues": [ "false", "true"]
},
"ActivateCondaEnv" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will benefit people who use conda environment, but it's better to keep this as an optional, given that we allow using customized AMI so people might also use base AMI or other AMI which does not install Conda then this will fail.

@mirocody
Copy link
Contributor

@ajayvohra2005 Thanks for creating this PR for adding so many awesome features!
As this is an open source that people will for different purposes, we should keep it as generic as possible. So my suggestions are:

  1. Two arguments should be added to default template in master: AMIOverride and ActivateCondaEnv(Only if we can make this as optional, see comments above).
  2. Another two arguments in private templates are also very useful: MyVpcId and PrivateSubnetId. But during my test, if we select any value in those fields, we have to choose one value from dropdown rather than creating a new VPC, which is not correct behavior. Is it possible to fix this?
  3. I would suggest to make these two templates in separate files for users' need.

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

Successfully merging this pull request may close these issues.

3 participants