Conversation
| client_connect_config: LambdaClientConnectConfig = field( | ||
| default_factory=LambdaClientConnectConfig | ||
| ) | ||
| worker_config: WorkerConfig = field(default_factory=WorkerConfig) |
There was a problem hiding this comment.
My main design question is about this. With this decision, we auto-reuse all the worker init parameters, which is good, but the syntax looks like:
config.worker_config["task_queue"] = "my_task_queue"
Which, the key literal does autocomplete, but it's maybe not as nice as duplicating some of the more common fields so we can have
config.task_queue = "my_task_queue"
The downside there is, of course, that they're duplicative of the same fields as keys on worker_config.
I think I prefer how it is now, since the keys still autocomplete, but would like to hear input.
There was a problem hiding this comment.
We don't have a type like WorkerConfig for client connect. We probably should - I could add it in this PR but it would mean making changes to the main part of the SDK (albeit easy ones)
There was a problem hiding this comment.
Feedback from SDK team: Let's stick with dict for now & add Client connection config class
1f438ac to
f837c8a
Compare
| def run_worker( | ||
| version: WorkerDeploymentVersion, | ||
| configure: Callable[[LambdaWorkerConfig], None], | ||
| ) -> Callable[[Any, Any], None]: |
There was a problem hiding this comment.
I assume there's nothing better we can do on the types here because this is what the lambda handler expects?
There was a problem hiding this comment.
We maybe could, but it'd involve taking a dep purely for that.
There was a problem hiding this comment.
Hmmm... And do they actually write code that interacts with those types, or it would just be for us?
There was a problem hiding this comment.
It would be only for type verification
98402f2 to
696fd88
Compare
This is verified working in combination with this sample: temporalio/samples-python#286
At least one open design question I want to resolve before merging.