fix: Improve PythonAsyncEngine error handling and Increase Tokio thread count

[ryanolson]

What does the PR do?

• Improved error handling in the PythonAsyncEngine
• Update the default Tokio Runtime to 16 async threads and 16 offload threads.

The default single threaded Tokio runtime was being starved, likely due to GIL contention in the python soak.py test.

When we grab the GIL, we are actually blocking our rust tokio async threads. We might consider scheduling GIL calls ot the offload threads meant for blocking calls.

We might also consider using separate single threaded tokio runtime to handle all task that will touch the GIL.

A third, and likely best option is to use static tokio mutex handle which need to be acquired before accessing the GIL, thus put a yielding tokio lock around the GIL.

The multi-threaded RT is sufficient now to unblock the soak test, but long-term strategies on GIL handling need to be considered.

Checklist

• PR title reflects the change and is of format <commit_type>: <Title>
• Changes are described in the pull request.
• Related issues are referenced.
• Populated github labels field
• Added test plan and verified test passes.
• Verified that the PR passes existing CI.
• Verified copyright is correct on all changed files.
• Added succinct git squash message before merging ref.
• All template sections are filled out.
• Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

• build
• ci
• docs
• feat
• fix
• perf
• refactor
• revert
• style
• test

Related PRs:

Where should the reviewer start?

If the channel is closed, the send errors. We were not checking the return status and unwrapping them.

Test plan:

• CI Pipeline ID:

Caveats:

Background

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

• closes GitHub issue: #xxx

[rmccorm4]

Few notes/questions:

1. Should these be env vars similar to the from_settings support for configurability? ex:

TRITON_RUNTIME_NUM_WORKER_THREADS=16
TRITON_RUNTIME_MAX_BLOCKING_THREADS=16

2. It looks like there is already a native tokio env var for worker threads: TOKIO_WORKER_THREADS - but I'm fine with defining our own for consistency with other config settings

3. From the docs - it looks like the default behavior, if left unset, is to use the num cpu cores rather than trying to apply a fixed number to various hardwares and scenarios. Do we want to define a fixed number here?

The default value is the number of cores available to the system.

4. Docs seem to say that worker_threads will spawn/use that many threads - so I think it's more aptly named "num_worker_threads" than "max_worker_threads" (which implies it may not spawn/use them all). For max_blocking_threads, I think that name fits well based on the docs:

Unlike the worker_threads, they are not always active and will exit if left idle for too long. You can change this timeout duration with thread_keep_alive.

[nnshah1]

let's do make these configurable -

[ryanolson]

The TRITON_RUNTIME_ envs are the correct envs. We can also use TOML files in specific paths to override compiled defaults.

[ryanolson]

we could rename NUM_WORKER_THREADS to NUM_ASYNC_THREADS

[rmccorm4]

Added #146 as a good intro task for someone to pick up - but feel free to commit it if you already have it done

[nnshah1]

@rmccorm4 - LGTM - good with your review -

[nnshah1]

can we run this via pytest - maybe nightly?

[rmccorm4]

I think so - we can test-ify and add the pytest.mark.nightly marker here - but I think the CI needs an update to run a nightly scheduled job @nv-anants

• DLIS-7968
• DLIS-7969

[ryanolson]

i marked the rust soak.rs test as a feature, so by default it will not run.

cargo test --features integration

we'll want a mark for test that depend on etcd/nats

[rmccorm4]

Added #150

[nnshah1]

LGTM- defer to @rmccorm4 for final review

[rmccorm4]

is it possible to match none of the cases here? if so, should we have a default case here? Or it's guaranteed to be a ResponseProcessingError and will compile-time fail if new errors are added but no corresponding match case?

[ryanolson]

the latter. its good practice not to have a default, because the compiler will enforce that all possible arms of the match statement be defined.

if you add another arm in the enum, you will see a compiler error on the match, or any where in the code base this is used. in this case the enum is private, so it's static to the file for now.