Add support for additional files #494

mvsusp · 2018-11-16T09:27:07Z

Issue #, if available:

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.

iquintero · 2018-11-16T18:19:19Z

src/sagemaker/pytorch/estimator.py

-                directory are preserved when training on Amazon SageMaker.
+            source_dir (str or [str]): Single path (absolute or relative) or a list of paths to directories with
+                any other training source code dependencies aside from the entry point file (default: None).
+                The structures within this directories are preserved when training on Amazon SageMaker.


these directories

laurenyu · 2018-11-16T18:37:30Z

src/sagemaker/chainer/estimator.py

-            source_dir (str): Path (absolute or relative) to a directory with any other training
-                source code dependencies aside from tne entry point file (default: None). Structure within this
-                directory are preserved when training on Amazon SageMaker.
+            source_dir (str or [str]): Single path (absolute or relative) or a list of paths to directories with


let's be more explicit and use list[str] for lists. also I'd change it to "A single path"

laurenyu · 2018-11-16T18:39:02Z

src/sagemaker/chainer/estimator.py

-                source code dependencies aside from tne entry point file (default: None). Structure within this
-                directory are preserved when training on Amazon SageMaker.
+            source_dir (str or [str]): Single path (absolute or relative) or a list of paths to directories with
+                any other training source code dependencies aside from the entry point file (default: None).


I'd change this line to "any source code (other than the entry point file) needed for training"

laurenyu · 2018-11-16T18:40:39Z

src/sagemaker/chainer/estimator.py

-                directory are preserved when training on Amazon SageMaker.
+            source_dir (str or [str]): Single path (absolute or relative) or a list of paths to directories with
+                any other training source code dependencies aside from the entry point file (default: None).
+                The structures within this directories are preserved when training on Amazon SageMaker.


should we include explanation about the structure of how to access each of the directories? i.e. I assume (but am still reading the PR) that it'll be something like:

| base dir from unpacking the tar | - source dir 1 | - source dir 2 | - etc.

based on reading the docstring, but it'd be good to be explicit about it

Yeah, more documentation and examples would be helpful.

laurenyu · 2018-11-16T18:41:42Z

src/sagemaker/fw_utils.py

@@ -118,31 +118,43 @@ def tar_and_upload_dir(session, bucket, s3_key_prefix, script, directory):
        bucket (str): S3 bucket to which the compressed file is uploaded.
        s3_key_prefix (str): Prefix for the S3 key.
        script (str): Script filename.
-        directory (str): Directory containing the source file. If it starts with "s3://", no action is taken.
+        directory (str or None): Directory containing the source file. If it starts with "s3://", no action is taken.



add additional_files to the docstring

laurenyu · 2018-11-16T18:49:38Z

src/sagemaker/fw_utils.py

-            source_files = [os.path.join(directory, name) for name in os.listdir(directory)]
+    key = '%s/sourcedir.tar.gz' % s3_key_prefix
+
+    if directory and directory.lower().startswith("s3://"):


single quotes for the string

jesterhazy · 2018-11-16T20:16:05Z

CHANGELOG.rst

@@ -14,6 +14,7 @@ CHANGELOG
 * feature: HyperparameterTuner: Make input channels optional
 * feature: Add support for Chainer 5.0
 * feature: Estimator: add support for MetricDefinitions
+* feature: source_dir accepts a list of directories


i still haven't heard an explanation for why this is useful. why can't user just stage their files properly in a single source dir before creating an estimator?

From someone on the outside looking in, this looks confusing and unneeded. I understand there's probably a (pressing) reason why we want to do this - but it feels like a hack that we're adding to the PythonSDK to overcome a problem in another system.

I am also against this change because of the same reason Jonathan mentioned.

User could stage the files before calling the estimator. User can also copy their own code to S3, and not use the estimator. Or they could build their own docker container, and make boto calls instead of using the SDK at all. The point of this project is to make things easier.

owen-t · 2018-11-16T20:25:11Z

src/sagemaker/fw_utils.py

+        os.remove(tar_file)
+
+
+def _list_root_files(script, directory, additional_files):


This is not independently unit tested. This logic is confusing (I'm not sure I completely understand it). I suggest unit testing this and providing some developer documentation describing the contract.

I am personally against testing private methods, it adds unnecessary coupling to non public facing signatures and does not provide a good grasp of the functionality.

I do agree that testing should be extensive, and cover any edge cases. The tests that I wrote are here https://github.com/aws/sagemaker-python-sdk/pull/494/files#diff-3108f99e19f25f4c77ad4f63d486b174R147

Let me know if you any suggestions of improvement of these methods.

mvsusp

Thanks for the reviews, I will close this PR for now.

mvsusp · 2018-11-16T22:41:21Z

src/sagemaker/chainer/estimator.py

-                directory are preserved when training on Amazon SageMaker.
+            source_dir (str or [str]): Single path (absolute or relative) or a list of paths to directories with
+                any other training source code dependencies aside from the entry point file (default: None).
+                The structures within this directories are preserved when training on Amazon SageMaker.


Yeah, more documentation and examples would be helpful.

mvsusp · 2018-11-16T22:45:57Z

src/sagemaker/fw_utils.py

+        os.remove(tar_file)
+
+
+def _list_root_files(script, directory, additional_files):


I am personally against testing private methods, it adds unnecessary coupling to non public facing signatures and does not provide a good grasp of the functionality.

I do agree that testing should be extensive, and cover any edge cases. The tests that I wrote are here https://github.com/aws/sagemaker-python-sdk/pull/494/files#diff-3108f99e19f25f4c77ad4f63d486b174R147

Let me know if you any suggestions of improvement of these methods.

codecov-io · 2018-11-18T06:06:35Z

Codecov Report

Merging #494 into master will decrease coverage by 0.04%.
The diff coverage is 87.87%.

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
- Coverage   94.28%   94.24%   -0.05%     
==========================================
  Files          59       59              
  Lines        4551     4567      +16     
==========================================
+ Hits         4291     4304      +13     
- Misses        260      263       +3

Impacted Files	Coverage Δ
src/sagemaker/mxnet/estimator.py	`100% <ø> (ø)`	⬆️
src/sagemaker/chainer/estimator.py	`100% <ø> (ø)`	⬆️
src/sagemaker/pytorch/estimator.py	`100% <ø> (ø)`	⬆️
src/sagemaker/fw_utils.py	`100% <100%> (ø)`	⬆️
src/sagemaker/model.py	`93.15% <60%> (-2.51%)`	⬇️
src/sagemaker/estimator.py	`89.79% <71.42%> (-0.52%)`	⬇️
src/sagemaker/tensorflow/estimator.py	`95.18% <0%> (+0.53%)`	⬆️

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 e37ac12...cf52ca8. Read the comment docs.

…ty Monitors support. (#494) (#503)

mvsusp and others added 7 commits November 16, 2018 01:26

Add support for additional files

4d2a7f5

Merge branch 'master' into mvs-many-src-dirs

7326b22

Fix docstring

df542d3

Add tests

e684021

Update docstrings and documentations

ef0cc4e

Fix model tests

2d27e67

Update changelog

a04affc

mvsusp changed the title ~~[WIP] Add support for additional files~~ Add support for additional files Nov 16, 2018

mvsusp requested review from nadiaya and iquintero November 16, 2018 14:07

iquintero reviewed Nov 16, 2018

View reviewed changes

laurenyu reviewed Nov 16, 2018

View reviewed changes

jesterhazy suggested changes Nov 16, 2018

View reviewed changes

owen-t suggested changes Nov 16, 2018

View reviewed changes

mvsusp commented Nov 16, 2018

View reviewed changes

mvsusp closed this Nov 16, 2018

Add local integ test

75d5715

nadiaya reopened this Nov 18, 2018

Merge branch 'master' into mvs-many-src-dirs

cf52ca8

mvsusp closed this Nov 19, 2018

ChoiByungWook pushed a commit that referenced this pull request Dec 8, 2020

feature: Add Clarify Processor, Model Bias, Explainability, and Quali…

553029b

…ty Monitors support. (#494) (#503)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for additional files #494

Add support for additional files #494

mvsusp commented Nov 16, 2018 •

edited

Loading

iquintero Nov 16, 2018

laurenyu Nov 16, 2018

laurenyu Nov 16, 2018

laurenyu Nov 16, 2018

mvsusp Nov 16, 2018

laurenyu Nov 16, 2018

laurenyu Nov 16, 2018

jesterhazy Nov 16, 2018

owen-t Nov 16, 2018

iquintero Nov 16, 2018

leopd Nov 16, 2018

owen-t Nov 16, 2018

mvsusp Nov 16, 2018

mvsusp left a comment

mvsusp Nov 16, 2018

mvsusp Nov 16, 2018

codecov-io commented Nov 18, 2018 •

edited

Loading

		os.remove(tar_file)


		def _list_root_files(script, directory, additional_files):

Add support for additional files #494

Add support for additional files #494

Conversation

mvsusp commented Nov 16, 2018 • edited Loading

Merge Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvsusp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 18, 2018 • edited Loading

Codecov Report

mvsusp commented Nov 16, 2018 •

edited

Loading

codecov-io commented Nov 18, 2018 •

edited

Loading