tractor icon indicating copy to clipboard operation
tractor copied to clipboard

Deprecate `ActorNursery.run_in_actor()` and offer as part of a new "high level" API: `tractor.to_actor`

Open goodboy opened this issue 3 years ago • 2 comments

I've rambled on wayyy too long in #287 but thanks to attempts to resolve that issue I think this is the way I'd like to go on a so called one shot task per actor-style "worker pool" API.


Proposed re-factored "one-shot" style API

After (too) much thought about where/how this should be exposed, i thankfully arrived at the right interface/naming, going with the same API-naming/semantics as in all other libs:

So it's simple; we offer,

  • a new tractor.to_actor submod with equivalent convenience invocation fns,
    • [ ] .to_actor.run() to run an async def fn() as a one-off trio.Task in a single subactor.
    • [ ] .to_actor.run_sync() to run an def fn() sync fn equivalent?
      • not sure we really need this so might leave it as a follow-up.

TDLR justification and premises

  • Portal.result() + ActorNursery.run_in_actor() couples a future-like API into our nursery's actor spawning interface at the wrong level of abstraction
  • .run_in_actor() can be implemented as a syntactic sugar on top of ActorNursery.start_actor() + Portal.run() using trio tasks in a similar way to our concurrent.futures worker pool example
  • conducting error collection and propagation in our ActorNursery's teardown machinery is entirely superfluous and a path dependent legacy design which has a few pretty awful side effects:
    • it makes the nursery need to be aware of .run_in_actor() portals
    • it enforces duplication between .run_in_actor() and Portal.run()
    • it complicates spawn machinery with special cases
    • it results in hard to maintain tests due to indeterminacy in cancelled vs. errored child results (which btw won't go away if we change the API layering but at least we can leverage std trio nursery machinery instead of rolling our own 😂)

ToDo

  • [x] figure out how to offer the higher level / new .run_in_actor() API as a cluster helper

    • [ ] add a public .to_actor which exposes all proposed one-shot APIs:
      • .run() for async sub-actor req/resp fn-style.
      • (maybe??) .run_sync() for sync equiv?
  • [ ] regard the prior API-extension/sugar ideas around .run_in_actor()-style but obvi design-n-implement on top of of this new API.

    • [ ] streaming-sugar idea in https://github.com/goodboy/tractor/issues/172
    • [ ] an async spawning API? per https://github.com/goodboy/tractor/issues/155
    • [ ] how will it interplay with supervisors, re https://github.com/goodboy/tractor/issues/22
    • [ ] avoid user confusion from the bug in https://github.com/goodboy/tractor/issues/287
      • patched (but not landed?) by https://github.com/goodboy/tractor/pull/288
        • detailed explainer comment: https://github.com/goodboy/tractor/issues/287#issuecomment-1004302645
        • comments on how .run_in_actor() is likely a bad API approach:
          • https://github.com/goodboy/tractor/issues/287#issuecomment-1005251485
          • https://github.com/goodboy/tractor/issues/287#issuecomment-1005746676
          • https://github.com/goodboy/tractor/issues/287#issuecomment-1005980332
  • [ ] allow easy selection of the start_method: str (which ofc sets the ._spawn._spawn_method: SpawnMethodKey) such that callers can spec which backend to employ.

  • [ ] convert all test_cancellation (mostly nested_multierrors()) suites to use the .open_context() / .run() style and ensure we can still get as reliable of performance

goodboy avatar Jan 05 '22 21:01 goodboy

A name came to me this morn:

  • async with tractor.open_task_pool() as cluster: or .open_actor_task_pool(), or we just do a simple .open_actor_cluster() as cluster: ?
  • provide a Cluster.run_in_actor()?

goodboy avatar Jan 06 '22 16:01 goodboy