feat: support codeartifact for installing requirements.txt packages #187

humanzz · 2023-07-10T14:08:37Z

Issue #, if available: #167

Description of changes:

update modules.install_requirements to check for the presence of codeartifact (CA_* prefixed) environment variable
if env variable is present, build the authenticated endpoint index url, and add that to the pip install command
update sagemaker test dependency to support the setting of environment variables and update tox to resolve conflicts with newer sagemaker
add commented out integ test for installing requirements from codeartifact

Testing done:

unit tests
commented out integ test due to infrastructure changes requirements + feat: support codeartifact for installing requirements.txt packages #187 (comment)

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

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have used the regional endpoint when creating S3 and/or STS clients (if appropriate)
I have updated any necessary documentation, including READMEs

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

humanzz · 2023-07-10T14:09:20Z

This is related to the similar PR for sagemaker-inference-toolkit at aws/sagemaker-inference-toolkit#130

humanzz · 2023-07-10T14:31:17Z

I've been using the codeartifact-related code for a few months already. I was not able to add any related integration tests as such tests would require access to a fixed codeartifact repo, and for the container to have access to AWS credentials.

I'm asking for guidance if any such integ tests would be needed here.

Additionally, is anything required in README to mention this CodeArtifact support?

humanzz · 2023-07-11T12:38:36Z

As an attempt to integ test this (code not in a committable state), I made the following changes (git diff)

diff --git a/setup.py b/setup.py
index 9b30321..17aea4a 100644
--- a/setup.py
+++ b/setup.py
@@ -80,11 +80,11 @@ setuptools.setup(
     install_requires=required_packages,
     extras_require={
         "test": [
-            "tox==3.13.1",
+            "tox==4.6.4",
             "pytest==4.4.1",
             "pytest-cov",
             "mock",
-            "sagemaker[local]<2",
+            "sagemaker[local]===2.172.0",
             "black==22.3.0 ; python_version >= '3.7'",
         ]
     },
diff --git a/test/integration/local/test_dummy.py b/test/integration/local/test_dummy.py
index f4db94e..8bc21bd 100644
--- a/test/integration/local/test_dummy.py
+++ b/test/integration/local/test_dummy.py
@@ -38,15 +38,24 @@ def container():
 def test_install_requirements(capsys):
     estimator = Estimator(
         image_uri="sagemaker-training-toolkit-test:dummy",
-        role="SageMakerRole",
+        # role="SageMakerRole",
+        role="...",
         instance_count=1,
         instance_type="local",
+        environment={
+            "CA_REPOSITORY_ARN": "..."
+        }
     )

     estimator.fit()

     stdout = capsys.readouterr().out

+    print(stdout)
+
     assert "Installing collected packages: pyfiglet" in stdout
     assert "Successfully installed pyfiglet-0.8.post1" in stdout
     assert "Reporting training SUCCESS" in stdout

which

Update container building to include the CA_* environment variables. I had to update sagemaker to latest version, coz the version used did not support environment= for Estimator. I also had to update tox as older version had conflicts with newer sagemaker
Update test_install_requirements to print the container's stdout so I can see the pip install commands

With that, I got stdout looking like

iqe4fvsnb7-algo-1-0z7sx  | Looking in indexes: https://xxx:****@xxx-xxx.d.codeartifact.us-west-2.amazonaws.com/pypi/xxx/simple/
iqe4fvsnb7-algo-1-0z7sx  | Collecting pyfiglet==0.8.post1 (from -r requirements.txt (line 1))
iqe4fvsnb7-algo-1-0z7sx  |   Downloading https://xxx-xxx.d.codeartifact.us-west-2.amazonaws.com/pypi/xxx/simple/pyfiglet/0.8.post1/pyfiglet-0.8.post1-py2.py3-none-any.whl (865 kB)

humanzz · 2023-07-15T15:14:47Z

Based on #187 (comment), I've updated the PR to include a commented out integ test, that includes updates to sagemaker and tox

mohanasudhan · 2023-07-17T16:05:51Z

src/sagemaker_training/modules.py

+        token = auth_token_response["authorizationToken"]
+        endpoint_response = client.get_repository_endpoint(domain=domain, domainOwner=owner, repository=repository, format="pypi")
+        unauthenticated_index = endpoint_response["repositoryEndpoint"]
+        return re.sub("https://", "https://aws:{}@".format(token), re.sub("{}/?$".format(repository), "{}/simple/".format(repository), unauthenticated_index))


Please ensure https://aws works for all regions. Some snow-forted region may have different prefix.

LGTM otherwise.

+1. Have you tested this in China region? Are we sure it will not be like https://aws.cn?

As per https://aws.amazon.com/codeartifact/faq/, CodeArtifact is not available everywhere

In which AWS Regions is CodeArtifact available? CodeArtifact is available in the following 13 [AWS Regions](https://aws.amazon.com/about-aws/global-infrastructure/regional-product-services/): US East (N.Virginia) US East (Ohio) US West (Oregon) EU (Ireland) EU (London) EU (Frankfurt) EU (Stockholm) EU (Milan) EU (Paris) Asia Pacific (Sydney) Asia Pacific (Tokyo) Asia Pacific (Mumbai) Asia Pacific (Singapore).

Additionally, the aws: part is not really abt the partition, it's the username e.g. https://username:[email protected]/somewhere

client.get_repository_endpoint returns the https://example.com/somewhere, then the following code is abt adding the username/password, and appending some path to the url

humanzz · 2023-07-24T18:52:47Z

The inference-side change has been merged at

feat: support codeartifact for installing requirements.txt packages sagemaker-inference-toolkit#130

Appreciate if you can have a look here.

emeraldbay · 2023-08-01T03:42:14Z

test/integration/local/test_dummy.py

    assert "Reporting training SUCCESS" in stdout
+
+
+# def test_install_requirements_from_codeartifact(capsys):


Will you enable this test later?

So, for this to really be enabled, it requires that the build infrastructure for this package to actually have a codeartifact repo which I believe it doesn't.
I've written the integ test, that can be enabled if the build infra has such access.

So, at the moment, the answer is no, I don't think I can enable this on my own.

emeraldbay · 2023-08-01T03:43:02Z

src/sagemaker_training/modules.py

+    https://docs.aws.amazon.com/service-authorization/latest/reference/list_awscodeartifact.html#awscodeartifact-resources-for-iam-policies
+    :return: authenticated codeartifact index url
+    """
+    repository_arn = os.getenv(CA_REPOSITORY_ARN_ENV)


Please add a repository_arn sample in comment

The second documentation link already has the format of the arn.

arn:${Partition}:codeartifact:${Region}:${Account}:repository/${DomainName}/${RepositoryName}

Also, keep in mind, this is a private method, but am happy to add this above format for clarity if u think that the link is not enough

Yes, pls add it. Thanks

emeraldbay

LGTM

- update modules.install_requirements to check for the presence of codeartifact (CA_* prefixed) environment variable - if env variable is present, build the authenticated endpoint index url, and add that to the pip install command - update sagemaker test dependency to support the setting of environment variables and update tox to resolve conflicts with newer sagemaker - add commented out integ test for installing requirements from codeartifact

emeraldbay

LGTM

…ckages (aws#187)" This reverts commit 0ad97fd.

humanzz mentioned this pull request Jul 10, 2023

[feature-request] Support installing dependencies in requirements.txt from CodeArtifact for both training/inference SageMaker Containers aws/deep-learning-containers#2509

Closed

5 tasks

humanzz force-pushed the codeartifact branch from 43f5b3e to 21a156f Compare July 15, 2023 15:11

mohanasudhan reviewed Jul 17, 2023

View reviewed changes

mohanasudhan approved these changes Jul 17, 2023

View reviewed changes

humanzz force-pushed the codeartifact branch 2 times, most recently from 9a75eee to 621ac15 Compare July 21, 2023 10:06

emeraldbay reviewed Aug 1, 2023

View reviewed changes

humanzz force-pushed the codeartifact branch from 621ac15 to ca64365 Compare August 2, 2023 15:57

emeraldbay previously approved these changes Aug 2, 2023

View reviewed changes

humanzz force-pushed the codeartifact branch from ca64365 to 5957388 Compare August 3, 2023 17:26

emeraldbay closed this Aug 4, 2023

emeraldbay reopened this Aug 4, 2023

yl-to closed this Aug 4, 2023

yl-to reopened this Aug 4, 2023

yl-to closed this Aug 4, 2023

yl-to reopened this Aug 4, 2023

humanzz mentioned this pull request Aug 7, 2023

feat: support codeartifact for installing requirements.txt packages #189

Closed

6 tasks

humanzz dismissed emeraldbay’s stale review via d68cfbf August 7, 2023 21:12

humanzz force-pushed the codeartifact branch 5 times, most recently from ec547c1 to 6bb7f8f Compare August 8, 2023 00:53

humanzz force-pushed the codeartifact branch from 6bb7f8f to cf72cf1 Compare August 8, 2023 01:32

humanzz linked an issue Aug 8, 2023 that may be closed by this pull request

Support CodeArtifact repositories for installing Python packages #167

Closed

emeraldbay approved these changes Aug 8, 2023

View reviewed changes

humanzz merged commit 0ad97fd into aws:master Aug 8, 2023

emeraldbay pushed a commit to emeraldbay/sagemaker-training-toolkit that referenced this pull request Oct 12, 2023

Revert "feat: support codeartifact for installing requirements.txt pa…

7d7c8b3

…ckages (aws#187)" This reverts commit 0ad97fd.

emeraldbay pushed a commit to emeraldbay/sagemaker-training-toolkit that referenced this pull request Oct 13, 2023

Revert "feat: support codeartifact for installing requirements.txt pa…

5af6d41

…ckages (aws#187)" This reverts commit 0ad97fd.

emeraldbay pushed a commit to emeraldbay/sagemaker-training-toolkit that referenced this pull request Oct 14, 2023

Revert "feat: support codeartifact for installing requirements.txt pa…

136a510

…ckages (aws#187)" This reverts commit 0ad97fd.

		assert "Reporting training SUCCESS" in stdout


		# def test_install_requirements_from_codeartifact(capsys):

feat: support codeartifact for installing requirements.txt packages #187

feat: support codeartifact for installing requirements.txt packages #187

Uh oh!

Conversation

humanzz commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

General

Tests

Uh oh!

humanzz commented Jul 10, 2023

Uh oh!

humanzz commented Jul 10, 2023

Uh oh!

humanzz commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humanzz commented Jul 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humanzz commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emeraldbay left a comment

Choose a reason for hiding this comment

Uh oh!

emeraldbay left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

humanzz commented Jul 10, 2023 •

edited

Loading

humanzz commented Jul 11, 2023 •

edited

Loading

humanzz commented Jul 24, 2023 •

edited

Loading