Make all ExtClients plain-ole-python objects #16352
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
| @@ -88,3 +84,6 @@ def _setup_io( | |||
| message_reader.read_messages(external_context) | |||
| ) | |||
| yield io_params_as_env_vars(context_injector_params, message_reader_params) | |||
|
|
|||
|
|
|||
| ExtSubprocess = ResourceParam[_ExtSubprocess] | |||
There was a problem hiding this comment.
@benpankow wanted to flag this. I think this is a way to avoid forcing people from doing ResourceParam everywhere. We could probably support this directly with a marker interface or something, but this was easy for now.
There was a problem hiding this comment.
This seems reasonable to me and is a neat pattern - I wonder if this sort of approach could help in cases where folks are using ResourceParam right now. Maybe if they re-use the same POPO in various places...?
There was a problem hiding this comment.
Yeah I guess I couldn't write this in docs without feeling embarassed about it. It is very counterintuitive at first glance.
| class _ExtSubprocess(ExtClient): | ||
| def __init__(self, env: Optional[Mapping[str, str]] = None, cwd: Optional[str] = None): | ||
| self.env = check.opt_mapping_param(env, "env", key_type=str, value_type=str) | ||
| self.cwd = check.opt_str_param(cwd, "cwd") |
There was a problem hiding this comment.
[1] Can we retain the param descriptions in a docstring
| self, env: Optional[Mapping[str, str]] = None, registry: Optional[Mapping[str, str]] = None | ||
| ): | ||
| self.env = check.opt_mapping_param(env, "env", key_type=str, value_type=str) | ||
| self.registry = check.opt_mapping_param(registry, "registry", key_type=str, value_type=str) |
| description="An optional dict of environment variables to set on the container.", | ||
| ) | ||
| def __init__(self, env: Optional[Mapping[str, str]] = None): | ||
| self.env = check.opt_mapping_param(env, "env", key_type=str, value_type=str) |
28c521d
to
f51a093
Compare
a898cac
to
9121668
Compare
## Summary & Motivation Making this class hierarchy not inherit from `ConfigurableResource`. To avoid forcing users from using `ResourceParam`, with every use, I am proposing a pattern where we export the annotated type. We could support this more directly and ergonomically in the framework, but I think this the user-facing behavior we want for ext users. ## How I Tested These Changes BK
Summary & Motivation
Making this class hierarchy not inherit from
ConfigurableResource. To avoid forcing users from usingResourceParam, with every use, I am proposing a pattern where we export the annotated type. We could support this more directly and ergonomically in the framework, but I think this the user-facing behavior we want for ext users.How I Tested These Changes
BK