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

Added support for subinterpreter workers #850

Merged
merged 17 commits into from
Jan 5, 2025
Merged

Added support for subinterpreter workers #850

merged 17 commits into from
Jan 5, 2025

Conversation

agronholm
Copy link
Owner

Changes

Adds experimental support for subinterpreter workers

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

  • You've added tests (in tests/) added which would fail without your patch
  • You've updated the documentation (in docs/, in case of behavior changes or new
    features)
  • You've added a new changelog entry (in docs/versionhistory.rst).

If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.

Updating the changelog

If there are no entries after the last release, use **UNRELEASED** as the version.
If, say, your patch fixes issue #123, the entry should look like this:

- Fix big bad boo-boo in task groups
  (`#123 <https://github.com/agronholm/anyio/issues/123>`_; PR by @yourgithubaccount)

If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.

else:
return dedent(
f"""
{super().__str__()}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better now?

try:
func, args, kwargs = loads(item)
retval = func(*args, **kwargs)
except Exception as exc:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseException?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

_interpreter_id: int
_queue_id: int

async def initialize(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async def initialize(self) -> None:
def initialize(self) -> None:

does this need to be async?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, and in fact it should not set up the interpreter in the event loop thread either. I'll fix both issues at once.

@agronholm
Copy link
Owner Author

Do we need pruning of interpreters that have been unused for too long?

@richardsheridan
Copy link
Contributor

Actually I was going to comment on that. following the trio thread cache model has been standard. Unlike threads or processes though, it seems like your implementation would not be able to free up any resources of a timed-out worker until someone calls run_sync. There might be a better way to free up after something caches a bunch of interpreters? otherwise you need something like a background pruning task.

Also, are there any implications for unexpected behavior as the subinterpreters jump around different threads?

@agronholm
Copy link
Owner Author

Actually I was going to comment on that. following the trio thread cache model has been standard. Unlike threads or processes though, it seems like your implementation would not be able to free up any resources of a timed-out worker until someone calls run_sync. There might be a better way to free up after something caches a bunch of interpreters? otherwise you need something like a background pruning task.

I was thinking of pruning unused workers before or after a call to run_sync(). IIRC we do the same with unused worker threads.

Also, are there any implications for unexpected behavior as the subinterpreters jump around different threads?

None that I'm aware of.

@richardsheridan
Copy link
Contributor

Separately, the cancellation story of a busy worker seems poor here. if you abandon the thread, the destroy in the atexit handler will fail. Not sure what the consequences will be!

@agronholm
Copy link
Owner Author

Separately, the cancellation story of a busy worker seems poor here. if you abandon the thread, the destroy in the atexit handler will fail. Not sure what the consequences will be!

But the worker threads should all be gone by the time the atexit hooks are run?

@agronholm
Copy link
Owner Author

Separately, the cancellation story of a busy worker seems poor here. if you abandon the thread, the destroy in the atexit handler will fail. Not sure what the consequences will be!

But the worker threads should all be gone by the time the atexit hooks are run?

I tested with this:

import time

import anyio
from anyio import to_interpreter


async def main():
    await to_interpreter.run_sync(time.sleep, 6, abandon_on_cancel=True)


anyio.run(main)

It won't exit the process until the worker thread has run its course.

@richardsheridan
Copy link
Contributor

Actually I was going to comment on that. following the trio thread cache model has been standard. Unlike threads or processes though, it seems like your implementation would not be able to free up any resources of a timed-out worker until someone calls run_sync. There might be a better way to free up after something caches a bunch of interpreters? otherwise you need something like a background pruning task.

I was thinking of pruning unused workers before or after a call to run_sync(). IIRC we do the same with unused worker threads.

I think you mean processes? anyway, the issue is that the processes can automatically free up most resources except the OS process table bits that need waiting. An interpreter might sit on gigabytes of memory or thousands of sockets until the next call rather than the prescribed timeout.

Separately, the cancellation story of a busy worker seems poor here. if you abandon the thread, the destroy in the atexit handler will fail. Not sure what the consequences will be!

But the worker threads should all be gone by the time the atexit hooks are run?

Not if the subinterpereter is deadlocked or something like that!

Also, just realized that a cancelled subinterpreter worker should definitely not be returned to the idle worker queue.

@agronholm
Copy link
Owner Author

Actually I was going to comment on that. following the trio thread cache model has been standard. Unlike threads or processes though, it seems like your implementation would not be able to free up any resources of a timed-out worker until someone calls run_sync. There might be a better way to free up after something caches a bunch of interpreters? otherwise you need something like a background pruning task.

I was thinking of pruning unused workers before or after a call to run_sync(). IIRC we do the same with unused worker threads.

I think you mean processes? anyway, the issue is that the processes can automatically free up most resources except the OS process table bits that need waiting. An interpreter might sit on gigabytes of memory or thousands of sockets until the next call rather than the prescribed timeout.

Separately, the cancellation story of a busy worker seems poor here. if you abandon the thread, the destroy in the atexit handler will fail. Not sure what the consequences will be!

But the worker threads should all be gone by the time the atexit hooks are run?

Not if the subinterpereter is deadlocked or something like that!

Explain please. The task might abandon the worker thread, but the worker thread won't abandon the subinterpreter; it will continue to run the given code until it completes.

Also, just realized that a cancelled subinterpreter worker should definitely not be returned to the idle worker queue.

Yes, if abandon_on_cancel=True, we should not add the subinterpreter to the idle queue when cancelled. I will deal with this somehow.

@richardsheridan
Copy link
Contributor

It won't exit the process until the worker thread has run its course.

Is this all happening in a non-daemon thread? or is the interpreter shutodwn logic of python doing this? Either way it seems problematic. Maybe an initial release could ignore "abandon_on_cancel"?

@agronholm
Copy link
Owner Author

It won't exit the process until the worker thread has run its course.

Is this all happening in a non-daemon thread? or is the interpreter shutodwn logic of python doing this? Either way it seems problematic. Maybe an initial release could ignore "abandon_on_cancel"?

The worker threads are daemonic, but there is a hook added to the root task that will ensure the threads have finished before the event loop exits. It's not 100% foolproof (what is?) but good enough for the vast majority of cases.

But I'm okay with leaving out abandon_on_cancel in the initial release.

@richardsheridan
Copy link
Contributor

But the worker threads should all be gone by the time the atexit hooks are run?

Not if the subinterpereter is deadlocked or something like that!

Explain please. The task might abandon the worker thread, but the worker thread won't abandon the subinterpreter; it will continue to run the given code until it completes.

Maybe i don't understand the order of operations on exit. I thought the main script ends, then atexits are run, then daemon threads are held up acquiring the gil, then interpreters are destroyed, then python runtime ends.

Reading your previous message, it sounds like anyio interjects another step up there.

I think cancellation should wait until subinterpreters have a better interruption story. I tried and failed to make workers that run on channels and thread/interpreter pairs.

docs/subinterpreters.rst Outdated Show resolved Hide resolved
Co-authored-by: Jordan Speicher <uSpike@users.noreply.github.com>
@richardsheridan
Copy link
Contributor

... processes can automatically free up most resources except the OS process table bits that need waiting.

I just reviewed the anyio.to_process implementation and noticed it lacks this ability as well, so I guess there's no strong reason to try to implement it for interpreters in this pr.

We can add it back later, just needs to be consistent across the worker thread/interpreter/process APIs
@agronholm agronholm requested a review from graingert January 4, 2025 23:26
@agronholm agronholm merged commit 264a6f9 into master Jan 5, 2025
17 checks passed
@agronholm agronholm deleted the subinterpreters branch January 5, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants