[pythonic resources][rfc] Adjust EnvVar behavior to explicitly disallow direct usage #14490

benpankow · 2023-05-26T11:44:32Z

Summary

Alternative RFC to #14488. Keeps EnvVar as a direct subclass of str but errors on resolving the value via __str__ to explicitly prevent users from using EnvVar outside of resource/config contexts. Right now we're sort of in the worst of both worlds where a user can use an EnvVar in inapproporiate places and they get an odd result.

Users who try to use an EnvVar value as a string will hit an error:

my_env_var = EnvVar("MY_ENV_VAR")

# errors!
print(f"Hello, {my_env_var}!")

Users can directly access the EnvVar name or value:

my_env_var = EnvVar("MY_ENV_VAR")

print(f"Env var {my_env_var.env_var_name} has value {my_env_var.get_value()}")

Test Plan

Set of unit tests, existing unit tests.

benpankow · 2023-05-26T11:44:45Z

Current dependencies on/for this PR:

master
- PR [pythonic resources][rfc] Adjust EnvVar behavior to explicitly disallow direct usage #14490 👈

This comment was auto-generated by Graphite.

schrockn · 2023-05-26T13:42:34Z

This one seems better to me. We could also offer a method to access the underlying value so that users won't have to import os if they find that annoying.

github-actions · 2023-09-29T18:14:42Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-hnxsdvvfq-elementl.vercel.app
https://benpankow-rfc-late-access-env-var-2.components-storybook.dagster-docs.io

Built with commit cb4aa7c.
This pull request is being automatically deployed with vercel-action

github-actions · 2023-09-29T18:15:07Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-6f9osqza7-elementl.vercel.app
https://benpankow-rfc-late-access-env-var-2.core-storybook.dagster-docs.io

Built with commit cb4aa7c.
This pull request is being automatically deployed with vercel-action

github-actions · 2023-09-29T18:16:12Z

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-h0jj34y6x-elementl.vercel.app
https://benpankow-rfc-late-access-env-var-2.dagster-university.dagster-docs.io

Built with commit 1ee1c12.
This pull request is being automatically deployed with vercel-action

schrockn · 2023-09-29T18:43:46Z

python_modules/dagster/dagster/_config/field_utils.py

+            f'Attempted to directly retrieve environment variable IntEnvVar("{self.name}").'
+            " IntEnvVar should only be used as input to Dagster config or resources."


Add:

"If you want to access the environment variable directly, you can call get_value on this object, or os.getenv directly"

We could also save.

"The purpose of EnvVar is to defer retrieval to right before the step is executed, and to be viewable in the UI"

schrockn

Let's do a turn on the error messaging, which is the most important part of this PR.

schrockn · 2023-09-29T18:43:59Z

python_modules/dagster/dagster/_config/field_utils.py

+            f'Attempted to directly retrieve environment variable EnvVar("{self.env_var_name}").'
+            " EnvVar should only be used as input to Dagster config or resources."
+        )


schrockn · 2023-09-29T18:45:12Z

python_modules/dagster/dagster/_config/field_utils.py

+            f'Attempted to directly retrieve environment variable IntEnvVar("{self.name}").'
+            " IntEnvVar should only be used as input to Dagster config or resources."


We could also save.

"The purpose of EnvVar is to defer retrieval to right before the step is executed, and to be viewable in the UI"

benpankow · 2023-09-29T23:37:39Z

Updated copy 👍

schrockn

Great. Consider a common function to share between two callsites to generate error string.

schrockn · 2023-09-30T08:07:32Z

python_modules/dagster/dagster/_config/field_utils.py

+        """
+        raise RuntimeError(
+            f'Attempted to directly retrieve environment variable EnvVar("{self.env_var_name}").'
+            " EnvVar defers resolution of the env var value until run time, and should only be"


spell out "environment variable" instead of "env var"

…ow direct usage

github-actions · 2023-09-30T18:36:35Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-bh67a7ww3-elementl.vercel.app
https://benpankow-rfc-late-access-env-var-2.dagster.dagster-docs.io

Direct link to changed pages:

https://dagster-docs-bh67a7ww3-elementl.vercel.app
https://benpankow-rfc-late-access-env-var-2.dagster.dagster-docs.io/_apidocs

benpankow mentioned this pull request May 26, 2023

[pythonic resources][rfc] Adjust EnvVar behavior to allow for direct usage #14488

Closed

benpankow requested review from schrockn, OwenKephart and jamiedemaria May 26, 2023 14:06

benpankow requested a review from gibsondan September 29, 2023 18:10

benpankow force-pushed the benpankow/rfc-late-access-env-var-2 branch from 1b9048d to 1ee1c12 Compare September 29, 2023 18:11

schrockn reviewed Sep 29, 2023

View reviewed changes

schrockn requested changes Sep 29, 2023

View reviewed changes

benpankow requested a review from schrockn September 29, 2023 23:37

schrockn approved these changes Sep 30, 2023

View reviewed changes

benpankow added 7 commits September 30, 2023 11:24

[pythonic resources][rfc] Adjust EnvVar behavior to explicitly disall…

3350037

…ow direct usage

a few more tests, add docs

574bea8

int env var

d13af4f

fix tests

9ad07bc

fix

97b49c7

update err copy

c9d2de3

factor out error fn, expand 'env var' to environment variable

cb4aa7c

benpankow force-pushed the benpankow/rfc-late-access-env-var-2 branch from ff9101c to cb4aa7c Compare September 30, 2023 18:29

benpankow merged commit ee8a150 into master Sep 30, 2023
3 checks passed

benpankow deleted the benpankow/rfc-late-access-env-var-2 branch September 30, 2023 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pythonic resources][rfc] Adjust EnvVar behavior to explicitly disallow direct usage #14490

[pythonic resources][rfc] Adjust EnvVar behavior to explicitly disallow direct usage #14490

benpankow commented May 26, 2023 •

edited

Loading

benpankow commented May 26, 2023 •

edited

Loading

schrockn commented May 26, 2023

github-actions bot commented Sep 29, 2023 •

edited

Loading

github-actions bot commented Sep 29, 2023 •

edited

Loading

github-actions bot commented Sep 29, 2023

schrockn Sep 29, 2023

schrockn Sep 29, 2023

schrockn left a comment

schrockn Sep 29, 2023

schrockn Sep 29, 2023

benpankow commented Sep 29, 2023

schrockn left a comment

schrockn Sep 30, 2023

github-actions bot commented Sep 30, 2023

		f'Attempted to directly retrieve environment variable IntEnvVar("{self.name}").'
		" IntEnvVar should only be used as input to Dagster config or resources."

[pythonic resources][rfc] Adjust EnvVar behavior to explicitly disallow direct usage #14490

[pythonic resources][rfc] Adjust EnvVar behavior to explicitly disallow direct usage #14490

Conversation

benpankow commented May 26, 2023 • edited Loading

Summary

Test Plan

benpankow commented May 26, 2023 • edited Loading

schrockn commented May 26, 2023

github-actions bot commented Sep 29, 2023 • edited Loading

github-actions bot commented Sep 29, 2023 • edited Loading

github-actions bot commented Sep 29, 2023

schrockn Sep 29, 2023

Choose a reason for hiding this comment

schrockn Sep 29, 2023

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

schrockn Sep 29, 2023

Choose a reason for hiding this comment

schrockn Sep 29, 2023

Choose a reason for hiding this comment

benpankow commented Sep 29, 2023

schrockn left a comment

Choose a reason for hiding this comment

schrockn Sep 30, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 30, 2023

benpankow commented May 26, 2023 •

edited

Loading

benpankow commented May 26, 2023 •

edited

Loading

github-actions bot commented Sep 29, 2023 •

edited

Loading

github-actions bot commented Sep 29, 2023 •

edited

Loading