Alternative proposal for concurrent.futures support in effectful#400
Alternative proposal for concurrent.futures support in effectful#400kiranandcode wants to merge 9 commits intostaging-llmfrom
concurrent.futures support in effectful#400Conversation
concurrent.futures support in effectful
jfeser
left a comment
There was a problem hiding this comment.
I think this looks like it's moving in good directions. The simple API is a good sign.
I don't like the look of release_handler_lock and friends. I'm surprised that nothing in the LLM provider code seems to call it, which suggests that it must be called by users of that code? I definitely don't think we want this to be part of the public API. My suggestion is to leave this extra API out until we have a clear need for it.
| @dataclass(frozen=True) | ||
| class DoneAndNotDoneFutures[T]: | ||
| done: set[Future[T]] | ||
| not_done: set[Future[T]] |
There was a problem hiding this comment.
Isn't this defined in the concurrent.futures library? Do we need our own dataclass definition?
There was a problem hiding this comment.
it's a namedtuple in the library, I added this dataclass because @defop ... using the namedtuple type was not allowing .done iirc (or there could have been another issue that was causing this to fail and I misdiagnosed the cause)
There was a problem hiding this comment.
There shouldn't be a behavioral difference in effectful between the dataclass and the namedtuple for this simple case, so it's probably worth creating a self-contained test that passes with the dataclass and fails with the namedtuple so that we can try to fix it.
There was a problem hiding this comment.
Oh yep will do!
Oh, yep, that was enabled by the realisation that |
|
Proposed interface is now: |
In draft mode as it's very WIP, but following discussions with @jfeser, this branch proposes an alternative implementation for async support for effectful through concurrent.futures.