Add VPC config to estimator for training job creation #353
Conversation
Codecov Report
@@ Coverage Diff @@
## master #353 +/- ##
==========================================
- Coverage 93.1% 93.09% -0.02%
==========================================
Files 51 51
Lines 3555 3564 +9
==========================================
+ Hits 3310 3318 +8
- Misses 245 246 +1
Continue to review full report at Codecov.
|
tests/integ/test_tf.py
Outdated
| @@ -22,6 +22,8 @@ | |||
| from tests.integ.timeout import timeout_and_delete_endpoint_by_name, timeout | |||
|
|
|||
| DATA_PATH = os.path.join(DATA_DIR, 'iris', 'data') | |||
| VPC_SUBNETS = ['subnet-06b8537735fac3757'] | |||
There was a problem hiding this comment.
Is this dependent on our account?
There was a problem hiding this comment.
Yep. I created this VPC in PDX. The test I added the vpc_config is not used in canary so we are not running it in other regions.
There was a problem hiding this comment.
Can we create the VPC configurations and subnet during the test? Are we guaranteed to run this VPC config in the same account all the time?
I think the test will be better suited if we make these changes.
There was a problem hiding this comment.
We certainly can create the VPC in the test and delete it when the test finishes. I understand your concern. On the other hand I do not want our test to depend on the availability of vpc. Perhaps we can pre create vpc configs for all the regions? I am not too keen on this solution either since we might want to run the tests in different accounts in the future. Thoughts?
There was a problem hiding this comment.
Create if not exists? This should be a good tradeoff
|
Create if not exists? I think this should be fine.
On Aug 17, 2018, 2:36 PM -0700, icywang86rui <[email protected]>, wrote:
@icywang86rui commented on this pull request.
________________________________
In tests/integ/test_tf.py<#353 (comment)>:
@@ -22,6 +22,8 @@
from tests.integ.timeout import timeout_and_delete_endpoint_by_name, timeout
DATA_PATH = os.path.join(DATA_DIR, 'iris', 'data')
+VPC_SUBNETS = ['subnet-06b8537735fac3757']
We certainly can create the VPC in the test and delete it when the test finishes. I understand your concern. On the other hand I do not want our test to depend on the availability of vpc. Perhaps we can pre create vpc configs for all the regions? I am not too keen on this solution either since we might want to run the tests in different accounts in the future. Thoughts?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#353 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABEvZ_Jja5fn8PxRDEbK0nU3XziBrIY4ks5uRzdRgaJpZM4V-xin>.
|
CreateTraningJob api supports vpc. This change adds vpc config as an optional argument to Estimator.
* Add VPC config to estimator for training job creation CreateTraningJob api supports vpc. This change adds vpc config as an optional argument to Estimator.
Completed mvs-keras-notebook example
CreateTraningJob api supports vpc. This change adds vpc config as an optional
argument to Estimator.
Issue #, if available:
Description of changes:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.