fix: catch SystemExit and error out [DET-2956]

[shiyuann]

Description

Catch SystemExit and error out.

Test Plan

N/A

Commentary (optional)

N/A

[rb-determined-ai]

This seems like the hardest way to do this change, because we are trying to mark every single function that involves user code.

In reality, we are writing a library and we should never be calling sys.exit() except around entrypoint when we are e.g. checking command line arguments.

So maybe we can just change all of the non-entrypoint sys.exit()'s that we have laying around (it's just a few) with AssertionErrors or something, and just wrap the following 3 functions calls with checks for sys.exit():

• build_and_run_training_pipeline in determined/exec/harness.py
• main() in determined/exec/worker_process.py (you'll have to move the argv check out of the main() function)
• local_experiment in determined_cli/experiment.py

I think that would be guaranteed to catch all of the sys.exit calls that a user could possibly make, and we only have to have the check in 3 different places.

[rb-determined-ai]

@shiyuann you have not responded to my high-level points from the previous review

[shiyuann]

@rb-determined-ai Sorry, I thought I had already responded.

There are more than just three places. We also need to catch SystemExit in any places that use the _local_execution_manager(). Using this way means we are catching the same error in different layers of the system, both on the harness execution layer and Native API execution layer, which sounds messy to me. It's more about making the system have a good division of layers than how many lines of code you could save.

[shiyuann]

@rb-determined-ai Never mind. I can put it on the harness execution layer.

[rb-determined-ai]

Thank you, this is way cleaner.

Do you know, will we hit issues with our own sys.exit() in det._native._init_cluster_mode()?

There are also additional sys.exit() calls to remove in:

• _estimator_trial.py
• gpu.py
• ipc.py

[shiyuann]

Do you know, will we hit issues with our own sys.exit() in det._native._init_cluster_mode()?

If it's inside cluster, we catch it on the harness execution layer. If it's submitting to a cluster, it doesn't catch the SystemExit. So it would not be an issue.

I don't really like putting it in the harness/Native execution layer just to save some lines of code since it really takes some mental effort to understand why the codes of catching SystemExit are there and why _init_cluster_mode doesn't have that.

There are also additional sys.exit() calls to remove in:
_estimator_trial.py
gpu.py
ipc.py

I fixed the gpu.py. Which sys.exit do you mean in _estimator_trial.py and ipc.py? I haven't seen any code that uses it.

[rb-determined-ai]

lol ipc.py has two calls to exit(1) which are not even valid python code.

the _estimator_trial.py also has a bare exit(1) which is not valid but which will never ever be hit, since it's the line after an AssertionError.

[rb-determined-ai]

This looks really good, but please do address the stray exit() calls I mentioned before merging.

[shiyuann]

@rb-determined-ai addressed

[rb-determined-ai]

ship it! thanks for fixing these issues

[rb-determined-ai]

@clabot check

[rb-determined-ai]

@cla-bot check

[rb-determined-ai]

@cla-bot[bot] check

[rb-determined-ai]

@cla-bot[bot] check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!