Uh oh!
There was an error while loading. Please reload this page.
Add support for additional files - #494
Conversation
| 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. |
| 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 |
There was a problem hiding this comment.
let's be more explicit and use list[str] for lists. also I'd change it to "A single path"
| 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). |
There was a problem hiding this comment.
I'd change this line to "any source code (other than the entry point file) needed for training"
| 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, more documentation and examples would be helpful.
| 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. | ||
There was a problem hiding this comment.
add additional_files to the docstring
| 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://"): |
| * 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am also against this change because of the same reason Jonathan mentioned.
There was a problem hiding this comment.
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.
| os.remove(tar_file) | ||
| def _list_root_files(script, directory, additional_files): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Thanks for the reviews, I will close this PR for now.
| 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. |
There was a problem hiding this comment.
Yeah, more documentation and examples would be helpful.
| os.remove(tar_file) | ||
| def _list_root_files(script, directory, additional_files): |
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
Issue #, if available:
Description of changes:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.