fix: localmode subprocess parent process not sending SIGTERM to child #2613

ranzvi · 2021-09-01T11:41:46Z

Issue #, if available:

Description of changes:

Due to an unknown issue, on some machines executing predictor.delete_endpoint() simply hangs.
This is because the SIGTERM sent to the subprocess doesn't propagate to the child process.

E.g pid 7521 doesn't propagate SIGTERM to child 7522.

This means that self.container.join() is waiting for a container which hasn't received a signal to stop execution and hangs indefinitely.

By sending SIGTERM to all child processes of the subprocess.Popen process, we can guarantee that the container will be Gracefully stopped...

Testing done:

Executed all unit and integration tests
Verified behaviour remains the same on machines without the bug
Verified bug fixed on faulty machines by executing multiple endpoints sequentially
Testing done:

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 passed the region in to all S3 and STS clients that I've initialized as part of this change.
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

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)
I have used 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.

sagemaker-bot · 2021-09-01T11:49:09Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 193c23e
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-09-01T11:49:09Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 268357f
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-09-01T11:49:59Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 5e33b4f
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-09-01T11:50:23Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 23ef42e
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

ranzvi · 2021-09-01T12:04:57Z

@ahsan-z-khan This PR doesn't contain and new dependencies

sagemaker-bot · 2021-09-01T12:07:44Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: c3708bd
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-09-01T12:09:38Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 7b07b90
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-09-19T07:05:09Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 37eeb5e
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

gilinachum · 2021-10-06T08:08:22Z

@shreyapandit Hi! can you have a look if the failing tests are relevant to the PR? And assign someone to review it?

ranzvi · 2021-10-28T09:58:39Z

@gilinachum any updates?

gilinachum · 2021-10-28T16:59:19Z

@gilinachum any updates?

Sorry about that. Checking again :\

sagemaker-bot · 2021-10-28T20:50:15Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: b6cb578
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

BasilBeirouti · 2021-10-28T21:16:50Z

src/sagemaker/local/image.py

@@ -841,6 +840,8 @@ def run(self):

    def down(self):
        """Placeholder docstring"""
+        if os.name != "nt":


what is this check for?

It's checking if the os is a windows machine, since I'm using unix commands to kill the processes

sagemaker-bot · 2021-10-28T21:42:35Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: b6cb578
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-10-28T21:56:32Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: b6cb578
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

shreyapandit · 2021-10-30T01:55:53Z

Hi @ranzvi, thanks for your contribution!

Please follow the one time setup for enabling git hooks, and this should allow you to reproduce the errors, and fix the tests cc @ranzvi @gilinachum. The unit tests are failing on linting issues.
Here is the setup can you do. Please change brew install to whatever is compatible with your OS.

# Install using brew:
brew install pyenv pyenv-virtualenv pyenv-virtualenvwrapper

# Make sure to add the pyenv and virtualenv init functions to your corresponding shell init (.bashrc, .zshrc, etc):
eval "*$(*pyenv init --path*)*"      # old usage was: $ eval "$(pyenv init -)"
eval "*$(*pyenv virtualenv-init -*)*"

# Start a new shell once you do that to pick up your changes.
# Install following Python versions:
pyenv install 3.8.8
pyenv install 3.7.10
pyenv install 3.6.13

# Set them as global versions:
pyenv global 3.8.8
pyenv global 3.7.10
pyenv global 3.6.13

# Verify they show up when you do:
pyenv versions

# Restart your shell and run the command again to verify that it persists across shell sessions.

# Install tox to run our tests:
pip install tox

# Set all versions as global:
pyenv global $(pyenv versions --bare)
pyenv rehash

# Setup GitHub Pre Push hooks
# To enable all git hooks in the .githooks directory.
# Run these commands in the GitHub repository directory:

find .git/hooks -type l -exec rm {} \;
find .githooks -type f -exec ln -sf ../../{} .git/hooks/ \;```

shreyapandit

As outlined in comments

…zvi/sagemaker-python-sdk into fix-localmode-subprocess-termination

sagemaker-bot · 2021-11-01T09:33:35Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: dd7f66c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-11-01T09:35:24Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 4166ceb
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-11-01T09:50:09Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: a2ddd4c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-11-01T10:15:17Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: 4166ceb
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-11-01T10:15:17Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: a2ddd4c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-11-01T10:18:31Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: dd7f66c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-11-01T10:23:18Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: dd7f66c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-11-01T10:26:08Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: a2ddd4c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2021-11-01T10:26:09Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 4166ceb
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

ranzvi · 2021-11-01T10:44:45Z

@shreyapandit Thanks for the insights. I fixed the linting issues and executed the unittest locally and they pass.

Do you have any idea why the rest of the ci is still failing?

navinsoni · 2021-11-01T16:34:27Z

@ranzvi I am restarting your tests

shreyapandit

LGTM! Thanks for your contribution

jeniyat

looks good to me.

…aws#2613) Co-authored-by: Shreya Pandit <[email protected]> Co-authored-by: Jeniya Tabassum <[email protected]>

wcarpenter1-godaddy · 2022-07-20T16:22:50Z

@ranzvi did this fix your issue? Interestingly, this new code introduces an error log for us during delete_endpoint:

Gracefully stopping... (press Ctrl+C again to force)
Exception in thread Thread-1:
Traceback (most recent call last):
  File "/Users/wcarpenter1/Library/Caches/pypoetry/virtualenvs/cerbo-model-bynQLcpi-py3.9/lib/python3.9/site-packages/sagemaker/local/image.py", line 852, in run
    _stream_output(self.process)
  File "/Users/wcarpenter1/Library/Caches/pypoetry/virtualenvs/cerbo-model-bynQLcpi-py3.9/lib/python3.9/site-packages/sagemaker/local/image.py", line 914, in _stream_output
    raise RuntimeError("Process exited with code: %s" % exit_code)
RuntimeError: Process exited with code: 2

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/wcarpenter1/.pyenv/versions/3.9.11/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/threading.py", line 973, in _bootstrap_inner
    self.run()
  File "/Users/wcarpenter1/Library/Caches/pypoetry/virtualenvs/cerbo-model-bynQLcpi-py3.9/lib/python3.9/site-packages/sagemaker/local/image.py", line 857, in run
    raise RuntimeError(msg)
RuntimeError: Failed to run: ['docker-compose', '-f', '/private/var/folders/90/2dm4q2zj72ncdgmgvmgykjy00000gp/T/tmpn531lijb/docker-compose.yaml', 'up', '--build', '--abort-on-container-exit'], Process exited with code: 2

Commenting out these lines fixes our issue.

For us, self.process.terminate(), successfully kills both the process and it's children. So kill_child_processes is not needed. But not only is it not needed, it is causing the process to exit return 2, which generates the RuntimeError above.

I'd be interesting in potentially rolling back these changes if that was OK with you.

ranzvi · 2022-07-20T16:42:25Z

@wcarpenter1-godaddy This did fix our issue, we had a problem with the self.process.terminate() which is why I added kill_child_processes. Could you investigate where this error is originating and why?

wcarpenter1-godaddy · 2022-07-22T15:57:53Z

@ranzvi what os are you on?

ranzvi · 2022-07-22T16:21:18Z

@wcarpenter1-godaddy MacOS, but this works on ubuntu as well. the code has a safeguard for windows

ranzvi closed this Sep 1, 2021

ranzvi force-pushed the fix-localmode-subprocess-termination branch from 268357f to 7b07b90 Compare September 1, 2021 11:58

fix: localmode subprocess parent process not sending SIGTERM to child

c3708bd

ranzvi reopened this Sep 1, 2021

ranzvi mentioned this pull request Sep 1, 2021

fix: localmode subprocess parent process not sending SIGTERM to child #2572

Merged

7 tasks

ranzvi and others added 3 commits September 2, 2021 13:14

Merge branch 'master' into fix-localmode-subprocess-termination

9fc0b44

Merge branch 'master' into fix-localmode-subprocess-termination

a50ce10

Merge branch 'master' into fix-localmode-subprocess-termination

37eeb5e

ranzvi and others added 2 commits October 3, 2021 10:53

Merge branch 'master' into fix-localmode-subprocess-termination

7b89cde

fix: linting

8950715

Merge branch 'master' into fix-localmode-subprocess-termination

a5a06dd

Merge branch 'master' into fix-localmode-subprocess-termination

b6cb578

BasilBeirouti previously approved these changes Oct 28, 2021

View reviewed changes

BasilBeirouti reviewed Oct 28, 2021

View reviewed changes

shreyapandit suggested changes Oct 30, 2021

View reviewed changes

ranzvi added 2 commits November 1, 2021 11:26

fix: pylint errors

177df84

Merge branch 'fix-localmode-subprocess-termination' of github.com:ran…

dd7f66c

…zvi/sagemaker-python-sdk into fix-localmode-subprocess-termination

ranzvi dismissed BasilBeirouti’s stale review via dd7f66c November 1, 2021 09:26

Merge branch 'master' into fix-localmode-subprocess-termination

4166ceb

fix: pylint errors

a2ddd4c

shreyapandit approved these changes Nov 1, 2021

View reviewed changes

jeniyat approved these changes Nov 1, 2021

View reviewed changes

jeniyat merged commit c4075bb into aws:master Nov 1, 2021

mufaddal-rohawala mentioned this pull request Nov 1, 2021

feature: Add timeout to wait_for_endpoint and _wait_until. #2684

Closed

9 tasks

fix: localmode subprocess parent process not sending SIGTERM to child #2613

fix: localmode subprocess parent process not sending SIGTERM to child #2613

Conversation

ranzvi commented Sep 1, 2021 • edited Loading

Merge Checklist

General

Tests

sagemaker-bot commented Sep 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 1, 2021

AWS CodeBuild CI Report

ranzvi commented Sep 1, 2021

sagemaker-bot commented Sep 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 19, 2021

AWS CodeBuild CI Report

gilinachum commented Oct 6, 2021

ranzvi commented Oct 28, 2021

gilinachum commented Oct 28, 2021

sagemaker-bot commented Oct 28, 2021

AWS CodeBuild CI Report

BasilBeirouti Oct 28, 2021

Choose a reason for hiding this comment

ranzvi Oct 31, 2021

Choose a reason for hiding this comment

sagemaker-bot commented Oct 28, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Oct 28, 2021

AWS CodeBuild CI Report

shreyapandit commented Oct 30, 2021 • edited Loading

shreyapandit left a comment

Choose a reason for hiding this comment

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Nov 1, 2021

AWS CodeBuild CI Report

ranzvi commented Nov 1, 2021

navinsoni commented Nov 1, 2021

shreyapandit left a comment

Choose a reason for hiding this comment

jeniyat left a comment

Choose a reason for hiding this comment

wcarpenter1-godaddy commented Jul 20, 2022

ranzvi commented Jul 20, 2022

wcarpenter1-godaddy commented Jul 22, 2022

ranzvi commented Jul 22, 2022 • edited Loading

ranzvi commented Sep 1, 2021 •

edited

Loading

shreyapandit commented Oct 30, 2021 •

edited

Loading

ranzvi commented Jul 22, 2022 •

edited

Loading