Deprecate enable_cloudwatch_metrics from Frameworks by iquintero · Pull Request #292 · aws/sagemaker-python-sdk

iquintero · 2018-07-13T23:42:34Z

There is a warning message for now, but it will be removed later.

*Issue #276

Description of changes:

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2018-07-14T00:08:31Z

Codecov Report

Merging #292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #292 +/- ##
==========================================
+ Coverage 92.93% 92.93% +<.01% 
==========================================
Files 51 51 Lines 3552 3555 +3 ==========================================
+ Hits 3301 3304 +3 
Misses 251 251

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`88.93% <100%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4613f24...1d10cde. Read the comment docs.

andremoeller · 2018-07-15T04:33:39Z

 self.entry_point = entry_point
- self.enable_cloudwatch_metrics = enable_cloudwatch_metrics
+ if enable_cloudwatch_metrics:
+ logging.warn('enable_cloudwatch_metrics is now deprecated and will be removed in the future.')


Could you use warnings.warn instead, with a DeprecationWarning?
https://docs.python.org/3/library/warnings.html#warnings.warn

There is a warning message for now, but it will be removed later.

Add Chainer hyperparameter tuning notebook

There is a warning message for now, but it will be removed later.

iquintero · 2018-07-13T23:42:34Z

There is a warning message for now, but it will be removed later.

*Issue #276

Description of changes:

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2018-07-14T00:08:31Z

Codecov Report

Merging #292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #292 +/- ##
==========================================
+ Coverage 92.93% 92.93% +<.01% 
==========================================
Files 51 51 Lines 3552 3555 +3 ==========================================
+ Hits 3301 3304 +3 
Misses 251 251

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`88.93% <100%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4613f24...1d10cde. Read the comment docs.

andremoeller · 2018-07-15T04:33:39Z

 self.entry_point = entry_point
- self.enable_cloudwatch_metrics = enable_cloudwatch_metrics
+ if enable_cloudwatch_metrics:
+ logging.warn('enable_cloudwatch_metrics is now deprecated and will be removed in the future.')


Could you use warnings.warn instead, with a DeprecationWarning?
https://docs.python.org/3/library/warnings.html#warnings.warn

There is a warning message for now, but it will be removed later.

Add Chainer hyperparameter tuning notebook

There is a warning message for now, but it will be removed later.

iquintero · 2018-07-13T23:42:34Z

There is a warning message for now, but it will be removed later.

*Issue #276

Description of changes:

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2018-07-14T00:08:31Z

Codecov Report

Merging #292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #292 +/- ##
==========================================
+ Coverage 92.93% 92.93% +<.01% 
==========================================
Files 51 51 Lines 3552 3555 +3 ==========================================
+ Hits 3301 3304 +3 
Misses 251 251

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`88.93% <100%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4613f24...1d10cde. Read the comment docs.

andremoeller · 2018-07-15T04:33:39Z

 self.entry_point = entry_point
- self.enable_cloudwatch_metrics = enable_cloudwatch_metrics
+ if enable_cloudwatch_metrics:
+ logging.warn('enable_cloudwatch_metrics is now deprecated and will be removed in the future.')


Could you use warnings.warn instead, with a DeprecationWarning?
https://docs.python.org/3/library/warnings.html#warnings.warn

There is a warning message for now, but it will be removed later.

Add Chainer hyperparameter tuning notebook

There is a warning message for now, but it will be removed later.

iquintero · 2018-07-13T23:42:34Z

There is a warning message for now, but it will be removed later.

*Issue #276

Description of changes:

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2018-07-14T00:08:31Z

Codecov Report

Merging #292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #292 +/- ##
==========================================
+ Coverage 92.93% 92.93% +<.01% 
==========================================
Files 51 51 Lines 3552 3555 +3 ==========================================
+ Hits 3301 3304 +3 
Misses 251 251

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`88.93% <100%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4613f24...1d10cde. Read the comment docs.

andremoeller · 2018-07-15T04:33:39Z

 self.entry_point = entry_point
- self.enable_cloudwatch_metrics = enable_cloudwatch_metrics
+ if enable_cloudwatch_metrics:
+ logging.warn('enable_cloudwatch_metrics is now deprecated and will be removed in the future.')


Could you use warnings.warn instead, with a DeprecationWarning?
https://docs.python.org/3/library/warnings.html#warnings.warn

There is a warning message for now, but it will be removed later.

Add Chainer hyperparameter tuning notebook

There is a warning message for now, but it will be removed later.

iquintero · 2018-07-13T23:42:34Z

There is a warning message for now, but it will be removed later.

*Issue #276

Description of changes:

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2018-07-14T00:08:31Z

Codecov Report

Merging #292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #292 +/- ##
==========================================
+ Coverage 92.93% 92.93% +<.01% 
==========================================
Files 51 51 Lines 3552 3555 +3 ==========================================
+ Hits 3301 3304 +3 
Misses 251 251

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`88.93% <100%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4613f24...1d10cde. Read the comment docs.

andremoeller · 2018-07-15T04:33:39Z

 self.entry_point = entry_point
- self.enable_cloudwatch_metrics = enable_cloudwatch_metrics
+ if enable_cloudwatch_metrics:
+ logging.warn('enable_cloudwatch_metrics is now deprecated and will be removed in the future.')


Could you use warnings.warn instead, with a DeprecationWarning?
https://docs.python.org/3/library/warnings.html#warnings.warn

There is a warning message for now, but it will be removed later.

Add Chainer hyperparameter tuning notebook

There is a warning message for now, but it will be removed later.

iquintero · 2018-07-13T23:42:34Z

There is a warning message for now, but it will be removed later.

*Issue #276

Description of changes:

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2018-07-14T00:08:31Z

Codecov Report

Merging #292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #292 +/- ##
==========================================
+ Coverage 92.93% 92.93% +<.01% 
==========================================
Files 51 51 Lines 3552 3555 +3 ==========================================
+ Hits 3301 3304 +3 
Misses 251 251

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`88.93% <100%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4613f24...1d10cde. Read the comment docs.

andremoeller · 2018-07-15T04:33:39Z

 self.entry_point = entry_point
- self.enable_cloudwatch_metrics = enable_cloudwatch_metrics
+ if enable_cloudwatch_metrics:
+ logging.warn('enable_cloudwatch_metrics is now deprecated and will be removed in the future.')


Could you use warnings.warn instead, with a DeprecationWarning?
https://docs.python.org/3/library/warnings.html#warnings.warn

There is a warning message for now, but it will be removed later.

Add Chainer hyperparameter tuning notebook

There is a warning message for now, but it will be removed later.

iquintero · 2018-07-13T23:42:34Z

There is a warning message for now, but it will be removed later.

*Issue #276

Description of changes:

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2018-07-14T00:08:31Z

Codecov Report

Merging #292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #292 +/- ##
==========================================
+ Coverage 92.93% 92.93% +<.01% 
==========================================
Files 51 51 Lines 3552 3555 +3 ==========================================
+ Hits 3301 3304 +3 
Misses 251 251

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`88.93% <100%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4613f24...1d10cde. Read the comment docs.

andremoeller · 2018-07-15T04:33:39Z

 self.entry_point = entry_point
- self.enable_cloudwatch_metrics = enable_cloudwatch_metrics
+ if enable_cloudwatch_metrics:
+ logging.warn('enable_cloudwatch_metrics is now deprecated and will be removed in the future.')


Could you use warnings.warn instead, with a DeprecationWarning?
https://docs.python.org/3/library/warnings.html#warnings.warn

There is a warning message for now, but it will be removed later.

Add Chainer hyperparameter tuning notebook

There is a warning message for now, but it will be removed later.

iquintero · 2018-07-13T23:42:34Z

There is a warning message for now, but it will be removed later.

*Issue #276

Description of changes:

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.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io · 2018-07-14T00:08:31Z

Codecov Report

Merging #292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #292 +/- ##
==========================================
+ Coverage 92.93% 92.93% +<.01% 
==========================================
Files 51 51 Lines 3552 3555 +3 ==========================================
+ Hits 3301 3304 +3 
Misses 251 251

Impacted Files	Coverage Δ
src/sagemaker/estimator.py	`88.93% <100%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4613f24...1d10cde. Read the comment docs.

andremoeller · 2018-07-15T04:33:39Z

 self.entry_point = entry_point
- self.enable_cloudwatch_metrics = enable_cloudwatch_metrics
+ if enable_cloudwatch_metrics:
+ logging.warn('enable_cloudwatch_metrics is now deprecated and will be removed in the future.')


Could you use warnings.warn instead, with a DeprecationWarning?
https://docs.python.org/3/library/warnings.html#warnings.warn

There is a warning message for now, but it will be removed later.

Add Chainer hyperparameter tuning notebook

There is a warning message for now, but it will be removed later.

iquintero requested a review from nadiaya July 13, 2018 23:42

andremoeller suggested changes Jul 15, 2018
View reviewed changes

Ignacio Quintero added 4 commits July 19, 2018 18:20

Deprecate enable_cloudwatch_metrics from Frameworks
b339a4e
There is a warning message for now, but it will be removed later.

Fix unit tests
c086913

address PR comments
9967d52

Merge branch 'master' into remove_cw_metrics_arg
1b083f5

andremoeller previously approved these changes Jul 20, 2018
View reviewed changes

andremoeller added 2 commits July 20, 2018 00:24

Merge branch 'master' into remove_cw_metrics_arg
bfaafe3

Merge branch 'master' into remove_cw_metrics_arg
d6729b3

andremoeller dismissed their stale review via d6729b3July 23, 2018 20:19

andremoeller previously approved these changes Jul 23, 2018
View reviewed changes

humanzz mentioned this pull request Jul 24, 2018
[BUG] Error thrown when trying to enable instance metrics for MXNet #276
Closed

iquinteroand others added 4 commits August 3, 2018 14:24

Merge branch 'master' into remove_cw_metrics_arg
226720a

Merge branch 'master' into remove_cw_metrics_arg
370e96a

Merge branch 'master' into remove_cw_metrics_arg
1d10cde

properly update changelog
e5012e1

iquintero dismissed andremoeller’s stale review via e5012e1August 16, 2018 21:34

laurenyu approved these changes Aug 16, 2018
View reviewed changes

nadiaya approved these changes Aug 16, 2018
View reviewed changes

iquintero merged commit 80fa52e into aws:masterAug 17, 2018

pdasamzn pushed a commit to pdasamzn/sagemaker-python-sdk that referenced this pull request Nov 1, 2018
Deprecate enable_cloudwatch_metrics from Frameworks (aws#292)
055a8ad
There is a warning message for now, but it will be removed later.

apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
Merge pull requestaws#292from awslabs/laurenyu-chainer-tuning
0606937
Add Chainer hyperparameter tuning notebook

knakad added a commit to knakad/sagemaker-python-sdk that referenced this pull request Dec 4, 2019
fix: correct S3Downloader behavior (aws#292)
4b9bb4f

knakad added a commit that referenced this pull request Dec 4, 2019
fix: correct S3Downloader behavior (#292)
80b5e18

laurenyu mentioned this pull request May 7, 2020
Deprecate unused parameters #1475
Closed

Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Deprecate enable_cloudwatch_metrics from Frameworks (aws#292)
aa8d8b1
There is a warning message for now, but it will be removed later.

Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
fix: correct S3Downloader behavior (aws#292)
ddb71c7