Make injectors take no params and move tempdir handling to body of inject_context #16354
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
schrockn
force-pushed
the
make-clients-POPO-3
branch
from
49a7bc3
to
9981edf
Compare
schrockn
changed the title
schrockn
force-pushed
the
make-clients-POPO-2
branch
from
28c521d
to
f51a093
Compare
schrockn
force-pushed
the
make-clients-POPO-3
branch
from
663c2d7
to
6d174be
Compare
schrockn
force-pushed
the
make-clients-POPO-2
branch
from
a898cac
to
9121668
Compare
self review part 2 add old descs to docblocks Make Injector a factory object that produces an injection move all to cleanup reduce overengineering undo rename fix tests fix test again whoopsie
schrockn
force-pushed
the
make-clients-POPO-3
branch
from
6d174be
to
043dcda
Compare
|
Deploy preview for dagster-university ready! ✅ Preview Built with commit 043dcda. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 043dcda. |
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 043dcda. |
| @@ -196,7 +206,7 @@ def build_pod_body( | |||
| "name": pod_name, | |||
| } | |||
| if "labels" in meta: | |||
| meta["labels"] = {**get_common_labels(), **meta["labels"]} | |||
| meta["labels"] = {**get_common_labels(), **meta["labels"]} # type: ignore | |||
There was a problem hiding this comment.
it was barking at me in vscode. maybe I have pyright misconfigured?
There was a problem hiding this comment.
do whatever works, just wanted to make sure it was intentional
## Summary & Motivation Similar to #16354, change the message reader class so it can be instantiated at client construction time, rather than requiring a temporary directory already exist. This ends up simplifying some code paths that had to account the readers could be created on demand. Instead all these codepaths not assume a single reader instance is passed in and always there. ## How I Tested These Changes BK
…ject_context (dagster-io#16354) ## Summary & Motivation We want to restructure the ext clients so that they receive the context injector arguments as `__init__` time, rather than as a parameter to run. However right now in some cases (namely the file case) the context injector has to be constructed when the temp directory is already created. This moves the temp dir creation logic into the `inject_context` method itself. This makes the `__init__` trivial. The downside is that we create two tempdirs, one for the message reader and one for the context injector. However that in my judgement is immateriai ## How I Tested These Changes BK
## Summary & Motivation Similar to dagster-io#16354, change the message reader class so it can be instantiated at client construction time, rather than requiring a temporary directory already exist. This ends up simplifying some code paths that had to account the readers could be created on demand. Instead all these codepaths not assume a single reader instance is passed in and always there. ## How I Tested These Changes BK
Summary & Motivation
We want to restructure the ext clients so that they receive the context injector arguments as
__init__time, rather than as a parameter to run. However right now in some cases (namely the file case) the context injector has to be constructed when the temp directory is already created. This moves the temp dir creation logic into theinject_contextmethod itself. This makes the__init__trivial. The downside is that we create two tempdirs, one for the message reader and one for the context injector. However that in my judgement is immateriaiHow I Tested These Changes
BK