Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Better Thread Pool integration #61

Closed
dbwiddis opened this issue Sep 28, 2023 · 3 comments · Fixed by #374
Closed

[FEATURE] Better Thread Pool integration #61

dbwiddis opened this issue Sep 28, 2023 · 3 comments · Fixed by #374
Assignees
Labels
enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Sep 28, 2023

Is your feature request related to a problem?

#47 implemented basic use of an ExecutorService to give better control of the async process execution.
#66 improved on #47 by using the actual thread pool but is still just using the generic name.

We can implement more custom thread pool behavior/prioritization similar to how other plugins have done so, moving beyond the generic thread pool.

What solution would you like?

Probably carefully choose the correct existing thread pool for the threads you're creating.

Possibly a custom thread pool integration properly prioritizing this plugin's threads. Custom thread pools override getExecutorBuilders(), see examples here and here

Do you have any additional context?

This is an admittedly vague feature because it's an optional prioritization, entered primarily to note that the initial implementation took the lowest/default priority in the interest of unblocking rapid development. This issue is intended to prompt a more careful implementation.

@austintlee
Copy link

opensearch-project/OpenSearch#10248 - none of this stuff needs to run in transport threads?

@dbwiddis
Copy link
Member Author

opensearch-project/OpenSearch#10248 - none of this stuff needs to run in transport threads?

We're calling the workflow steps via the MLClient, so the eventual calls will run on their appropriate threads. All of our calls have timeout (unless the user explicitly overrides.)

@joshpalis is working on our own thread pool in #63, see https://github.com/opensearch-project/opensearch-ai-flow-framework/pull/63/files#diff-9d60d6086ce87e2240d7d6bcf8d2a8b79c73400fc6ec6865f74fc571e03bf1af

@joshpalis
Copy link
Member

PR #63 has been merged, which actions this issue by overriding the getExecutorBuilders extension point to introduce a custom thread pool for use in provisioning the infrastructure defined within a use case template here.

There is an open question that I would like to bring up for discussion : what should our thread pool size/queueSize be for a provision workflow (which may become an expensive operation depending on the given use case)

    @Override
    public List<ExecutorBuilder<?>> getExecutorBuilders(Settings settings) {
        // TODO : Determine final size/queueSize values for the provision thread pool
        return ImmutableList.of(
            new FixedExecutorBuilder(
                settings,
                PROVISION_THREAD_POOL,
                OpenSearchExecutors.allocatedProcessors(settings),
                10,
                FLOW_FRAMEWORK_THREAD_POOL_PREFIX + PROVISION_THREAD_POOL
            )
        );
    }

Currently the provision thread pool is implemented as a FixedExecutorBuilder with a size value of 1 and a queue size value of 10. Perhaps this is sufficient, but I'm open to any suggestions for changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants