-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fixing container path for source files when using local_session #499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #499 +/- ##
==========================================
+ Coverage 92.77% 92.79% +0.01%
==========================================
Files 71 71
Lines 5359 5371 +12
==========================================
+ Hits 4972 4984 +12
Misses 387 387
Continue to review full report at Codecov.
|
Not sure if this is the best way to fix the bug, but I wanted to put this here as an issue and proposed fix. If anyone has any other ideas, please feel free to submit a new PR and close this one! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Would you mind adding some tests?
Yup will do |
4118b9d
to
5f54934
Compare
Updated commit - 5f54934 with extended unit tests |
5f54934
to
fb7269d
Compare
Thanks for updating! There are a couple flake8 errors:
|
fb7269d
to
94da7e6
Compare
Cool. Should be fixed now! |
@@ -106,6 +106,8 @@ def train(self, input_data_config, output_data_config, hyperparameters, job_name | |||
data_dir = self._create_tmp_folder() | |||
volumes = self._prepare_training_volumes(data_dir, input_data_config, output_data_config, | |||
hyperparameters) | |||
# If local, source directory needs to be updated to mounted /opt/ml/code path | |||
hyperparameters = self._update_local_src_path(hyperparameters, key=sagemaker.estimator.DIR_PARAM_NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an integ test is failing because it expects this hyperparameter to be JSON (at least for the SageMaker deep learning framework images)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch! Updated hyperparameters with json conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unable to run integration tests on my machine, so please let me know if there are any more errors on those tests
63c43c7
to
516ffb4
Compare
Copying env variable instead Adding tests forl local code
516ffb4
to
6a73652
Compare
@bearpelican the integ tests look fine now, but could you add the changelog entry to a new 1.16.3 section? (1.16.2 got released yesterday) |
Issue #, if available:
Fixes:
No such file or directory
error when running estimatorfit/predict
with a local sagemaker session and a local script.When running sagemaker in local mode,
sagemaker_submit_directory
is set to the the local environment path (Ex: file:///home/ec2-user/mnist/src'), when it should be set to the container path - '/opt/ml/code'. The container path is mounted in src/local/image.py.The container does not have access to the local environment path, which is why a
No such file or directory
error happens.Minimal example that breaks:
https://gist.github.com/bearpelican/440d10f28003c3653c7866c129418ad7
This was taken directly from the sagemaker example notebook:
https://github.com/awslabs/amazon-sagemaker-examples/tree/master/sagemaker-python-sdk/pytorch_mnist
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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.