Generalized functions for getting, setting, and finding environment variables - #798
Conversation
RHammond2
left a comment
RHammond2
left a comment
There was a problem hiding this comment.
@elenya-grant I think this is a great step in the right direction, especially with moving much of the functionality out of the resource subpackage!
I have two significant hangups with the current draft: 1) we should generally avoid using global variables as a best practice, and 2) much of the refactored code is only lightly refactored, leaving it still highly geared towards an NLR credential getting/setting process. The second point results in this solution still being overly complex for general use.
I think that the core code could be something more like the following example that largely mirrors your setup (I've got a few mismatched names for expediency), but is more flexible to non-NLR credential getting and setting in addition to simplifying the NLR setup. I would also imagine using the module as from h2integrate.core.env_tools as api so we could invoke usage such as api.get_credentials("NLR_API_EMAIL", "NLR_API_KEY").
Another added benefit is that instead of a developer being required to do 3 steps to interact with API settings, the below reduces is it to simply calling api.set_env_var_from_file(".newapirc", "NEW_EMAIL", "NEW_KEY"). Later, if needed the credentials could be extracted as new_email = os.environ["NEW_EMAIL"] or new_email = api.get_credentials("NEW_EMAIL"). Alternatively, they could simply be retrieved as part of the module setup.
To be clear, I don't believe my code needs to be taken exactly as written as it's a prototype, but the straightforwardness
of the solution should be considered to resolve the heart of #765.
import os
import warnings
from pathlib import Path
from h2integrate import ROOT_DIR
HOME_DIR = Path.home()
NREL_DEPRECATION_MSG = (
"The '{old}' environment variable is deprecated and will be removed in a future release. "
"Please use '{new}' instead. The nrel.gov API domain has moved to nlr.gov."
)
def read_config(file_path: Path) -> dict:
"""Load any dictionary-like key, value pairs from a configuration file (e.g. .env or .cdsapirc)
that uses either a ``key:value` or `key=value` format for storing data.
Args:
file_path (Path): The full file path and name containing configuration details to be
extracted.
Returns:
dict: Dictionary of key, value pairs found in :py:attr:`file_path`.
"""
if isinstance(file_path, str):
file_path = Path(file_path).resolve()
config = {}
with file_path.open("r") as f:
for line in f.readlines():
if ":" in line:
sep = ":"
elif "=" in line:
sep = "="
k, v = line.strip().split(sep, 1)
config[k.strip()] = v.strip()
return config
def get_credentials(*args: str, file_name: str | None = None) -> dict:
"""Retrieve a series of credentials from a :py:attr:`file_name` in either the home directory
or H2Integrate root directory.
Args:
args (str): Name(s) of the credential(s) that should be retrieved from either
:py:attr:`file_name` or environment variables.
file_name (str, optional): The name of a configuration file found in either the H2Integrate
root directory or the user's home directory that should contain the credential(s) in
:py:attr:`args`.
Returns:
dict: Dictionary of all :py:attr:`args` with values of either a found value or None.
"""
if file_name is not None:
if (file_path := (ROOT_DIR / file_name)).is_file():
config = read_config(file_path)
elif (file_path := (HOME_DIR / file_name)).is_file():
config = read_config(file_path)
return {name: config.get(name) for name in args}
return {name: os.environ.get(name) for name in args}
def set_env_var_from_file(file_name: str, *args: str):
"""Sets a series of environment variables from a file base in the H2Integrate root directory
or the user's home directory.
Args:
file_name (str): Name of the file found in either the H2Integrate root directory or the
user's home directory that contains the variables to be set for the computational
environment.
args (str): Name(s) of the credentials to retrieve from :py:attr:`file_name`.
Raises:
KeyError: Raised if any of :py:attr:`args` wasn't found in :py:attr:`file_name`.
"""
credentials = get_credentials(*args, file_name=file_name)
for name, value in credentials.items():
if value is None and os.environ.get(name) is None:
raise KeyError(f"No credential for {name} was found in '{file_name}'")
os.environ[name] = value
def get_nlr_api_credential(which: str) -> str:
"""Get either the NLR API email or key with a fallback for the NREL credentials.
Args:
which (str): One of "email" or "key" to indicate which NLR API credential should be
retrieved.
Raises:
ValueError: Raised if an invalid value was passed to :py:attr:`which`.
KeyError: Raised if neither of the NLR or NREL credentials could be found.
"""
if which not in ("email", "key"):
raise ValueError("`which` must be one of 'email' or 'key'.")
new_name = f"NLR_API_{which.upper()}"
old_name = f"NREL_API_{which.upper()}"
credentials = get_credentials(new_name, old_name, file_name=".env")
if (email := credentials[new_name]) is None:
if (email := credentials[old_name]) is None:
msg = (
f"No credential matching {new_name} or {old_name} found as an environment variable"
" nor the '.env' configuration file in the user home directory or H2Integrate root"
" directory."
)
raise KeyError(msg)
warnings.warn(
NREL_DEPRECATION_MSG.format(old=old_name, new=new_name),
FutureWarning,
stacklevel=3,
)
return email
def set_nlr_api_credential(which: str):
"""Set an NLR API email or key with a fallback for the NREL credentials.
Args:
which (str): One of "email" or "key" to indicate which NLR API credential should be
retrieved.
Raises:
ValueError: Raised if an invalid value was passed to :py:attr:`which`.
"""
if which not in ("email", "key"):
raise ValueError("`which` must be one of 'email' or 'key'.")
os.environ[f"NLR_API_{which.upper()}"] = get_nlr_api_credential(which=which)To answer one of your questions, I think the env_tools.py should be api_credentials.py or something with either "api" or "credentials" or similar in the name to indicate we're working specifically with API credentials. I think "env_tools.py" would be assumed to be coding environments more often than environment variables. The environment variables is also only one piece of the puzzle API credentials puzzle.
|
Responding to this comment from @RHammond2: First of all, thank you for the very helpful and thoughtful comment, this is very helpful in understanding your vision for resolving Issue #765. I like some of the added functionality that your approach introduces and I think I could get that into this PR (such as getting variables from a dictionary type formatted file). If I'm understanding your proposed code correctly, then I think that some of the existing functionality will be lost and other changes to the code may be needed (and some of this is related to the use of global variables). I'm also concerned about any lost functionality because it could mess with people's workflows. My understanding is that your proposed code would basically be used instead of the functions I have in I'm going to try to be concise and I'm sorry if it doesn't make sense! When I originally made the NLR API methods for the resource models, I was trying to make it so that finding/loading/setting/getting environment variables was as user-friendly as possible and would work for a variety of workflows and ways of setting environment variables while minimizing complexity within the resource models themselves. Quick summary of the current (before this PR) functionality with the NLR API credentials and my understanding of it:
The reason we're using global variables (and maybe we could use environment variables instead or something) is to accommodate for other user-workflow/preference cases. For example, a user is using a .env file to define the environment variables and that .env file is not in one of the "default" locations. If they were to try to run H2I with a resource model that needs the NLR_API_KEY, it would not be able to find the API key and the run would fail if their run script looked like this: from h2integrate import H2IntegrateModel
h2i = H2IntegrateModel("config.yaml")
h2i.setup()
h2i.run()Since the path to their ".env" file cannot be input to the resource models themselves, they need to instead call the setter method ahead of time so that the environment variables are set before the resource models call from h2integrate import H2IntegrateModel
from h2integrate.resource.utilities.nlr_developer_api_keys import set_nlr_key_dot_env
env_path = "Users/my/nonstandard/place/for/.env"
set_nlr_key_dot_env(path=env_path)
h2i = H2IntegrateModel("config.yaml")
h2i.setup()
h2i.run()Basically, the Responses to other stuff:I imagine that environment variables may be used to include more than just API credentials. Environment variables allow for folks to set a) info that shouldn't be shared and b) info that is specific to their workflow. For example, the RESOURCE_DIR can also be set as an environment variable and that'd be used instead of Related to above, I think that the Perhaps some of my push-back is because I'm misinterpreting your comment "it still highly geared towards an NLR credential getting/setting process". Do you mean specific to the NLR_API_KEY and NLR_API_EMAIL variables used for resource data downloads OR do you mean like some workflow that is common at NLR that isn't common elsewhere? I think the only NLR-specific part of my proposed changes is the depreciation method warning. Let me know if you want to hop on a call and chat about through some of this! I would be down to integrate a form of the |
|
Thanks for the all the additional context, @elenya-grant, it's quite helpful to understand where some of the naming is coming from! I'm going to respond below to all your comments/responses as encountered, so apologies if this reads a little akwardly. I'll immediately contradict that sentiment by addressing an item from the end first: let's keep you original thinking with an "env"-focused name instead of my thoughts on the "api"-based name. The naming of functions in my example code could easily be changed, they are merely what made sense of demonstrating the level of simplicity/limited scope I meant when discussing this refactor previously. As for global variables, I don't believe they should be used at all, particularly because they are generally discouraged as a best practice. I also didn't realize the existing implementation used them at all until I just double checked. I think if we're trying to be able to just trying to make access easier, then at the bottom of # env_tools.py
# all preceding code, including function definitions
set_nlr_api_credential()
NLR_API_EMAIL = get_nlr_api_credential("email")
NLR_API_KEY = get_nlr_api_credential("key")
# some other module
from h2integrate import NLR_API_EMAIL, NLR_API_KEYMy opinion is also that we can take the lead from how most other APIs enforce the placement of their API credential files (when environment variables are not set) and simply error out as opposed to allowing for arbitrary environment files without users explicitly defining this in a custom setup. As you suggested, users can easily avoid this by configuring their environments using the already provided directions, or simply setting them independently as is shown in your response. If that's less than desirable, then I think the credential getter I proposed could simply be updated to the following. def get_credentials(*args: str, file_name: str | None = None, file_path: str | Path | None = None) -> dict:
"""Retrieve a series of credentials from a :py:attr:`file_name` in either the home directory
or H2Integrate root directory. If `:py:attr:`file_path` is provided, then :py:attr:`file_name`
and already set environment variables will be ignored. If :py:attr:`file_name` is provided, then
already set environment variables will be ignored. If neither file options are used, then an
existing environment variable will be retrieved.
Args:
args (str): Name(s) of the credential(s) that should be retrieved from either
:py:attr:`file_name` or environment variables.
file_name (str, optional): The name of a configuration file found in either the H2Integrate
root directory or the user's home directory that should contain the credential(s) in
:py:attr:`args`.
file_path (str | Path, optional): The full file path for where the configuration file can be
found if not using the H2Integrate root directory or user home directory
Returns:
dict: Dictionary of all :py:attr:`args` with values of either a found value or None.
"""
if file_path is not None:
file_path = Path(file_path).resolve()
if file_path.is_file():
config = read_config(file_path)
return {name: config.get(name) for name in args}
raise FileNotFoundError(f"Provided `file_path` is invalid: {file_path}")
if file_name is not None:
if (file_path := (ROOT_DIR / file_name)).is_file():
config = read_config(file_path)
elif (file_path := (HOME_DIR / file_name)).is_file():
config = read_config(file_path)
return {name: config.get(name) for name in args}
return {name: os.environ.get(name) for name in args}To extend this, we could also add an def set_env_var(*, overwrite: bool = False, **kwargs: str):
"""Set or overwrite environment variables.
Args:
overwrite (bool, optional): Indicator to overwrite existing environment variables provided
in :py:attr:`kwargs`. Defaults to False.
kwargs (str): name and value of environment variables to set. If :py:attr:`overwrite` is
False, the value will be skipped.
"""
for name, value in kwargs.items():
if os.environ.get(name) is not None and not overwrite:
continue
os.environ[name] = value
def set_env_var_from_file(file_name: str, *args: str, *, overwrite: bool = False):
"""Sets a series of environment variables from a file base in the H2Integrate root directory
or the user's home directory.
Args:
file_name (str): Name of the file found in either the H2Integrate root directory or the
user's home directory that contains the variables to be set for the computational
environment.
args (str): Name(s) of the credentials to retrieve from :py:attr:`file_name`.
Raises:
KeyError: Raised if any of :py:attr:`args` wasn't found in :py:attr:`file_name`.
"""
credentials = get_credentials(*args, file_name=file_name)
for name, value in credentials.items():
if value is None and os.environ.get(name) is None:
raise KeyError(f"No credential for {name} was found in '{file_name}'")
set_env_var(overwrite=overwrite, **credentials)
Regarding my comment about the code being heavily geared towards NLR credentials, this is coming from the fact that |
|
Responding to this comment from @RHammond2 Thanks for the additional clarification and more context on your thoughts. Based on your feedback, I see 3-4 immediate changes that I could make to the code to align with your comments:
My thoughts on the generalize flow are listed below:
api_key = get_credentials("NLR_API_KEY", strict=False) # don't throw error, just try
if api_key is None:
api_key = get_credentials("NREL_API_KEY", strict=False) # don't throw error, just try
if api_key is not None:
# the deprecated one exists, so throw depreciation warning
warnings.warn(DEPRECIATION_MSG.format(old="NREL_API_KEY", new="NLR_API_KEY"), FutureWarning, stacklevel=3)
if api_key is None:
# API key has not been found - have it throw an error in `get_credentials` call
api_key = get_credentials("NLR_API_KEY", strict=True) Finally - if setting environment variables is done in Last question: The default folders we look in for the |
| env_path = temp_dir / ".env" | ||
| with env_path.open("w+") as file: | ||
| file.write("TEST_CREDENTIAL=my_credential_value\n") | ||
|
|
||
| env_vars = load_env_vars_from_file(file_path=env_path) | ||
| with subtests.test("Read using = seperator"): | ||
| assert env_vars["TEST_CREDENTIAL"] == "my_credential_value" | ||
|
|
||
| # add another variable to the file | ||
| with env_path.open("a+") as file: | ||
| file.write("TEST_CREDENTIAL_B=testing@yahoo.fake\n") | ||
|
|
||
| env_vars = load_env_vars_from_file(env_path) | ||
| with subtests.test("Two credential with = seperator (TEST_CREDENTIAL)"): | ||
| assert env_vars["TEST_CREDENTIAL"] == "my_credential_value" | ||
| with subtests.test("Two credential with = seperator (TEST_CREDENTIAL_B)"): | ||
| assert env_vars["TEST_CREDENTIAL_B"] == "testing@yahoo.fake" | ||
|
|
||
| # add a line without a separator to the file and a line using a different separator | ||
| with env_path.open("a+") as file: | ||
| file.write("TEST_CREDENTIAL_C_IS_A_SENTENCE\nTEST_CREDENTIAL_D : testingValue\n") | ||
|
|
||
| env_vars = load_env_vars_from_file(env_path) | ||
| with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL)"): | ||
| assert env_vars["TEST_CREDENTIAL"] == "my_credential_value" | ||
| with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL_B)"): | ||
| assert env_vars["TEST_CREDENTIAL_B"] == "testing@yahoo.fake" | ||
| with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL_D)"): | ||
| assert env_vars["TEST_CREDENTIAL_D"] == "testingValue" | ||
| with subtests.test("Empty line and mixed separators (skipped sentence line)"): | ||
| extra_vars = set(env_vars) - {"TEST_CREDENTIAL", "TEST_CREDENTIAL_B", "TEST_CREDENTIAL_D"} | ||
| assert len(extra_vars) == 0 |
There was a problem hiding this comment.
| env_path = temp_dir / ".env" | |
| with env_path.open("w+") as file: | |
| file.write("TEST_CREDENTIAL=my_credential_value\n") | |
| env_vars = load_env_vars_from_file(file_path=env_path) | |
| with subtests.test("Read using = seperator"): | |
| assert env_vars["TEST_CREDENTIAL"] == "my_credential_value" | |
| # add another variable to the file | |
| with env_path.open("a+") as file: | |
| file.write("TEST_CREDENTIAL_B=testing@yahoo.fake\n") | |
| env_vars = load_env_vars_from_file(env_path) | |
| with subtests.test("Two credential with = seperator (TEST_CREDENTIAL)"): | |
| assert env_vars["TEST_CREDENTIAL"] == "my_credential_value" | |
| with subtests.test("Two credential with = seperator (TEST_CREDENTIAL_B)"): | |
| assert env_vars["TEST_CREDENTIAL_B"] == "testing@yahoo.fake" | |
| # add a line without a separator to the file and a line using a different separator | |
| with env_path.open("a+") as file: | |
| file.write("TEST_CREDENTIAL_C_IS_A_SENTENCE\nTEST_CREDENTIAL_D : testingValue\n") | |
| env_vars = load_env_vars_from_file(env_path) | |
| with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL)"): | |
| assert env_vars["TEST_CREDENTIAL"] == "my_credential_value" | |
| with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL_B)"): | |
| assert env_vars["TEST_CREDENTIAL_B"] == "testing@yahoo.fake" | |
| with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL_D)"): | |
| assert env_vars["TEST_CREDENTIAL_D"] == "testingValue" | |
| with subtests.test("Empty line and mixed separators (skipped sentence line)"): | |
| extra_vars = set(env_vars) - {"TEST_CREDENTIAL", "TEST_CREDENTIAL_B", "TEST_CREDENTIAL_D"} | |
| assert len(extra_vars) == 0 | |
| env_path = temp_dir / ".env" | |
| with env_path.open("w+") as file: | |
| file.write("TEST_CREDENTIAL=my_credential_value\n") # = with no spaces | |
| file.write("TEST_CREDENTIAL_B=testing@yahoo.fake \n") # = with spaces | |
| file.write("TEST_CREDENTIAL_C_IS_A_SENTENCE\n") # should be skipped | |
| file.write("TEST_CREDENTIAL_D : testingValue\n") # : with spaces | |
| env_vars = load_env_vars_from_file(file_path=env_path) | |
| assert len(env_vars) == 3 | |
| assert env_vars["TEST_CREDENTIAL"] == "my_credential_value" | |
| assert env_vars["TEST_CREDENTIAL_B"] == "testing@yahoo.fake" | |
| assert env_vars["TEST_CREDENTIAL_D"] == "testingValue" |
The test as written covers all the bases well, but is a bit too verbose and hard to follow for what is a pretty straightforward test.
There was a problem hiding this comment.
It could even be worth adding a consecutive delimiter example like the below modification to highlight what happens when users don't pay attention.
file.write("TEST_CREDENTIAL_E :: another_credential\n")
assert len(env_vars) == 4
assert env_vars["TEST_CREDENTIAL_E"] == ": another_credential"
| env_file_txt = f"TEST_CREDENTIAL_A={temp_dir}\nTEST_CREDENTIAL_B=howdyFolks\n" | ||
| env_file_txt += "TEST_CREDENTIAL_C=i<3H2I\n" |
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_get_environment_variables_from_default_folder(subtests, temp_dir): |
There was a problem hiding this comment.
Same comment about grouping subtests applies here, though there are more categories.
There was a problem hiding this comment.
same response about I hear ya but I kept the overly verbose subtests and tried to add in comments to make it more clear.
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_get_environment_variables_from_default_cwd(subtests, temp_dir): |
There was a problem hiding this comment.
Same comment about grouping subtests.
…nto generalize_api
… and to raise a user warning
| with subtests.test("Empty line and mixed separators (skipped sentence line)"): | ||
| extra_vars = set(env_vars) - {"TEST_CREDENTIAL", "TEST_CREDENTIAL_B", "TEST_CREDENTIAL_D"} | ||
| assert len(extra_vars) == 0 |
There was a problem hiding this comment.
It's possible the changes in my below comment weren't pushed, but I do think this should just be a basic test at the beginning that simply checks the length of the returned env_vars as this is a bit of a convoluted way to do that.
There was a problem hiding this comment.
I think these changes should be pushed up. In test_load_env_vars_from_file is has assert len(env_vars)=4 (because I added another environment variable to the test) instead of the weird version I had before.
RHammond2
left a comment
RHammond2
left a comment
There was a problem hiding this comment.
Great work, @elenya-grant! I think once you've addressed everything we've talked about, this is good from my end since we cleared everything up in a discussion offline.
johnjasa
left a comment
johnjasa
left a comment
There was a problem hiding this comment.
This is fantastic, thank you for this @elenya-grant and for iterating with @RHammond2!
f872c8b to
e133caf
Compare
5 participants
Generalized functions for getting, setting, and finding environment variables
Generalized the functions in
h2integrate/resource/utilities/nlr_developer_api_keys.pyfor getting, finding, and setting environment variables. The functions innlr_developer_api_keys.pywere only usable for getting, finding, and setting environment variables for downloading resource data from NLR API calls.The generalized functions now live in
h2integrate/core/env_tools.py. The functions for NLR API calls have been updated to use these generalized functions.This PR will make it easier for new environment variables to be introduced and used across models and tools without having to duplicate a bunch of code. Another benefit of this PR is that is has introduced more thorough testing for these methods.
A future PR could utilize this functionality to check if
RESOURCE_DIRis specified as an environment variable.Major shoutout to @RHammond2 who helped a lot with making this code an improvement on the existing API functionality and gave really great feedback throughout!
Section 1: Type of Contribution
Section 2: Draft PR Checklist
TODO:
Type of Reviewer Feedback Requested (on Draft PR)
Structural feedback:
Implementation feedback:
Section 3: General PR Checklist
docs/files are up-to-date, or added when necessaryCHANGELOG.md"A complete thought. [PR XYZ]((https://github.com/NatLabRockies/H2Integrate/pull/XYZ)", where
XYZshould be replaced with the actual number.Section 4: Related Issues
This is intended to resolve Issue #765
Issue #802 was created to note possible follow-on work that could be done once this PR is merged in!
Issue #811 was created to document some broader reaching reviewer feedback that could be resolved in a follow-on PR and should be discussed within the team.
Section 5: Impacted Areas of the Software
Section 5.1: New Files
docs/misc_resources/debugging_environment_variables.md: new doc page that outlines ways to debug environment variables or set them prior to running H2Ih2integrate/core/env_tools.pyget_environment_variables(): main method that is used to return the key/value pairs of specified environment variables where the environment variable value was found (and set any environment variables if specified).load_env_vars_from_file(): loads all environment variables from a file, flexible to different separatorsset_env_var(): method that sets values of environment variables from a dictionaryh2integrate/core/test/test_env_tools.py: new test file for the functions inh2integrate/core/env_tools.pySection 5.2: Modified Files
h2integrate/resource/utilities/nlr_developer_api_keys.py: removed old getter/setter methods and global variables, totally updated to useget_environment_variables()get_nlr_developer_api_credential: general method to get either nlr api key or emailget_nlr_developer_api_email: updated to wrapget_nlr_developer_api_credentialto get API email: updated to wrapget_nlr_developer_api_credential` to get API keydocs/getting_started/environment_variables.md: reformatted/updated to include the EIA API info.Section 6: Additional Supporting Information
Section 7: Test Results, if applicable