Environment configuration #895

THardy98 · 2025-06-10T16:37:51Z

What was changed

Environment configuration for Python SDK, using core base envconfig module

Closes [Feature Request] Environment configuration #835
How was this tested:
test_envconfig.py
Any docs updates needed?
probably

Sushisource

Make sure you get rid of those .DS_Store macOS file nonsense. They should be globally excluded by gitignore.

I dunno if we need to do this in this PR, we can wait and see what @cretz and @dandavison opinions are too, but IMO it would make good sense to add a Client.connect() overload that takes a ClientConfigProfile here for easy use.

Also these guys need experimental notices on them too

temporalio/envconfig.py

THardy98 · 2025-06-13T08:17:42Z

Make sure you get rid of those .DS_Store macOS file nonsense. They should be globally excluded by gitignore.

Whoops, didn't realize I had committed them. Removed, added to gitignore

I dunno if we need to do this in this PR, we can wait and see what @cretz and @dandavison opinions are too, but IMO it would make good sense to add a Client.connect() overload that takes a ClientConfigProfile here for easy use.

seems reasonable, will wait to chad gets back as i think he'll have more to suggestions anyway

Also these guys need experimental notices on them too

added to each class (not sure if each method is necessary?)

Sushisource

This is looking good to me, but have Dan or Chad when he's back take a look

pyproject.toml

temporalio/envconfig.py

cretz · 2025-06-20T14:07:37Z

temporalio/envconfig.py

+DataSource: TypeAlias = Union[
+    str, bytes
+]  # str represents a file path, bytes represents raw data


This is a bit confusing. It is very normal to provide PEM file contents as a string in languages. We should differentiate file path better. Maybe Union[pathlib.Path, str, bytes] and only paths are treated as such?

I've changed the typing to your suggestion but I'm unsure whether or not to keep the str type.

The core TOML parser reads the cert configs into:

pub enum DataSource { Path(String), Data(Vec<u8>), }

so lang only sees these cert configs as bytes or a path (which we now read as an actual Path type).

Users can provide file contents as string but it would only be possible programmatically, like the (added) test test_read_source_from_string_content (unless I'm misunderstanding something). I am not sure if we wanted envconfig to be used this way

The core TOML parser reads the cert configs into:

Rust has a clearer difference between paths and data. It is normal in Python to have a string of data. test_read_source_from_string_content is actually test_read_source_from_bytes_content because you have a b in front. I think it makes sense to accept string content too since PEM data is often a string, but if not, no prob, let's just make sure the exception is clear if the data type is wrong.

test_read_source_from_string_content reads in string content, but asserts that it is read correctly into bytes.
So it's still from string content so to speak.

temporalio/envconfig.py

cretz · 2025-06-23T14:16:36Z

temporalio/envconfig.py

+        return source.encode("utf-8")
+    if isinstance(source, bytes):
+        return source
+    return None


We should not silently drop/ignore invalid data types in this case at runtime, should raise here IMO

raise a TypeError if invalid

cretz · 2025-06-23T14:19:06Z

temporalio/envconfig.py

+from typing_extensions import TypeAlias, TypedDict
+
+import temporalio.service
+from temporalio.bridge.temporal_sdk_bridge import envconfig as _bridge_envconfig


I think it may make more sense to have a temporalio/bridge/envconfig.py module that wraps all the bridge stuff. Every other use of the bridge in this repo goes through that approach. But not required.

cretz · 2025-06-23T14:20:57Z

temporalio/envconfig.py

+    def load_client_connect_config(
+        profile: str = "default",
+        *,
+        env_vars: Optional[Mapping[str, str]] = None,


If we want this here, can this be moved to the bottom of the kwarg list and be called override_env_vars to be clear that it's abnormal to set?

moved/renamed

cretz · 2025-06-23T14:22:24Z

tests/test_envconfig.py

+        ClientConfig.load_profile_from_data(toml_config)
+
+
+def test_load_client_connect_config(base_config_file: Path):


Can you have somewhere that splats this on to .connect and makes an actual connection (say to the same dev server used in other tests or start your own if you must)? We need something to confirm type checking on the result of the load_client_connect_config call that it matches the connect arg expectations.

cretz · 2025-06-23T14:26:55Z

temporalio/envconfig.py

+        return ClientConfig._from_bridge_profiles(loaded_profiles)
+
+    @staticmethod
+    def load_profiles_from_file(


I wonder if we can combine all of these load calls into one to reduce surface area since they are not the commonly called methods. I know Go doesn't combine them because it's a bit harder for that language. I figure you have one load call that can accept config file override (can be path or data like cert), disable env, override env, etc.

As for the load_profile stuff, wouldn't that just be load on ClientConfigProfile instead of this class? Most of the same parameters should apply.

done, some changes to the bridge to support this

cretz · 2025-06-23T14:29:14Z

temporalio/envconfig.py

+
+
+@dataclass
+class ClientConfig:


We want users to be able to convert this (and ClientConfigProfile) to/from TOML on their own if they want. However, we don't necessarily need to expose literal TOML conversion, just the ability to convert to/from the dict that represents TOML (they can use some Python TOML library if they want to do the TOML part). Can we expose to_dict and from_dict on this and the ClientConfigProfile class that represents exactly what it looks like in TOML?

added to_dict methods
the resulting dictionaries are well-typed, not sure if that was necessary or not

one caveat with this, when the given config input is in bytes
to_dict converts bytes data into strings because strings are more suitable for TOML

I wrote it in a test for this, converting from to_dict -> from_dict -> compare:

# We expect the new profile to be the same, but with bytes-based data # sources converted to strings. This is because to_dict converts # bytes-based data to a string, suitable for TOML. So we only have # a string representation to work with.

cretz

Looks great! This is a good model to follow for other SDKs. Only one class name change request (and two other unimportant comments).

cretz · 2025-06-26T12:43:27Z

temporalio/envconfig.py

+
+
+# We define typed dictionaries for what these configs look like as TOML.
+class ClientTLSConfigDict(TypedDict, total=False):


I was originally going to say you could leave these out since users using to/from dict should not really care about the types since that's low level. But I now see that we also are dogfooding the dict serialization stuff (awesome), so can leave even if just for our benefit.

cretz · 2025-06-26T12:44:34Z

temporalio/envconfig.py

+        )
+
+
+class ClientConnectConfig(TypedDict, total=False):


Just a heads up here, I think we may be having another situation ("plugins") where this actually has value at the client level for others wanting to type a dict for the kwargs of connect/init. So this may move to the client module in the future.

temporalio/envconfig.py

…ntal to class docstrings, misc

…client_connect_config actually matches connect arguments

* Implementation of environment configuration for Python SDK * pyright lint exclusion * simplify DataSource to union type, read file using Path, add experimental to class docstrings, misc * Remove .DS_Store files from Git tracking * fix bridge after pyo3 upgrade * handle windows file paths * quick pr suggestions * add Path to DataSource to read file paths, read file contents via string * add ClientConnectConfig and to_client_connect_config * added load_client_connect_config * raise TypeError when reading invalid data source * load_client_connect_config rename env_vars to override_env_vars * add .connect calls to test_load_client_connect_config to ensure load_client_connect_config actually matches connect arguments * refactor to general 'load' methods * add to_dict methods * formatting + linting * class rename, some cleanup with type casting

THardy98 requested a review from a team as a code owner June 10, 2025 16:37

THardy98 marked this pull request as draft June 10, 2025 16:37

THardy98 force-pushed the env_config branch from 924ead8 to da54b88 Compare June 11, 2025 17:59

THardy98 changed the title ~~init impl~~ Environment configuration Jun 11, 2025

THardy98 marked this pull request as ready for review June 11, 2025 22:19

THardy98 force-pushed the env_config branch from ccc6a0d to ed05f33 Compare June 11, 2025 22:26

Sushisource reviewed Jun 12, 2025

View reviewed changes

temporalio/envconfig.py Outdated Show resolved Hide resolved

temporalio/envconfig.py Outdated Show resolved Hide resolved

temporalio/envconfig.py Outdated Show resolved Hide resolved

THardy98 requested a review from Sushisource June 16, 2025 15:38

Sushisource approved these changes Jun 16, 2025

View reviewed changes

THardy98 requested a review from cretz June 20, 2025 14:04

cretz reviewed Jun 20, 2025

View reviewed changes

cretz reviewed Jun 23, 2025

View reviewed changes

THardy98 force-pushed the env_config branch from 4e4551b to 69bc396 Compare June 25, 2025 22:57

THardy98 requested a review from cretz June 25, 2025 22:58

cretz approved these changes Jun 26, 2025

View reviewed changes

THardy98 added 9 commits June 26, 2025 16:45

Implementation of environment configuration for Python SDK

474029b

pyright lint exclusion

9735dda

simplify DataSource to union type, read file using Path, add experime…

2467869

…ntal to class docstrings, misc

Remove .DS_Store files from Git tracking

77a6537

fix bridge after pyo3 upgrade

036bd5d

handle windows file paths

2bb09fc

quick pr suggestions

262ae97

add Path to DataSource to read file paths, read file contents via string

bee65cd

add ClientConnectConfig and to_client_connect_config

58d7378

THardy98 added 8 commits June 26, 2025 16:45

added load_client_connect_config

240ff0e

raise TypeError when reading invalid data source

652f610

load_client_connect_config rename env_vars to override_env_vars

f517dab

add .connect calls to test_load_client_connect_config to ensure load_…

82809d9

…client_connect_config actually matches connect arguments

refactor to general 'load' methods

0e65f22

add to_dict methods

68efb13

formatting + linting

f62727a

class rename, some cleanup with type casting

c4f1345

THardy98 force-pushed the env_config branch from 119baa9 to c4f1345 Compare June 26, 2025 14:46

THardy98 merged commit 34bb050 into main Jun 26, 2025
40 of 48 checks passed

THardy98 deleted the env_config branch June 26, 2025 16:52

cretz mentioned this pull request Jun 27, 2025

[Feature Request] Environment configuration temporalio/features#441

Open

9 tasks

THardy98 mentioned this pull request Sep 5, 2025

Environment configuration temporalio/sdk-ruby#326

Merged

		ClientConfig.load_profile_from_data(toml_config)


		def test_load_client_connect_config(base_config_file: Path):



		# We define typed dictionaries for what these configs look like as TOML.
		class ClientTLSConfigDict(TypedDict, total=False):

Environment configuration #895

Environment configuration #895

Uh oh!

Conversation

THardy98 commented Jun 10, 2025

What was changed

Uh oh!

Sushisource left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

THardy98 commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sushisource left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

THardy98 commented Jun 13, 2025 •

edited

Loading

THardy98 Jun 21, 2025 •

edited

Loading

cretz Jun 23, 2025 •

edited

Loading

THardy98 Jun 25, 2025 •

edited

Loading

THardy98 Jun 25, 2025 •

edited

Loading