Add ntm algorithm with doc, unit tests, integ tests - #73

Merged
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm
Feb 9, 2018
Merged

Add ntm algorithm with doc, unit tests, integ tests#73
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm

Conversation

@yangaws

Copy link
Copy Markdown
Contributor

Since ntm does similar job to lda, the implementation basically follows LDA. All codes include 4 parts:

1, NTM, NTMModel, NTMPredictor implementation
2, Unit tests
3, Integ tests
4, Doc

@yangaws
yangaws requested a review from lukmisFebruary 7, 2018 18:48
Comment threadsrc/sagemaker/amazon/ntm.py Outdated

return NTMModel(self.model_data, self.role, sagemaker_session=self.sagemaker_session)

def fit(self, records, mini_batch_size, **kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the doc (https://docs.aws.amazon.com/sagemaker/latest/dg/ntm_hyperparameters.html) mini_batch_size is not required. This function should not be necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do validation for mini_batch_size if provided.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks.

Now remove old validator and validate range of mini_batch_size instead.

Comment threadtests/integ/test_ntm.py Outdated
assert record.label["topic_mixture"] is not None


def _prepare_record_set_from_local_files(dir_path, destination, num_records, feature_dim, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move to a separate location as it is reused by both NTM and LDA?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved the method to new file that imported by both lda and ntm.

Comment threadtests/integ/test_ntm.py Outdated

assert len(result) == 1
for record in result:
assert record.label["topic_mixture"] is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! Changed to topic_weights.

Comment threadtests/integ/test_ntm.py Outdated

record_set = _prepare_record_set_from_local_files(data_path, ntm.data_location,
len(all_records), feature_num, sagemaker_session)
ntm.fit(record_set, 100)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we can skip 2nd parameter here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to None. I think we still need to pass a None there even if we don't want to pass any values.

Comment threadtests/unit/test_ntm.py Outdated
NTM(epochs='other', sagemaker_session=sagemaker_session, **ALL_REQ_ARGS)


def test_epochs_validation_fail_value(sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the validation checks both min and max it would be great if we had both conditions checked for these HPs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated.

Now all hyper-parameters with a range will be validated for both lower and upper limit.

Comment threadtests/unit/test_ntm.py Outdated
MINI_BATCH_SIZE = 200
HYPERPARAMS = {'num_topics': NUM_TOPICS, 'feature_dim': FEATURE_DIM, 'mini_batch_size': MINI_BATCH_SIZE}
STRINGIFIED_HYPERPARAMS = dict([(x, str(y)) for x, y in HYPERPARAMS.items()])
HP_TRAIN_CALL = dict(BASE_TRAIN_CALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not being used anywhere please remove.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed unnecessary parameters.

@lukmislukmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

Comment threadREADME.rst

git clone https://github.com/aws/sagemaker-python-sdk.git
python setup.py sdist
pip install dist/sagemaker-1.0.3.tar.gz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to bump the version here, please update setup.py and CHANGELOG

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed! Thanks

channel='train')

with pytest.raises(ValueError):
with pytest.raises((TypeError, ValueError)):

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Some comments here:

The reason I use tuple (TypeError, ValueError) is because, different error is raised in different python versions. Python 2 raises ValueError and Python 3 raises TypeError.

@lukmis
lukmis merged commit 795b030 into aws:masterFeb 9, 2018
jalabort added a commit to hudl/sagemaker-python-sdk that referenced this pull request Mar 1, 2018
* Add data_type to hyperparameters (aws#54)
When we describe a training job the data type of the hyper parameters is
lost because we use a dict[str, str]. This adds a new field to
Hyperparameter so that we can convert the datatypes at runtime.
instead of validating with isinstance(), we cast the hp value to the type it
is meant to be. This enforces a "strongly typed" value. When we
deserialize from the API string responses it becomes easier to deal with
too.
* Add wrapper for LDA. (aws#56)
Update CHANGELOG and bump the version number.
* Add support for async fit() (aws#59)
when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.
_prepare_init_params_from_job_description() is now a classmethod instead
of being a static method. Each class is responsible to implement their
specific logic to convert a training job description into arguments that
can be passed to its own __init__()
* Fix Estimator role expansion (aws#68)
Instead of manually constructing the role ARN, use the IAM boto client
to do it. This properly expands service-roles and regular roles.
* Add FM and LDA to the documentation. (aws#66)
* Fix description of an argument of sagemaker.session.train (aws#69)
* Fix description of an argument of sagemaker.session.train
'input_config' should be an array which has channel objects.
* Add a link to the botocore docs
* Use 'list' instead of 'array' in the description
* Add ntm algorithm with doc, unit tests, integ tests (aws#73)
* JSON serializer: predictor.predict accepts dictionaries (aws#62)
Add support for serializing python dictionaries to json
Add prediction with dictionary in tf iris integ test
* Fixing timeouts for PCA async integration test. (aws#78)
Execute tf_cifar test without logs to eliminate delay to detect that job has finished.
* Fixes in LinearLearner and unit tests addition. (aws#77)
* Print out billable seconds after training completes (aws#30)
* Added: print out billable seconds after training completes
* Fixed: test_session.py to pass unit tests
* Fixed: removed offending tzlocal()
* Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81)
* Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82)
* Update .gitignore to ignore pytest_cache.
* Support TensorFlow-1.5.0 and MXNet-1.0.0
* Update and refactor tests. Add tests for fw_utils.
* Fix typo.
* Update changelog for 1.1.0 (aws#85)
apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
…lled
Scikit learn is already installed on mead
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Sign up for freeto 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.

2 participants

@yangaws@lukmis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add ntm algorithm with doc, unit tests, integ tests - #73

Merged
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm
Feb 9, 2018
Merged

Add ntm algorithm with doc, unit tests, integ tests#73
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm

Conversation

@yangaws

Copy link
Copy Markdown
Contributor

Since ntm does similar job to lda, the implementation basically follows LDA. All codes include 4 parts:

1, NTM, NTMModel, NTMPredictor implementation
2, Unit tests
3, Integ tests
4, Doc

@yangaws
yangaws requested a review from lukmisFebruary 7, 2018 18:48
Comment threadsrc/sagemaker/amazon/ntm.py Outdated

return NTMModel(self.model_data, self.role, sagemaker_session=self.sagemaker_session)

def fit(self, records, mini_batch_size, **kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the doc (https://docs.aws.amazon.com/sagemaker/latest/dg/ntm_hyperparameters.html) mini_batch_size is not required. This function should not be necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do validation for mini_batch_size if provided.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks.

Now remove old validator and validate range of mini_batch_size instead.

Comment threadtests/integ/test_ntm.py Outdated
assert record.label["topic_mixture"] is not None


def _prepare_record_set_from_local_files(dir_path, destination, num_records, feature_dim, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move to a separate location as it is reused by both NTM and LDA?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved the method to new file that imported by both lda and ntm.

Comment threadtests/integ/test_ntm.py Outdated

assert len(result) == 1
for record in result:
assert record.label["topic_mixture"] is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! Changed to topic_weights.

Comment threadtests/integ/test_ntm.py Outdated

record_set = _prepare_record_set_from_local_files(data_path, ntm.data_location,
len(all_records), feature_num, sagemaker_session)
ntm.fit(record_set, 100)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we can skip 2nd parameter here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to None. I think we still need to pass a None there even if we don't want to pass any values.

Comment threadtests/unit/test_ntm.py Outdated
NTM(epochs='other', sagemaker_session=sagemaker_session, **ALL_REQ_ARGS)


def test_epochs_validation_fail_value(sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the validation checks both min and max it would be great if we had both conditions checked for these HPs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated.

Now all hyper-parameters with a range will be validated for both lower and upper limit.

Comment threadtests/unit/test_ntm.py Outdated
MINI_BATCH_SIZE = 200
HYPERPARAMS = {'num_topics': NUM_TOPICS, 'feature_dim': FEATURE_DIM, 'mini_batch_size': MINI_BATCH_SIZE}
STRINGIFIED_HYPERPARAMS = dict([(x, str(y)) for x, y in HYPERPARAMS.items()])
HP_TRAIN_CALL = dict(BASE_TRAIN_CALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not being used anywhere please remove.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed unnecessary parameters.

@lukmislukmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

Comment threadREADME.rst

git clone https://github.com/aws/sagemaker-python-sdk.git
python setup.py sdist
pip install dist/sagemaker-1.0.3.tar.gz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to bump the version here, please update setup.py and CHANGELOG

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed! Thanks

channel='train')

with pytest.raises(ValueError):
with pytest.raises((TypeError, ValueError)):

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Some comments here:

The reason I use tuple (TypeError, ValueError) is because, different error is raised in different python versions. Python 2 raises ValueError and Python 3 raises TypeError.

@lukmis
lukmis merged commit 795b030 into aws:masterFeb 9, 2018
jalabort added a commit to hudl/sagemaker-python-sdk that referenced this pull request Mar 1, 2018
* Add data_type to hyperparameters (aws#54)
When we describe a training job the data type of the hyper parameters is
lost because we use a dict[str, str]. This adds a new field to
Hyperparameter so that we can convert the datatypes at runtime.
instead of validating with isinstance(), we cast the hp value to the type it
is meant to be. This enforces a "strongly typed" value. When we
deserialize from the API string responses it becomes easier to deal with
too.
* Add wrapper for LDA. (aws#56)
Update CHANGELOG and bump the version number.
* Add support for async fit() (aws#59)
when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.
_prepare_init_params_from_job_description() is now a classmethod instead
of being a static method. Each class is responsible to implement their
specific logic to convert a training job description into arguments that
can be passed to its own __init__()
* Fix Estimator role expansion (aws#68)
Instead of manually constructing the role ARN, use the IAM boto client
to do it. This properly expands service-roles and regular roles.
* Add FM and LDA to the documentation. (aws#66)
* Fix description of an argument of sagemaker.session.train (aws#69)
* Fix description of an argument of sagemaker.session.train
'input_config' should be an array which has channel objects.
* Add a link to the botocore docs
* Use 'list' instead of 'array' in the description
* Add ntm algorithm with doc, unit tests, integ tests (aws#73)
* JSON serializer: predictor.predict accepts dictionaries (aws#62)
Add support for serializing python dictionaries to json
Add prediction with dictionary in tf iris integ test
* Fixing timeouts for PCA async integration test. (aws#78)
Execute tf_cifar test without logs to eliminate delay to detect that job has finished.
* Fixes in LinearLearner and unit tests addition. (aws#77)
* Print out billable seconds after training completes (aws#30)
* Added: print out billable seconds after training completes
* Fixed: test_session.py to pass unit tests
* Fixed: removed offending tzlocal()
* Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81)
* Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82)
* Update .gitignore to ignore pytest_cache.
* Support TensorFlow-1.5.0 and MXNet-1.0.0
* Update and refactor tests. Add tests for fw_utils.
* Fix typo.
* Update changelog for 1.1.0 (aws#85)
apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
…lled
Scikit learn is already installed on mead
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Sign up for freeto 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.

2 participants

@yangaws@lukmis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add ntm algorithm with doc, unit tests, integ tests - #73

Merged
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm
Feb 9, 2018
Merged

Add ntm algorithm with doc, unit tests, integ tests#73
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm

Conversation

@yangaws

Copy link
Copy Markdown
Contributor

Since ntm does similar job to lda, the implementation basically follows LDA. All codes include 4 parts:

1, NTM, NTMModel, NTMPredictor implementation
2, Unit tests
3, Integ tests
4, Doc

@yangaws
yangaws requested a review from lukmisFebruary 7, 2018 18:48
Comment threadsrc/sagemaker/amazon/ntm.py Outdated

return NTMModel(self.model_data, self.role, sagemaker_session=self.sagemaker_session)

def fit(self, records, mini_batch_size, **kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the doc (https://docs.aws.amazon.com/sagemaker/latest/dg/ntm_hyperparameters.html) mini_batch_size is not required. This function should not be necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do validation for mini_batch_size if provided.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks.

Now remove old validator and validate range of mini_batch_size instead.

Comment threadtests/integ/test_ntm.py Outdated
assert record.label["topic_mixture"] is not None


def _prepare_record_set_from_local_files(dir_path, destination, num_records, feature_dim, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move to a separate location as it is reused by both NTM and LDA?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved the method to new file that imported by both lda and ntm.

Comment threadtests/integ/test_ntm.py Outdated

assert len(result) == 1
for record in result:
assert record.label["topic_mixture"] is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! Changed to topic_weights.

Comment threadtests/integ/test_ntm.py Outdated

record_set = _prepare_record_set_from_local_files(data_path, ntm.data_location,
len(all_records), feature_num, sagemaker_session)
ntm.fit(record_set, 100)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we can skip 2nd parameter here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to None. I think we still need to pass a None there even if we don't want to pass any values.

Comment threadtests/unit/test_ntm.py Outdated
NTM(epochs='other', sagemaker_session=sagemaker_session, **ALL_REQ_ARGS)


def test_epochs_validation_fail_value(sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the validation checks both min and max it would be great if we had both conditions checked for these HPs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated.

Now all hyper-parameters with a range will be validated for both lower and upper limit.

Comment threadtests/unit/test_ntm.py Outdated
MINI_BATCH_SIZE = 200
HYPERPARAMS = {'num_topics': NUM_TOPICS, 'feature_dim': FEATURE_DIM, 'mini_batch_size': MINI_BATCH_SIZE}
STRINGIFIED_HYPERPARAMS = dict([(x, str(y)) for x, y in HYPERPARAMS.items()])
HP_TRAIN_CALL = dict(BASE_TRAIN_CALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not being used anywhere please remove.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed unnecessary parameters.

@lukmislukmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

Comment threadREADME.rst

git clone https://github.com/aws/sagemaker-python-sdk.git
python setup.py sdist
pip install dist/sagemaker-1.0.3.tar.gz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to bump the version here, please update setup.py and CHANGELOG

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed! Thanks

channel='train')

with pytest.raises(ValueError):
with pytest.raises((TypeError, ValueError)):

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Some comments here:

The reason I use tuple (TypeError, ValueError) is because, different error is raised in different python versions. Python 2 raises ValueError and Python 3 raises TypeError.

@lukmis
lukmis merged commit 795b030 into aws:masterFeb 9, 2018
jalabort added a commit to hudl/sagemaker-python-sdk that referenced this pull request Mar 1, 2018
* Add data_type to hyperparameters (aws#54)
When we describe a training job the data type of the hyper parameters is
lost because we use a dict[str, str]. This adds a new field to
Hyperparameter so that we can convert the datatypes at runtime.
instead of validating with isinstance(), we cast the hp value to the type it
is meant to be. This enforces a "strongly typed" value. When we
deserialize from the API string responses it becomes easier to deal with
too.
* Add wrapper for LDA. (aws#56)
Update CHANGELOG and bump the version number.
* Add support for async fit() (aws#59)
when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.
_prepare_init_params_from_job_description() is now a classmethod instead
of being a static method. Each class is responsible to implement their
specific logic to convert a training job description into arguments that
can be passed to its own __init__()
* Fix Estimator role expansion (aws#68)
Instead of manually constructing the role ARN, use the IAM boto client
to do it. This properly expands service-roles and regular roles.
* Add FM and LDA to the documentation. (aws#66)
* Fix description of an argument of sagemaker.session.train (aws#69)
* Fix description of an argument of sagemaker.session.train
'input_config' should be an array which has channel objects.
* Add a link to the botocore docs
* Use 'list' instead of 'array' in the description
* Add ntm algorithm with doc, unit tests, integ tests (aws#73)
* JSON serializer: predictor.predict accepts dictionaries (aws#62)
Add support for serializing python dictionaries to json
Add prediction with dictionary in tf iris integ test
* Fixing timeouts for PCA async integration test. (aws#78)
Execute tf_cifar test without logs to eliminate delay to detect that job has finished.
* Fixes in LinearLearner and unit tests addition. (aws#77)
* Print out billable seconds after training completes (aws#30)
* Added: print out billable seconds after training completes
* Fixed: test_session.py to pass unit tests
* Fixed: removed offending tzlocal()
* Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81)
* Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82)
* Update .gitignore to ignore pytest_cache.
* Support TensorFlow-1.5.0 and MXNet-1.0.0
* Update and refactor tests. Add tests for fw_utils.
* Fix typo.
* Update changelog for 1.1.0 (aws#85)
apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
…lled
Scikit learn is already installed on mead
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Sign up for freeto 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.

2 participants

@yangaws@lukmis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add ntm algorithm with doc, unit tests, integ tests - #73

Merged
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm
Feb 9, 2018
Merged

Add ntm algorithm with doc, unit tests, integ tests#73
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm

Conversation

@yangaws

Copy link
Copy Markdown
Contributor

Since ntm does similar job to lda, the implementation basically follows LDA. All codes include 4 parts:

1, NTM, NTMModel, NTMPredictor implementation
2, Unit tests
3, Integ tests
4, Doc

@yangaws
yangaws requested a review from lukmisFebruary 7, 2018 18:48
Comment threadsrc/sagemaker/amazon/ntm.py Outdated

return NTMModel(self.model_data, self.role, sagemaker_session=self.sagemaker_session)

def fit(self, records, mini_batch_size, **kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the doc (https://docs.aws.amazon.com/sagemaker/latest/dg/ntm_hyperparameters.html) mini_batch_size is not required. This function should not be necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do validation for mini_batch_size if provided.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks.

Now remove old validator and validate range of mini_batch_size instead.

Comment threadtests/integ/test_ntm.py Outdated
assert record.label["topic_mixture"] is not None


def _prepare_record_set_from_local_files(dir_path, destination, num_records, feature_dim, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move to a separate location as it is reused by both NTM and LDA?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved the method to new file that imported by both lda and ntm.

Comment threadtests/integ/test_ntm.py Outdated

assert len(result) == 1
for record in result:
assert record.label["topic_mixture"] is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! Changed to topic_weights.

Comment threadtests/integ/test_ntm.py Outdated

record_set = _prepare_record_set_from_local_files(data_path, ntm.data_location,
len(all_records), feature_num, sagemaker_session)
ntm.fit(record_set, 100)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we can skip 2nd parameter here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to None. I think we still need to pass a None there even if we don't want to pass any values.

Comment threadtests/unit/test_ntm.py Outdated
NTM(epochs='other', sagemaker_session=sagemaker_session, **ALL_REQ_ARGS)


def test_epochs_validation_fail_value(sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the validation checks both min and max it would be great if we had both conditions checked for these HPs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated.

Now all hyper-parameters with a range will be validated for both lower and upper limit.

Comment threadtests/unit/test_ntm.py Outdated
MINI_BATCH_SIZE = 200
HYPERPARAMS = {'num_topics': NUM_TOPICS, 'feature_dim': FEATURE_DIM, 'mini_batch_size': MINI_BATCH_SIZE}
STRINGIFIED_HYPERPARAMS = dict([(x, str(y)) for x, y in HYPERPARAMS.items()])
HP_TRAIN_CALL = dict(BASE_TRAIN_CALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not being used anywhere please remove.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed unnecessary parameters.

@lukmislukmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

Comment threadREADME.rst

git clone https://github.com/aws/sagemaker-python-sdk.git
python setup.py sdist
pip install dist/sagemaker-1.0.3.tar.gz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to bump the version here, please update setup.py and CHANGELOG

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed! Thanks

channel='train')

with pytest.raises(ValueError):
with pytest.raises((TypeError, ValueError)):

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Some comments here:

The reason I use tuple (TypeError, ValueError) is because, different error is raised in different python versions. Python 2 raises ValueError and Python 3 raises TypeError.

@lukmis
lukmis merged commit 795b030 into aws:masterFeb 9, 2018
jalabort added a commit to hudl/sagemaker-python-sdk that referenced this pull request Mar 1, 2018
* Add data_type to hyperparameters (aws#54)
When we describe a training job the data type of the hyper parameters is
lost because we use a dict[str, str]. This adds a new field to
Hyperparameter so that we can convert the datatypes at runtime.
instead of validating with isinstance(), we cast the hp value to the type it
is meant to be. This enforces a "strongly typed" value. When we
deserialize from the API string responses it becomes easier to deal with
too.
* Add wrapper for LDA. (aws#56)
Update CHANGELOG and bump the version number.
* Add support for async fit() (aws#59)
when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.
_prepare_init_params_from_job_description() is now a classmethod instead
of being a static method. Each class is responsible to implement their
specific logic to convert a training job description into arguments that
can be passed to its own __init__()
* Fix Estimator role expansion (aws#68)
Instead of manually constructing the role ARN, use the IAM boto client
to do it. This properly expands service-roles and regular roles.
* Add FM and LDA to the documentation. (aws#66)
* Fix description of an argument of sagemaker.session.train (aws#69)
* Fix description of an argument of sagemaker.session.train
'input_config' should be an array which has channel objects.
* Add a link to the botocore docs
* Use 'list' instead of 'array' in the description
* Add ntm algorithm with doc, unit tests, integ tests (aws#73)
* JSON serializer: predictor.predict accepts dictionaries (aws#62)
Add support for serializing python dictionaries to json
Add prediction with dictionary in tf iris integ test
* Fixing timeouts for PCA async integration test. (aws#78)
Execute tf_cifar test without logs to eliminate delay to detect that job has finished.
* Fixes in LinearLearner and unit tests addition. (aws#77)
* Print out billable seconds after training completes (aws#30)
* Added: print out billable seconds after training completes
* Fixed: test_session.py to pass unit tests
* Fixed: removed offending tzlocal()
* Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81)
* Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82)
* Update .gitignore to ignore pytest_cache.
* Support TensorFlow-1.5.0 and MXNet-1.0.0
* Update and refactor tests. Add tests for fw_utils.
* Fix typo.
* Update changelog for 1.1.0 (aws#85)
apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
…lled
Scikit learn is already installed on mead
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Sign up for freeto 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.

2 participants

@yangaws@lukmis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add ntm algorithm with doc, unit tests, integ tests - #73

Merged
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm
Feb 9, 2018
Merged

Add ntm algorithm with doc, unit tests, integ tests#73
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm

Conversation

@yangaws

Copy link
Copy Markdown
Contributor

Since ntm does similar job to lda, the implementation basically follows LDA. All codes include 4 parts:

1, NTM, NTMModel, NTMPredictor implementation
2, Unit tests
3, Integ tests
4, Doc

@yangaws
yangaws requested a review from lukmisFebruary 7, 2018 18:48
Comment threadsrc/sagemaker/amazon/ntm.py Outdated

return NTMModel(self.model_data, self.role, sagemaker_session=self.sagemaker_session)

def fit(self, records, mini_batch_size, **kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the doc (https://docs.aws.amazon.com/sagemaker/latest/dg/ntm_hyperparameters.html) mini_batch_size is not required. This function should not be necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do validation for mini_batch_size if provided.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks.

Now remove old validator and validate range of mini_batch_size instead.

Comment threadtests/integ/test_ntm.py Outdated
assert record.label["topic_mixture"] is not None


def _prepare_record_set_from_local_files(dir_path, destination, num_records, feature_dim, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move to a separate location as it is reused by both NTM and LDA?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved the method to new file that imported by both lda and ntm.

Comment threadtests/integ/test_ntm.py Outdated

assert len(result) == 1
for record in result:
assert record.label["topic_mixture"] is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! Changed to topic_weights.

Comment threadtests/integ/test_ntm.py Outdated

record_set = _prepare_record_set_from_local_files(data_path, ntm.data_location,
len(all_records), feature_num, sagemaker_session)
ntm.fit(record_set, 100)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we can skip 2nd parameter here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to None. I think we still need to pass a None there even if we don't want to pass any values.

Comment threadtests/unit/test_ntm.py Outdated
NTM(epochs='other', sagemaker_session=sagemaker_session, **ALL_REQ_ARGS)


def test_epochs_validation_fail_value(sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the validation checks both min and max it would be great if we had both conditions checked for these HPs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated.

Now all hyper-parameters with a range will be validated for both lower and upper limit.

Comment threadtests/unit/test_ntm.py Outdated
MINI_BATCH_SIZE = 200
HYPERPARAMS = {'num_topics': NUM_TOPICS, 'feature_dim': FEATURE_DIM, 'mini_batch_size': MINI_BATCH_SIZE}
STRINGIFIED_HYPERPARAMS = dict([(x, str(y)) for x, y in HYPERPARAMS.items()])
HP_TRAIN_CALL = dict(BASE_TRAIN_CALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not being used anywhere please remove.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed unnecessary parameters.

@lukmislukmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

Comment threadREADME.rst

git clone https://github.com/aws/sagemaker-python-sdk.git
python setup.py sdist
pip install dist/sagemaker-1.0.3.tar.gz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to bump the version here, please update setup.py and CHANGELOG

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed! Thanks

channel='train')

with pytest.raises(ValueError):
with pytest.raises((TypeError, ValueError)):

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Some comments here:

The reason I use tuple (TypeError, ValueError) is because, different error is raised in different python versions. Python 2 raises ValueError and Python 3 raises TypeError.

@lukmis
lukmis merged commit 795b030 into aws:masterFeb 9, 2018
jalabort added a commit to hudl/sagemaker-python-sdk that referenced this pull request Mar 1, 2018
* Add data_type to hyperparameters (aws#54)
When we describe a training job the data type of the hyper parameters is
lost because we use a dict[str, str]. This adds a new field to
Hyperparameter so that we can convert the datatypes at runtime.
instead of validating with isinstance(), we cast the hp value to the type it
is meant to be. This enforces a "strongly typed" value. When we
deserialize from the API string responses it becomes easier to deal with
too.
* Add wrapper for LDA. (aws#56)
Update CHANGELOG and bump the version number.
* Add support for async fit() (aws#59)
when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.
_prepare_init_params_from_job_description() is now a classmethod instead
of being a static method. Each class is responsible to implement their
specific logic to convert a training job description into arguments that
can be passed to its own __init__()
* Fix Estimator role expansion (aws#68)
Instead of manually constructing the role ARN, use the IAM boto client
to do it. This properly expands service-roles and regular roles.
* Add FM and LDA to the documentation. (aws#66)
* Fix description of an argument of sagemaker.session.train (aws#69)
* Fix description of an argument of sagemaker.session.train
'input_config' should be an array which has channel objects.
* Add a link to the botocore docs
* Use 'list' instead of 'array' in the description
* Add ntm algorithm with doc, unit tests, integ tests (aws#73)
* JSON serializer: predictor.predict accepts dictionaries (aws#62)
Add support for serializing python dictionaries to json
Add prediction with dictionary in tf iris integ test
* Fixing timeouts for PCA async integration test. (aws#78)
Execute tf_cifar test without logs to eliminate delay to detect that job has finished.
* Fixes in LinearLearner and unit tests addition. (aws#77)
* Print out billable seconds after training completes (aws#30)
* Added: print out billable seconds after training completes
* Fixed: test_session.py to pass unit tests
* Fixed: removed offending tzlocal()
* Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81)
* Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82)
* Update .gitignore to ignore pytest_cache.
* Support TensorFlow-1.5.0 and MXNet-1.0.0
* Update and refactor tests. Add tests for fw_utils.
* Fix typo.
* Update changelog for 1.1.0 (aws#85)
apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
…lled
Scikit learn is already installed on mead
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Sign up for freeto 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.

2 participants

@yangaws@lukmis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add ntm algorithm with doc, unit tests, integ tests - #73

Merged
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm
Feb 9, 2018
Merged

Add ntm algorithm with doc, unit tests, integ tests#73
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm

Conversation

@yangaws

Copy link
Copy Markdown
Contributor

Since ntm does similar job to lda, the implementation basically follows LDA. All codes include 4 parts:

1, NTM, NTMModel, NTMPredictor implementation
2, Unit tests
3, Integ tests
4, Doc

@yangaws
yangaws requested a review from lukmisFebruary 7, 2018 18:48
Comment threadsrc/sagemaker/amazon/ntm.py Outdated

return NTMModel(self.model_data, self.role, sagemaker_session=self.sagemaker_session)

def fit(self, records, mini_batch_size, **kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the doc (https://docs.aws.amazon.com/sagemaker/latest/dg/ntm_hyperparameters.html) mini_batch_size is not required. This function should not be necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do validation for mini_batch_size if provided.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks.

Now remove old validator and validate range of mini_batch_size instead.

Comment threadtests/integ/test_ntm.py Outdated
assert record.label["topic_mixture"] is not None


def _prepare_record_set_from_local_files(dir_path, destination, num_records, feature_dim, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move to a separate location as it is reused by both NTM and LDA?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved the method to new file that imported by both lda and ntm.

Comment threadtests/integ/test_ntm.py Outdated

assert len(result) == 1
for record in result:
assert record.label["topic_mixture"] is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! Changed to topic_weights.

Comment threadtests/integ/test_ntm.py Outdated

record_set = _prepare_record_set_from_local_files(data_path, ntm.data_location,
len(all_records), feature_num, sagemaker_session)
ntm.fit(record_set, 100)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we can skip 2nd parameter here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to None. I think we still need to pass a None there even if we don't want to pass any values.

Comment threadtests/unit/test_ntm.py Outdated
NTM(epochs='other', sagemaker_session=sagemaker_session, **ALL_REQ_ARGS)


def test_epochs_validation_fail_value(sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the validation checks both min and max it would be great if we had both conditions checked for these HPs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated.

Now all hyper-parameters with a range will be validated for both lower and upper limit.

Comment threadtests/unit/test_ntm.py Outdated
MINI_BATCH_SIZE = 200
HYPERPARAMS = {'num_topics': NUM_TOPICS, 'feature_dim': FEATURE_DIM, 'mini_batch_size': MINI_BATCH_SIZE}
STRINGIFIED_HYPERPARAMS = dict([(x, str(y)) for x, y in HYPERPARAMS.items()])
HP_TRAIN_CALL = dict(BASE_TRAIN_CALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not being used anywhere please remove.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed unnecessary parameters.

@lukmislukmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

Comment threadREADME.rst

git clone https://github.com/aws/sagemaker-python-sdk.git
python setup.py sdist
pip install dist/sagemaker-1.0.3.tar.gz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to bump the version here, please update setup.py and CHANGELOG

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed! Thanks

channel='train')

with pytest.raises(ValueError):
with pytest.raises((TypeError, ValueError)):

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Some comments here:

The reason I use tuple (TypeError, ValueError) is because, different error is raised in different python versions. Python 2 raises ValueError and Python 3 raises TypeError.

@lukmis
lukmis merged commit 795b030 into aws:masterFeb 9, 2018
jalabort added a commit to hudl/sagemaker-python-sdk that referenced this pull request Mar 1, 2018
* Add data_type to hyperparameters (aws#54)
When we describe a training job the data type of the hyper parameters is
lost because we use a dict[str, str]. This adds a new field to
Hyperparameter so that we can convert the datatypes at runtime.
instead of validating with isinstance(), we cast the hp value to the type it
is meant to be. This enforces a "strongly typed" value. When we
deserialize from the API string responses it becomes easier to deal with
too.
* Add wrapper for LDA. (aws#56)
Update CHANGELOG and bump the version number.
* Add support for async fit() (aws#59)
when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.
_prepare_init_params_from_job_description() is now a classmethod instead
of being a static method. Each class is responsible to implement their
specific logic to convert a training job description into arguments that
can be passed to its own __init__()
* Fix Estimator role expansion (aws#68)
Instead of manually constructing the role ARN, use the IAM boto client
to do it. This properly expands service-roles and regular roles.
* Add FM and LDA to the documentation. (aws#66)
* Fix description of an argument of sagemaker.session.train (aws#69)
* Fix description of an argument of sagemaker.session.train
'input_config' should be an array which has channel objects.
* Add a link to the botocore docs
* Use 'list' instead of 'array' in the description
* Add ntm algorithm with doc, unit tests, integ tests (aws#73)
* JSON serializer: predictor.predict accepts dictionaries (aws#62)
Add support for serializing python dictionaries to json
Add prediction with dictionary in tf iris integ test
* Fixing timeouts for PCA async integration test. (aws#78)
Execute tf_cifar test without logs to eliminate delay to detect that job has finished.
* Fixes in LinearLearner and unit tests addition. (aws#77)
* Print out billable seconds after training completes (aws#30)
* Added: print out billable seconds after training completes
* Fixed: test_session.py to pass unit tests
* Fixed: removed offending tzlocal()
* Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81)
* Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82)
* Update .gitignore to ignore pytest_cache.
* Support TensorFlow-1.5.0 and MXNet-1.0.0
* Update and refactor tests. Add tests for fw_utils.
* Fix typo.
* Update changelog for 1.1.0 (aws#85)
apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
…lled
Scikit learn is already installed on mead
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Sign up for freeto 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.

2 participants

@yangaws@lukmis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add ntm algorithm with doc, unit tests, integ tests - #73

Merged
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm
Feb 9, 2018
Merged

Add ntm algorithm with doc, unit tests, integ tests#73
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm

Conversation

@yangaws

Copy link
Copy Markdown
Contributor

Since ntm does similar job to lda, the implementation basically follows LDA. All codes include 4 parts:

1, NTM, NTMModel, NTMPredictor implementation
2, Unit tests
3, Integ tests
4, Doc

@yangaws
yangaws requested a review from lukmisFebruary 7, 2018 18:48
Comment threadsrc/sagemaker/amazon/ntm.py Outdated

return NTMModel(self.model_data, self.role, sagemaker_session=self.sagemaker_session)

def fit(self, records, mini_batch_size, **kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the doc (https://docs.aws.amazon.com/sagemaker/latest/dg/ntm_hyperparameters.html) mini_batch_size is not required. This function should not be necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do validation for mini_batch_size if provided.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks.

Now remove old validator and validate range of mini_batch_size instead.

Comment threadtests/integ/test_ntm.py Outdated
assert record.label["topic_mixture"] is not None


def _prepare_record_set_from_local_files(dir_path, destination, num_records, feature_dim, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move to a separate location as it is reused by both NTM and LDA?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved the method to new file that imported by both lda and ntm.

Comment threadtests/integ/test_ntm.py Outdated

assert len(result) == 1
for record in result:
assert record.label["topic_mixture"] is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! Changed to topic_weights.

Comment threadtests/integ/test_ntm.py Outdated

record_set = _prepare_record_set_from_local_files(data_path, ntm.data_location,
len(all_records), feature_num, sagemaker_session)
ntm.fit(record_set, 100)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we can skip 2nd parameter here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to None. I think we still need to pass a None there even if we don't want to pass any values.

Comment threadtests/unit/test_ntm.py Outdated
NTM(epochs='other', sagemaker_session=sagemaker_session, **ALL_REQ_ARGS)


def test_epochs_validation_fail_value(sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the validation checks both min and max it would be great if we had both conditions checked for these HPs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated.

Now all hyper-parameters with a range will be validated for both lower and upper limit.

Comment threadtests/unit/test_ntm.py Outdated
MINI_BATCH_SIZE = 200
HYPERPARAMS = {'num_topics': NUM_TOPICS, 'feature_dim': FEATURE_DIM, 'mini_batch_size': MINI_BATCH_SIZE}
STRINGIFIED_HYPERPARAMS = dict([(x, str(y)) for x, y in HYPERPARAMS.items()])
HP_TRAIN_CALL = dict(BASE_TRAIN_CALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not being used anywhere please remove.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed unnecessary parameters.

@lukmislukmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

Comment threadREADME.rst

git clone https://github.com/aws/sagemaker-python-sdk.git
python setup.py sdist
pip install dist/sagemaker-1.0.3.tar.gz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to bump the version here, please update setup.py and CHANGELOG

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed! Thanks

channel='train')

with pytest.raises(ValueError):
with pytest.raises((TypeError, ValueError)):

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Some comments here:

The reason I use tuple (TypeError, ValueError) is because, different error is raised in different python versions. Python 2 raises ValueError and Python 3 raises TypeError.

@lukmis
lukmis merged commit 795b030 into aws:masterFeb 9, 2018
jalabort added a commit to hudl/sagemaker-python-sdk that referenced this pull request Mar 1, 2018
* Add data_type to hyperparameters (aws#54)
When we describe a training job the data type of the hyper parameters is
lost because we use a dict[str, str]. This adds a new field to
Hyperparameter so that we can convert the datatypes at runtime.
instead of validating with isinstance(), we cast the hp value to the type it
is meant to be. This enforces a "strongly typed" value. When we
deserialize from the API string responses it becomes easier to deal with
too.
* Add wrapper for LDA. (aws#56)
Update CHANGELOG and bump the version number.
* Add support for async fit() (aws#59)
when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.
_prepare_init_params_from_job_description() is now a classmethod instead
of being a static method. Each class is responsible to implement their
specific logic to convert a training job description into arguments that
can be passed to its own __init__()
* Fix Estimator role expansion (aws#68)
Instead of manually constructing the role ARN, use the IAM boto client
to do it. This properly expands service-roles and regular roles.
* Add FM and LDA to the documentation. (aws#66)
* Fix description of an argument of sagemaker.session.train (aws#69)
* Fix description of an argument of sagemaker.session.train
'input_config' should be an array which has channel objects.
* Add a link to the botocore docs
* Use 'list' instead of 'array' in the description
* Add ntm algorithm with doc, unit tests, integ tests (aws#73)
* JSON serializer: predictor.predict accepts dictionaries (aws#62)
Add support for serializing python dictionaries to json
Add prediction with dictionary in tf iris integ test
* Fixing timeouts for PCA async integration test. (aws#78)
Execute tf_cifar test without logs to eliminate delay to detect that job has finished.
* Fixes in LinearLearner and unit tests addition. (aws#77)
* Print out billable seconds after training completes (aws#30)
* Added: print out billable seconds after training completes
* Fixed: test_session.py to pass unit tests
* Fixed: removed offending tzlocal()
* Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81)
* Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82)
* Update .gitignore to ignore pytest_cache.
* Support TensorFlow-1.5.0 and MXNet-1.0.0
* Update and refactor tests. Add tests for fw_utils.
* Fix typo.
* Update changelog for 1.1.0 (aws#85)
apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
…lled
Scikit learn is already installed on mead
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Sign up for freeto 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.

2 participants

@yangaws@lukmis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add ntm algorithm with doc, unit tests, integ tests - #73

Merged
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm
Feb 9, 2018
Merged

Add ntm algorithm with doc, unit tests, integ tests#73
lukmis merged 9 commits into
aws:masterfrom
yangaws:ntm

Conversation

@yangaws

Copy link
Copy Markdown
Contributor

Since ntm does similar job to lda, the implementation basically follows LDA. All codes include 4 parts:

1, NTM, NTMModel, NTMPredictor implementation
2, Unit tests
3, Integ tests
4, Doc

@yangaws
yangaws requested a review from lukmisFebruary 7, 2018 18:48
Comment threadsrc/sagemaker/amazon/ntm.py Outdated

return NTMModel(self.model_data, self.role, sagemaker_session=self.sagemaker_session)

def fit(self, records, mini_batch_size, **kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the doc (https://docs.aws.amazon.com/sagemaker/latest/dg/ntm_hyperparameters.html) mini_batch_size is not required. This function should not be necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do validation for mini_batch_size if provided.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks.

Now remove old validator and validate range of mini_batch_size instead.

Comment threadtests/integ/test_ntm.py Outdated
assert record.label["topic_mixture"] is not None


def _prepare_record_set_from_local_files(dir_path, destination, num_records, feature_dim, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move to a separate location as it is reused by both NTM and LDA?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved the method to new file that imported by both lda and ntm.

Comment threadtests/integ/test_ntm.py Outdated

assert len(result) == 1
for record in result:
assert record.label["topic_mixture"] is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! Changed to topic_weights.

Comment threadtests/integ/test_ntm.py Outdated

record_set = _prepare_record_set_from_local_files(data_path, ntm.data_location,
len(all_records), feature_num, sagemaker_session)
ntm.fit(record_set, 100)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we can skip 2nd parameter here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to None. I think we still need to pass a None there even if we don't want to pass any values.

Comment threadtests/unit/test_ntm.py Outdated
NTM(epochs='other', sagemaker_session=sagemaker_session, **ALL_REQ_ARGS)


def test_epochs_validation_fail_value(sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the validation checks both min and max it would be great if we had both conditions checked for these HPs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated.

Now all hyper-parameters with a range will be validated for both lower and upper limit.

Comment threadtests/unit/test_ntm.py Outdated
MINI_BATCH_SIZE = 200
HYPERPARAMS = {'num_topics': NUM_TOPICS, 'feature_dim': FEATURE_DIM, 'mini_batch_size': MINI_BATCH_SIZE}
STRINGIFIED_HYPERPARAMS = dict([(x, str(y)) for x, y in HYPERPARAMS.items()])
HP_TRAIN_CALL = dict(BASE_TRAIN_CALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not being used anywhere please remove.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed unnecessary parameters.

@lukmislukmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

Comment threadREADME.rst

git clone https://github.com/aws/sagemaker-python-sdk.git
python setup.py sdist
pip install dist/sagemaker-1.0.3.tar.gz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to bump the version here, please update setup.py and CHANGELOG

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed! Thanks

channel='train')

with pytest.raises(ValueError):
with pytest.raises((TypeError, ValueError)):

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Some comments here:

The reason I use tuple (TypeError, ValueError) is because, different error is raised in different python versions. Python 2 raises ValueError and Python 3 raises TypeError.

@lukmis
lukmis merged commit 795b030 into aws:masterFeb 9, 2018
jalabort added a commit to hudl/sagemaker-python-sdk that referenced this pull request Mar 1, 2018
* Add data_type to hyperparameters (aws#54)
When we describe a training job the data type of the hyper parameters is
lost because we use a dict[str, str]. This adds a new field to
Hyperparameter so that we can convert the datatypes at runtime.
instead of validating with isinstance(), we cast the hp value to the type it
is meant to be. This enforces a "strongly typed" value. When we
deserialize from the API string responses it becomes easier to deal with
too.
* Add wrapper for LDA. (aws#56)
Update CHANGELOG and bump the version number.
* Add support for async fit() (aws#59)
when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.
_prepare_init_params_from_job_description() is now a classmethod instead
of being a static method. Each class is responsible to implement their
specific logic to convert a training job description into arguments that
can be passed to its own __init__()
* Fix Estimator role expansion (aws#68)
Instead of manually constructing the role ARN, use the IAM boto client
to do it. This properly expands service-roles and regular roles.
* Add FM and LDA to the documentation. (aws#66)
* Fix description of an argument of sagemaker.session.train (aws#69)
* Fix description of an argument of sagemaker.session.train
'input_config' should be an array which has channel objects.
* Add a link to the botocore docs
* Use 'list' instead of 'array' in the description
* Add ntm algorithm with doc, unit tests, integ tests (aws#73)
* JSON serializer: predictor.predict accepts dictionaries (aws#62)
Add support for serializing python dictionaries to json
Add prediction with dictionary in tf iris integ test
* Fixing timeouts for PCA async integration test. (aws#78)
Execute tf_cifar test without logs to eliminate delay to detect that job has finished.
* Fixes in LinearLearner and unit tests addition. (aws#77)
* Print out billable seconds after training completes (aws#30)
* Added: print out billable seconds after training completes
* Fixed: test_session.py to pass unit tests
* Fixed: removed offending tzlocal()
* Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81)
* Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82)
* Update .gitignore to ignore pytest_cache.
* Support TensorFlow-1.5.0 and MXNet-1.0.0
* Update and refactor tests. Add tests for fw_utils.
* Fix typo.
* Update changelog for 1.1.0 (aws#85)
apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018
…lled
Scikit learn is already installed on mead
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
Sign up for freeto 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.

2 participants

@yangaws@lukmis