This repository was archived by the owner on Mar 26, 2025. It is now read-only.
Feat: Sync spawn methods + fixing sync_result
#321
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrkaye97
commented
Feb 14, 2025
Comment on lines
-37
to
+46
| with EventLoopThread() as loop: | ||
| coro = self.workflow_listener.result(self.workflow_run_id) | ||
| future = asyncio.run_coroutine_threadsafe(coro, loop) | ||
| return future.result() | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
| try: | ||
| return loop.run_until_complete(coro) | ||
| finally: | ||
| asyncio.set_event_loop(None) | ||
| else: | ||
| coro = self.workflow_listener.result(self.workflow_run_id) | ||
| future = asyncio.run_coroutine_threadsafe(coro, loop) | ||
| return future.result() | ||
| return loop.run_until_complete(coro) |
Contributor
Author
There was a problem hiding this comment.
I'm not 100% sure this is safe, but I think the only risk of it causing issues is if there's no event loop, which I can't imagine being super common. It's a little tough to test well though
Merged
mrkaye97
added a commit
that referenced
this pull request
Feb 19, 2025
* Fix: More OTel Cleanup (#319) * feat: factor our helpers * chore: bump version * fix: replace none-default * fix: auto-create parent * feat: docs * fix: doc format * fix: example * feat: use global tracer if none provided * fix: docs * Fix: Bump min `celpy` version (#320) * fix: bump min celpy version * chore: ver * Feat: Sync spawn methods + fixing `sync_result` (#321) * feat: add sync spawn method * feat: example * fix: sync workflow run + sync_result * fix: examples * fix: lint * feat: bulk spawn * chore: ver * fix: test * fix: lint * Feat: Killing sync threads (#323) * feat: thread killing * fix: rm events * fix: lint * fix: cruft * fix: async sleep * feat: flag for enabling force killing threads
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Couple things:
spawn_workflowandspawn_workflowson the context since a few people have asked about thatsync_result- it was pretty buried, but basically it was hanging for two reasons. One is that thePooledWorkflowRunListenerwasn't being initialized correctly if no loop was running, because the workflow listener needs an async gRPC connection but we were trying to create one with no loop running. Then the other issue was thatrun_coroutine_threadsafewasn't actually doing what we want - we needrun_until_completeinsteadThis is definitely a bit of a case of "it worked on my local", but it should at least be less broken than the current state of affairs