Created
September 28, 2023 23:56
-
-
Save smackesey/95235d303bcae1fddf9a85c9fb504fd1 to your computer and use it in GitHub Desktop.
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
I like `open`. Here is PR: | |
https://github.com/dagster-io/dagster/pull/16894/files | |
That currently just modifies subprocess. If that approach looks reasonable to you then I will go ahead and convert the other clients. | |
I keep running into the issue that `open_pipes_session` needs to `__exit__` before we can guarantee all results are available. We need a way to execute this from the yielded `PipesSession`, otherwise it is difficult to find an appropriate place to start `open_pipes_session`. | |
There is a hack in the PR that accomplishes this that I think is sufficient for 1.5, but in order to better solve the problem I think we will need to fast-follow with a class-based context manager somewhere for the pipes session. | |
Also, with the `open_pipes_session` manual `__enter__`/`__exit__` workaround in that PR, we don’t need a context manager for `stream`. It could be changed to this: | |
```return subprocess_client.open(...).run() # sync | |
yield from subprocess_client.open(...).stream() # async``` | |
That feels cleaner to me because: | |
* it matches dagster-dbt | |
* don’t need to support two methods (`run` and `open`) with the same signature | |
* no context manager complexity for the user | |
Alternatively it could be: | |
```return subprocess_client.run(...).get_results() # sync | |
yield from subprocess_client.run(...).stream_results() # async``` |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment