-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feature: Add optional CodeArtifact login to FrameworkProcessing job script #4145
feature: Add optional CodeArtifact login to FrameworkProcessing job script #4145
Conversation
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.
/bot run all
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.
Please get a review from one member in your team. please follow best practices - add Unit tests , integ tests
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
53c46b0
to
61190de
Compare
@akrishna1995 Can you re-run the tests? I added 3 more unit tests around the code and fixed the issues that popped up in the last run. |
Curious if anyone has had a chance to take a look at this. Would appreciate another review. |
@akuma12 Can you rebase your change? I had a brief look and it lgtm. It would be good to get all the test successful. |
Hi @mohanasudhan and @akuma12, I pulled this PR, and ran the only thing that happened was that it seems like
With no access to the build logs, I wonder if that is the only issue? |
One more thought, my teammate @Stacy-D, has started looking into using processing jobs, and has been experimenting with a different approach for setting up CodeArtifact. Rather than uploading a script that has the hardcoded index in
The script then looks something along the lines of
To make CA optional, the |
I had almost forgotten about this. Thank you @humanzz for your feedback. I'll look into that script change and see if I can modify the code to make use of that. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4145 +/- ##
==========================================
+ Coverage 86.70% 87.44% +0.74%
==========================================
Files 409 389 -20
Lines 39067 36904 -2163
==========================================
- Hits 33872 32272 -1600
+ Misses 5195 4632 -563 ☔ View full report in Codecov by Sentry. |
@humanzz The process using the AWS CLI is muuuuuch simpler. I don't have to rely on boto3, and I confirmed that the PyTorch training images have the AWS CLI installed. Updated the code and added some additional unit tests. |
@mohanasudhan Looks like all tests and lints are passing now, if you could take one last look. |
@akrishna1995 I'd love to get a final review on this, if possible. Thanks! |
ea0263a
to
d68b19c
Compare
@akrishna1995 Could I get a final review on this PR? I'd really like to get my internal projects off of my fork so I can start getting the more recent updates to sagemaker-python-sdk. Thank you! |
@akuma12 |
@sage-maker |
Issue #, if available:
#4144
Description of changes:
This PR adds an optional
codeartifact_repo_arn
parameter to theFrameworkProcessor.run()
method. Providing this ARN will allow the_generate_framework_script()
method to call_get_codeartifact_index()
, which will parse the ARN into a CodeArtifact repo URL, retrieve an authentication token, and write an index option into thepip install -r requirements.txt
call generated by_generate_framework_script()
If
codeartifact_repo_arn
is not provided, then_get_codeartifact_index()
will not be called and nothing new will be injected into therunproc.sh
script.The
_get_codeartifact_index()
code is copied from the sagemaker-training-toolkit. All credit to @humanzz for that update.Testing done:
Validated a PytorchProcessing job both with and without the
codeartifact_repo_arn
parameter. Downloaded the generatedrunproc.sh
file from S3 and verified that the index option is written to the file if the ARN is provided, and does nothing if it is not.I could use some advice when it comes to automated testing, however. Since
_get_codeartifact_index()
interacts with CodeArtifact via Boto3, I was unsure of the best way to handle this. In unit tests, I would typically usemoto
or patch the Boto3make_api_call
method. With the integration tests, I wasn't sure how I should interact with CodeArtifact, or if I should even add an integration test.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.General
Tests
unique_name_from_base
to create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.