-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: forward network_isolation parameter to Estimators when False #4543
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
fix: forward network_isolation parameter to Estimators when False #4543
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4543 +/- ##
==========================================
- Coverage 87.50% 87.38% -0.12%
==========================================
Files 391 389 -2
Lines 37098 36779 -319
==========================================
- Hits 32461 32138 -323
- Misses 4637 4641 +4 ☔ View full report in Codecov by Sentry. |
Well it turns out this fails something like 500 unit tests that check the args. I think it is a good failure since it confirms the argument is actually being passed. Just need to go through and update all of them. I just ran a few since there are installation problems with the test requirements on windows. |
6827073
to
621d403
Compare
I think I fixed them all now. Confirms the kwargs weren't being passed at least. They should be passed in all cases to override the config values when needed. |
621d403
to
1d6e9b3
Compare
A sagemaker config file would override this parameter if it was True in the config but False in the Estimator parameters. Closes aws#4542
1d6e9b3
to
04057ff
Compare
A sagemaker config file would override this parameter if it was True in the config but False in the Estimator parameters.
Closes #4542
Issue #, if available: #4542
Description of changes: Forward the parameter to boto.
Testing done: Manually verified via the logs. I'm not sure where to add a unit test for this. If you can point me to the right spot, happy to add.
Merge Checklist
Put an
x
in 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.General
Tests
unique_name_from_base
to create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.