ameya-parab:feature/sagemaker_proxies by ameya-parab · Pull Request #6120 · mlflow/mlflow

ameya-parab · 2022-06-23T17:03:49Z

Related Issues/PRs

Close #6119

What changes are proposed in this pull request?

Injecting proxies during docker build mlflow sagemaker build-and-push-container
Injecting proxies as environment variables inside the deployed Sagemaker endpoint.

How is this patch tested?

I have written tests (not required for typo or doc fix) and confirmed the proposed feature/bug-fix/change works.

Does this PR change the documentation?

No. You can skip the rest of this section.
Yes. Make sure the changed pages / sections render correctly by following the steps below.

Check the status of the ci/circleci: build_doc check. If it's successful, proceed to the
next step, otherwise fix it.
Click Details on the right to open the job page of CircleCI.
Click the Artifacts tab.
Click docs/build/html/index.html.
Find the changed pages / sections and make sure they render correctly.

Release Notes

Is this a user-facing change?

No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release notes for MLflow users.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

What component(s), interfaces, languages, and integrations does this PR affect?

Components

Interface

area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
area/windows: Windows support

Language

language/r: R APIs and clients
language/java: Java APIs and clients
language/new: Proposals for new client languages

Integrations

integrations/azure: Azure and Azure ML integrations
integrations/sagemaker: SageMaker integrations
integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
rn/feature - A new user-facing feature worth mentioning in the release notes
rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
rn/documentation - A user-facing documentation change worth mentioning in the release notes

Signed-off-by: Ameya Parab <[email protected]>

mlflow/models/docker_utils.py

WeichenXu123 · 2022-06-24T04:13:45Z

mlflow/models/docker_utils.py

+        "-Dhttps.proxyPort={https_proxy_port} -Dhttps.nonProxyHosts=repo.maven.apache.org"
+    ).format(
+        http_proxy_host=os.environ["http_proxy"].split("//")[1].split(":")[0],
+        http_proxy_port=os.environ["http_proxy"].rsplit(":", maxsplit=1)[1],


why not use http_proxy_port=os.environ["http_proxy"].split("//")[1].split(":")[1] ? any difference ?

@WeichenXu123 maybe it's because http_proxy_port=os.environ["http_proxy"].rsplit(":", maxsplit=1)[1] is simpler, a bit faster, and eaiser to understand.

@harupy and @WeichenXu123 thanks for your comments, my intention for using the split approach was to forcefully fail the code, if the proxy is not adhering to the standard format http://HOST:PORT, but I'll update the code (and add comments) to make use of `urllib.parse.urlparse' and update the PR.

WeichenXu123 · 2022-06-24T04:15:01Z

Could you show your testing result ? e.g. screen records :)

mlflow/models/docker_utils.py

mlflow/sagemaker/__init__.py

Signed-off-by: Ameya Parab <[email protected]>

ameya-parab · 2022-06-28T19:33:20Z

#6120 (comment)

The unit test results are attached below, after executing the /dev/run-python-sagemaker-tests.sh script.

dbczumar · 2022-06-29T22:56:26Z

@WeichenXu123 Can you take another look at this?

WeichenXu123 · 2022-06-30T12:59:50Z

tests/sagemaker/test_sagemaker_deployment_client.py

+    if os.getenv("http_proxy") is not None:
+        expected_model_environment.update({"http_proxy": os.environ["http_proxy"]})
+
+    if os.getenv("https_proxy") is not None:
+        expected_model_environment.update({"https_proxy": os.environ["https_proxy"]})
+
+    if os.getenv("no_proxy") is not None:
+        expected_model_environment.update({"no_proxy": os.environ["no_proxy"]})


In order to cover the case http(s)_proxy setting existing, we should patch the os.environ to make it contains "http(s)_proxy" env ?

Could you please elaborate on the ask here? How would you suggest handling the case wherein we mock proxy variables, should I duplicate each of the test case (9 in total) after mocking the proxies, or would you recommend a different approach?

I think you can use mock.patch.dict(os.environ, {...}) and add http(s)_proxy envs to patched os.environ

Sounds good, and would you suggest duplicating each of the 9 unit test cases, one without the proxies and one with mocked proxies?

duplicating each of the 9 unit test cases

Don't need to duplicate them, but I would suggest use @pytest.mark.parametrize on these tests.

Perfect, will work on that!

Updated PR, the unit tests now include parametrized tests to validate scenarios with and without proxies.

WeichenXu123 · 2022-06-30T13:00:21Z

mlflow/models/docker_utils.py

+    assert _http_proxy.hostname is not None, "Invalid `http_proxy` hostname."
+    assert isinstance(_http_proxy.port, int), f"Invalid Proxy Port: {_http_proxy.port}"
+
+    _https_proxy = urlparse(os.environ["https_proxy"])


nit: _https_proxy ==> parsed_https_proxy

Makes sense, will rename the variables

Renamed the variables as suggested above.

WeichenXu123 · 2022-06-30T13:05:28Z

mlflow/models/docker_utils.py

+        https_proxy_port=_https_proxy.port,
+    )
+else:
+    MAVEN_PROXY = ""  # No Proxy


Shall we add test for MAVEN_PROXY ?

I have added asserts for validating the proxy values, can you please elaborate as to what additional scenarios I need to test when it comes to MAVEN_PROXY. Thanks!

If hard to add test for MAVEN_PROXY, we can manually test it.

Sure, I can implement a manual test for MAVEN PROXY; however, as this is my first PR for mlflow, could you please advise on how to set up manual tests when contributing to mlflow?

Signed-off-by: Ameya Parab <[email protected]>

WeichenXu123 · 2022-07-01T00:34:18Z

mlflow/models/docker_utils.py

+    MAVEN_PROXY = (
+        " -DproxySet=true -Dhttp.proxyHost={http_proxy_host} "
+        "-Dhttp.proxyPort={http_proxy_port} -Dhttps.proxyHost={https_proxy_host} "
+        "-Dhttps.proxyPort={https_proxy_port} -Dhttps.nonProxyHosts=repo.maven.apache.org"


If the http(s)_proxy contains username/password, shall set them in maven proxy config ?

I'm not that well versed with maven, but I believe we can pass username and password as

MAVEN_PROXY += " -Dhttp.proxyUser={proxy_username} -Dhttp.proxyPassword={proxy_password}"

Could you please confirm, thanks!
My current organization's development setup does not require proxy authentication, making it difficult for me to test this scenario.

I don't have env at hand, but I think this is correct.
-Dhttp.proxyUser={proxy_username} -Dhttp.proxyPassword={proxy_password}

Ref: https://www.baeldung.com/maven-behind-proxy

Implemented the changes in the latest PR commit and have also modified the unit tests to validate authenticated proxy servers.

Signed-off-by: Ameya Parab <[email protected]>

WeichenXu123 · 2022-07-02T03:54:05Z

tests/sagemaker/test_deployment.py

        proxy_variables = {
            "http_proxy": "http://proxy.example.net:1234",
            "https_proxy": "https://proxy.example.net:1234",
-            "no_proxy": "localhost",


Let's comment this is aws IP and change it as a global variable definition.

Sure, so defining the AWS_IP as a variable AWS_IP = "169.254.169.254" over here https://github.com/mlflow/mlflow/blob/master/tests/helper_functions.py#L32 and then importing it over in tests/sagemaker/test_deployment.py, should be okay? Or did you have anything else in mind?

sounds good.

Can we replace 169.254.169.254 with a hostname ? And explain why we need to add no-proxy for it here ?

It appears the IP 169.254.169.254 does not have a hostname but needs to be specified in no_proxy for interacting with AWS services.

Ref: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-proxy.html#cli-configure-proxy-ec2

WeichenXu123

LGTM

WeichenXu123 · 2022-07-04T14:11:05Z

Some sagemake test failed. Could you fix it ?

Signed-off-by: Ameya Parab <[email protected]>

ameya-parab · 2022-07-05T17:42:56Z

#6120 (comment) Some sagemake test failed. Could you fix it ?

I modified the failing Sagemaker tests to remove only the 'proxy' variables while leaving the other environment variables alone. All of the Sagemaker tests passed in my local setup but failed in the Github runner, most likely due to the different environment setup.

* injecting proxies for maven calls Signed-off-by: Ameya Parab <[email protected]> * add proxies (if available) into SageMaker endpoint Signed-off-by: Ameya Parab <[email protected]> * towards sagemaker proxies fix Signed-off-by: Ameya Parab <[email protected]> * parsing proxies with urlparse Signed-off-by: Ameya Parab <[email protected]> * renamed 'proxy' variables Signed-off-by: Ameya Parab <[email protected]> * testing 'proxy' and 'no proxy' scenarios Signed-off-by: Ameya Parab <[email protected]> * excluding AWS IP in proxy Signed-off-by: Ameya Parab <[email protected]> * including proxy user and password if present Signed-off-by: Ameya Parab <[email protected]> * added AWS_METADATA_IP for use in no proxy Signed-off-by: Ameya Parab <[email protected]> * testing auth proxies for sagemaker deployments Signed-off-by: Ameya Parab <[email protected]> * auth proxies for deployments / sagemaker tests fix Signed-off-by: Ameya Parab <[email protected]> Co-authored-by: Ameya Parab <[email protected]>

* injecting proxies for maven calls Signed-off-by: Ameya Parab <[email protected]> * add proxies (if available) into SageMaker endpoint Signed-off-by: Ameya Parab <[email protected]> * towards sagemaker proxies fix Signed-off-by: Ameya Parab <[email protected]> * parsing proxies with urlparse Signed-off-by: Ameya Parab <[email protected]> * renamed 'proxy' variables Signed-off-by: Ameya Parab <[email protected]> * testing 'proxy' and 'no proxy' scenarios Signed-off-by: Ameya Parab <[email protected]> * excluding AWS IP in proxy Signed-off-by: Ameya Parab <[email protected]> * including proxy user and password if present Signed-off-by: Ameya Parab <[email protected]> * added AWS_METADATA_IP for use in no proxy Signed-off-by: Ameya Parab <[email protected]> * testing auth proxies for sagemaker deployments Signed-off-by: Ameya Parab <[email protected]> * auth proxies for deployments / sagemaker tests fix Signed-off-by: Ameya Parab <[email protected]> Co-authored-by: Ameya Parab <[email protected]> Signed-off-by: Michal Karzynski <[email protected]>

Ameya Parab added 2 commits June 15, 2022 10:59

injecting proxies for maven calls

87a53a4

Signed-off-by: Ameya Parab <[email protected]>

add proxies (if available) into SageMaker endpoint

5d60394

Signed-off-by: Ameya Parab <[email protected]>

ameya-parab marked this pull request as ready for review June 23, 2022 17:06

WeichenXu123 reviewed Jun 24, 2022

View reviewed changes

mlflow/models/docker_utils.py Outdated Show resolved Hide resolved

WeichenXu123 reviewed Jun 24, 2022

View reviewed changes

harupy reviewed Jun 24, 2022

View reviewed changes

mlflow/models/docker_utils.py Outdated Show resolved Hide resolved

ameya-parab commented Jun 24, 2022

View reviewed changes

mlflow/sagemaker/__init__.py Show resolved Hide resolved

Ameya Parab added 2 commits June 28, 2022 15:23

towards sagemaker proxies fix

cba40b5

Signed-off-by: Ameya Parab <[email protected]>

parsing proxies with urlparse

0f6a451

Signed-off-by: Ameya Parab <[email protected]>

ameya-parab requested review from WeichenXu123 and harupy June 28, 2022 19:33

WeichenXu123 reviewed Jun 30, 2022

View reviewed changes

Ameya Parab added 2 commits June 30, 2022 18:47

renamed 'proxy' variables

63c7598

Signed-off-by: Ameya Parab <[email protected]>

testing 'proxy' and 'no proxy' scenarios

da4cb41

Signed-off-by: Ameya Parab <[email protected]>

ameya-parab requested a review from WeichenXu123 June 30, 2022 22:55

WeichenXu123 reviewed Jul 1, 2022

View reviewed changes

excluding AWS IP in proxy

5c2d425

Signed-off-by: Ameya Parab <[email protected]>

ameya-parab requested a review from WeichenXu123 July 1, 2022 18:59

WeichenXu123 reviewed Jul 2, 2022

View reviewed changes

WeichenXu123 approved these changes Jul 2, 2022

View reviewed changes

Ameya Parab added 4 commits July 5, 2022 13:18

including proxy user and password if present

cd5d066

Signed-off-by: Ameya Parab <[email protected]>

added AWS_METADATA_IP for use in no proxy

06fa805

Signed-off-by: Ameya Parab <[email protected]>

testing auth proxies for sagemaker deployments

e03c11f

Signed-off-by: Ameya Parab <[email protected]>

auth proxies for deployments / sagemaker tests fix

6712e47

Signed-off-by: Ameya Parab <[email protected]>

ameya-parab requested a review from WeichenXu123 July 5, 2022 17:43

WeichenXu123 approved these changes Jul 6, 2022

View reviewed changes

WeichenXu123 merged commit 698574e into mlflow:master Jul 6, 2022

ameya-parab deleted the feature/sagemaker_proxies branch July 21, 2022 17:32

harupy mentioned this pull request Aug 15, 2022

Avoid using assertions at top level in mlflow/models/docker_utils.py #6463

Merged

31 tasks

Conversation

ameya-parab commented Jun 23, 2022

Related Issues/PRs

What changes are proposed in this pull request?

How is this patch tested?

Does this PR change the documentation?

Release Notes

Is this a user-facing change?

What component(s), interfaces, languages, and integrations does this PR affect?

How should the PR be classified in the release notes? Choose one:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 commented Jun 24, 2022

Uh oh!

Uh oh!

Uh oh!

ameya-parab commented Jun 28, 2022

Uh oh!

dbczumar commented Jun 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ameya-parab Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

ameya-parab Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

harupy Jun 24, 2022 •

edited

Loading

ameya-parab Jun 30, 2022 •

edited

Loading

WeichenXu123 Jun 30, 2022 •

edited

Loading

ameya-parab Jul 1, 2022 •

edited

Loading