-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add model parameters to Estimator, and bump library version to 1.13.0 #450
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
Conversation
…ary version to 1.13.0
Codecov Report
@@ Coverage Diff @@
## master #450 +/- ##
==========================================
- Coverage 93.75% 93.67% -0.08%
==========================================
Files 55 55
Lines 4034 4079 +45
==========================================
+ Hits 3782 3821 +39
- Misses 252 258 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure @eslesar-aws has a chance at some point to look at the doc changes as well
src/sagemaker/estimator.py
Outdated
@@ -601,6 +644,7 @@ def __init__(self, entry_point, source_dir=None, hyperparameters=None, enable_cl | |||
Valid values are defined in the Python logging module. | |||
code_location (str): Name of the S3 bucket where custom code is uploaded (default: None). | |||
If not specified, default bucket created by ``sagemaker.session.Session`` is used. | |||
**kwargs: Additional kwargs passed to the ``EstimatorBase`` constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this line (it's already on l. 652)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove, thanks for pointing it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm curious how I even got that line there. I'll do a second pass over the code changes.
README.rst
Outdated
Incremental Training | ||
~~~~~~~~~~~~~~~~~~~~ | ||
|
||
Incremental training allows you to bring a pre-trained model into a SageMaker training job, to use as a starting point for a new model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: remove the comma in this sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will also remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
giving an approve with the assumption there will be a follow-up PR to fix the two small issues. (talked with @nadiaya offline.)
Removed unnecessary comma from sentence.
Removed duplicated line
…aws#450) * Add incremental training model parameters to Estimator, and bump library version to 1.13.0 * Update README.rst Removed unnecessary comma from sentence. * Update estimator.py Removed duplicated line
…3.0 (aws#450) * add spark image_uri_retriever support for framework_version 3.0 * changing framework_version to 2.4 in spark unit tests * fix spark integration
Issue #, if available:
Description of changes:
This code change adds the following changes to Estimators:
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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.