feat: improve failure status reason #117

ajberdy · 2023-10-24T00:39:15Z

Issue #, if available:

Description of changes:

Log specific exception from customer method to failure status

Testing done:

Manual tests and unit test added, braket and sagemaker integ tests passing for base

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 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)

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

ajberdy · 2023-10-24T02:47:46Z

~~Pending test updates~~

done

krneta · 2023-10-24T17:26:31Z

src/braket_container.py

+            try:
+                customer_method(**kwargs)
+            except Exception as exc:
+                queue.put(exc)


Do you need to re-throw the exception here so that the customer execution script will end?

Why not just write to the output file and exit here?

This is code that runs inside the process, but we want to "log failure and exit" from the main container process. Putting the exception in the queue lets us transfer the information from the customer method process to the main container process. Logging the failure may work properly from within the child process, but calling sys.exit won't impact the parent process like we want.

Our current codepath

Child process start

Child process raises an exception

Child process joins (immediately after exception occurs)

Check if child process exited with code 0

If not, raise a generic exception

New codepath

Child process starts

Child process raises an exception

Exception is captured and put in the queue

Child process joins (immediately after exception occurs)

Check if there is an exception in the queue

If so, raise a specific exception

Do you need to re-throw the exception here so that the customer execution script will end?

No, since there are no more lines of code in wrapped_customer_method. We may want to rethrow the exception for the sake of logging; I'll check what the impact is here

ajberdy · 2023-10-24T20:23:25Z

New changes will need testing updates

krneta · 2023-10-24T20:38:00Z

src/braket_container.py

-        log_failure_and_exit(f"Unable to run job at entry point {entry_point}\nException: {e}")
+        log_failure_and_exit(f"Job did not exit gracefully.\nException: {e}")
+
+    customer_code_process.join()


Are we doing .join() twice?

src/braket_container.py

krneta · 2023-10-24T20:41:39Z

src/braket_container.py

+        customer_module = importlib.import_module(str_module)
+        customer_method = getattr(customer_module, str_method)
+    else:
+        def customer_method():


This seems to be misnamed

Sure, I can update the name to be "customer_executable"

src/braket_container.py

krneta · 2023-10-24T20:48:40Z

test/unit_tests/test_braket_container.py

-    mock_subprocess.run.assert_called_with(
-        ["python", "-m", "test_entry_point"], cwd='/opt/braket/code/customer_code/extracted',
-    )
+    # mock_subprocess.run.assert_called_with(


cleanup here and below?

src/braket_container.py

krneta · 2023-10-24T20:52:01Z

src/braket_container.py

+    customer_method = extract_customer_method(entry_point)
+    customer_method_process = kick_off_customer_script(customer_method)
+    join_customer_script(customer_method_process)
+    sys.exit(customer_method_process.exitcode)


I'm not crazy about exiting with 0 here, since we may want to add additional functionality after this method. I would check if it's non-zero (as we were doing before), and then exit with 0 (or SM won't propagate the error correctly). Or we should check if they've fixed that bug.

Yup, still see a blank sm error message with a non-zero exit code. Fixed to return zero on failure as before

Tbf, there's no sagemaker error when exiting with zero anyways. The braket failure reason does get propagated in both cases, just a difference in the logs. I'd argue perhaps SM_ERROR="" is better than Reporting training SUCCESS, so maybe we want to use the correct exit code?

ajberdy · 2023-10-25T00:46:31Z

Ran manual tests successfully on the latest commit

krneta · 2023-10-25T23:52:26Z

src/braket_container.py

    print("Code Run Finished")
-    return_code = result.returncode
-    return return_code
+    return customer_code_process.exitcode


You're actually not using this returned value.

feat: improve failure status reason

be95c0b

ajberdy requested review from kshitijc and krneta October 24, 2023 00:39

ajberdy marked this pull request as draft October 24, 2023 02:47

add unit test

1e97b41

ajberdy marked this pull request as ready for review October 24, 2023 03:45

Merge branch 'main' into log-tb

a195bb2

krneta reviewed Oct 24, 2023

View reviewed changes

feat: run all entry points as a process

a6a1f5c

krneta reviewed Oct 24, 2023

View reviewed changes

ajberdy added 6 commits October 24, 2023 16:39

update tests

1c84f1e

set exit code as 0

6be9616

terminate when non-zero code

890183e

updates tests wrt status codes

1ac2874

method to code

b967e82

remove comments

a66e8a1

set exit code to 1 on failure

b3df4aa

ajberdy requested a review from krneta October 25, 2023 18:15

ajberdy added 2 commits October 25, 2023 16:29

use customer exit code and check str(e) instead of args

7f5b301

update tests

f1a0a5a

krneta reviewed Oct 25, 2023

View reviewed changes

krneta previously approved these changes Oct 25, 2023

View reviewed changes

use exit code from join

30cda77

ajberdy dismissed krneta’s stale review via 30cda77 October 26, 2023 00:04

ajberdy requested a review from krneta October 26, 2023 00:04

krneta approved these changes Oct 26, 2023

View reviewed changes

ajberdy merged commit 58e289b into main Oct 26, 2023
6 checks passed

ajberdy deleted the log-tb branch October 26, 2023 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve failure status reason #117

feat: improve failure status reason #117

ajberdy commented Oct 24, 2023 •

edited

Loading

ajberdy commented Oct 24, 2023 •

edited

Loading

krneta Oct 24, 2023

krneta Oct 24, 2023

ajberdy Oct 24, 2023

ajberdy Oct 24, 2023

ajberdy commented Oct 24, 2023

krneta Oct 24, 2023

ajberdy Oct 25, 2023

krneta Oct 24, 2023

ajberdy Oct 25, 2023

krneta Oct 24, 2023

krneta Oct 24, 2023

ajberdy Oct 25, 2023

ajberdy Oct 25, 2023

ajberdy commented Oct 25, 2023

krneta Oct 25, 2023

ajberdy Oct 26, 2023

feat: improve failure status reason #117

feat: improve failure status reason #117

Conversation

ajberdy commented Oct 24, 2023 • edited Loading

Merge Checklist

General

Tests

ajberdy commented Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajberdy commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajberdy commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajberdy commented Oct 24, 2023 •

edited

Loading

ajberdy commented Oct 24, 2023 •

edited

Loading