calcDelaysGUNW - added api credentials check and writing locally [Issue: #483] #484

mgovorcin · 2023-02-02T23:59:44Z

Added option to add weather model API credentials and write it locally to a hidden file
Issue: #481

Description

added credentials.py to RAiDER.models with function check_api that check if env API credential file already exists, if not it looks for api: UID and KEY in args and writes it locally. Added options to raider.py **process calcDelysGUNW to add api key and uid

Added args parms to calcDelaysGUNW: --api_uid, --api_key

raider.py ++process calcDelaysGUNW -finput_file -m model -o output --api_uid UID --api_key KEY

NOTE: if API_RC hidden file exists such as ~/.cdsapirc, inserted UID and KEY will be ignored, There is an update flag that overwrites hidden file, but for calcGUNWDelays is set to be False. It can be added if api_key is not None and api_uid is not None: update_flag=True . This is will give priority to user input?

Another option to add API credentials is through ENV variable, if API_UID and API_KEY are None. Credentials.py searches for the env and use them to populate API_RC hidden file: List of environmental API variables:

cdsapirc : models(ERA5, ERA5T):
$RAIDER_ECMWF_ERA5_API_KEY
$RAIDER_ECMWF_ERA5_UID

ecmwfapirc : models(HRES, ERAI):
$RAIDER_HRES_URL
$RAIDER_HRES_API_KEY
$RAIDER_HRES_EMAIL

netrc : models (GMAO)
$EARTHDATA_USERNAME
$EARTHDATA_PASSWORD

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

I have added an explanation of what your changes do and why you'd like us to include them.
I have written new tests for your core changes, as applicable.
I have successfully ran tests with your changes locally.
My change requires a change to the documentation.
I have updated the documentation accordingly.

cmarshak

Looks good.

A few aesthetic things.

Look at Joe's tests and add some tests to the function to make sure you catch errors correctly.

It would also be helpful for ASF development to provide an example in this PR about how this is used. They will want some interface related to AWS:

raider ++process calcGUNW ....

Super helpful, Marin!

tools/RAiDER/models/credentials.py

cmarshak · 2023-02-03T00:15:51Z

tools/RAiDER/models/credentials.py

+            with open(api_filename_path, 'w') as f:
+                f.write(MODEL_API_DICT[api_filename]['api'].format(uid=UID,
+                                                                    key=KEY))


cmarshak · 2023-02-03T00:20:50Z

For the changelog - look at the latest release and the current changelog version in your forked branch (from dev branch).

When I am reading this, the release on conda is 0.4.1. The release you are adding to is 0.4.2 - you would add a brief description there as to what you are adding.

cmarshak · 2023-02-03T00:34:19Z

One more piece that you need to talk to @asjohnston-asf and @jhkennedy about.

They typically write API credentials as environment variables.

Please coordinate with them so that if the weather model is HRES it reads something like os.environ['HRES_API_KEY'].

dbekaert · 2023-02-03T16:16:56Z

tools/RAiDER/aria/prepFromGUNW.py

@@ -231,6 +232,10 @@ def update_yaml(dct_cfg:dict, dst:str='GUNW.yaml'):
 def main(args):
    """ Read parameters needed for RAiDER from ARIA Standard Products (GUNW) """

+    # Check if WEATHER MODEL API credentials hidden file exists, if not create it or raise ERROR
+    credentials.check_api(args.weather_model, args.api_uid, args.api_key,


@mgovorcin @cmarshak I wonder if we should add this add the main riader program. The checking of credentials is applicable to all workflows where a model with API is requested?

@jlmaurer I also wonder is there a way for us to check without doing a downlaod of data if credentials are correct? aka do the ecmwfapi and cdsa api allow for a credential check? It would not be bad if we could error out fast at the top.

I think raider already checks if the credentials exist and has great documentation on how to setup the hidden credential files. Pretty sure errors are very helpful.

I honestly think the existing approach in raider is sufficient/preferred. It aligns well with the existing API libraries. I think passing credentials each time you want to call raider is probably not preferred choice for a user particularly when they share commands/code, they are not going want to share the credentials (just instructions for their team to update their own hidden credential files).

The main thrust of this is being able to pass environment variables that will be defined when a container is created. I don't know the details of this approach except this is a secure, reliable way to pass credentials for this type of processing.

I think this can be included in the main raider workflow. This is why I added the prompt option to remind a user about API credentials in order to use raider.py. The majority of people do not really read all instructions.

This function serves as a double check to see if the hidden file exists, if not it creates it, and it does it only once (the first time). Later on, a user does not need to pass the credentials or think about this anymore. In the case of calcDelayGUNW, prompt_flag will be hardcoded to False, as we do not want to get stuck by waiting for the input.

For the downloading error check, maybe we can add one test dataset to download per model, and try to download it. But I do not know if this is a good option as this will run every time when we wanna run raider. I believe a good approach is to report an error if files cannot be downloaded and point user to check his credentials or use credentials.check_api with update_flag=True (to overwrite the existing hidden file)

Got it, perhaps we could add a seperate script stand-alone from the raider workflow where we do this.
e.g. script --model

the script checks if file exist, if not checks env variables, if nto pings users

then script makes a dummy call to the API to check if credentials work and report out.
Half of the functionally exist with what you have already.
The dummy check i would like to see if there is a potentially handschake only option rather than fetching data.

Inclusion in the main raider code to see if the file exist would be good for that model being requested and error out if needed.

For example our HRES credentials cycle and you will want to catch that early.

mgovorcin · 2023-02-03T22:00:02Z

@cmarshak will add the tests as suggested. As for the passing os.environ['HRES_API_KEY'], maybe this can be done when passing API_UID or API_KEY as

raider.py ++process calcDelaysGUNW - -m HRES -uid $HRES_API_UID -key $HRES_API_KEY

just not sure how to pass env ${MODEL}_API_UID

cmarshak · 2023-02-03T22:07:49Z

just not sure how to pass env ${MODEL}_API_UID

Something like this: https://github.com/ACCESS-Cloud-Based-InSAR/DockerizedTopsApp/blob/dev/isce2_topsapp/__main__.py#L64-L68

mgovorcin · 2023-02-04T01:37:33Z

just not sure how to pass env ${MODEL}_API_UID

Something like this: https://github.com/ACCESS-Cloud-Based-InSAR/DockerizedTopsApp/blob/dev/isce2_topsapp/__main__.py#L64-L68

@cmarshak : I applied all your suggestions. Please check it out and see if you would like to proceed with merging this PR

cmarshak · 2023-02-06T22:30:26Z

@mgovorcin - I am loosing you with how you are setting up your reading of env variables - there seems to be nested dictionaries. I trust you are doing things correctly - just make sure they are consistent with below.

For HRES:

RAIDER_HRES_URL
RAIDER_HRES_API_KEY
RAIDER_HRES_EMAIL

Use these variables exactly as os.environ['RAIDER_HRES_API_KEY'].

For ERA5:

RAIDER_ECMWF_ERA5_API_KEY
RAIDER_ECMWF_ERA5_UID

These are how our secrets will be accessed.

cmarshak · 2023-02-06T23:47:39Z

For the netrc (GMAO), use the environment variables exactly as they are in for the plugin. Please double check with @asjohnston-asf.

mgovorcin · 2023-02-06T23:50:33Z

@mgovorcin - I am loosing you with how you are setting up your reading of env variables - there seems to be nested dictionaries. I trust you are doing things correctly - just make sure they are consistent with below.

For HRES:

RAIDER_HRES_URL

RAIDER_HRES_API_KEY

RAIDER_HRES_EMAIL

Use these variables exactly as os.environ['RAIDER_HRES_API_KEY'].

For ERA5:

RAIDER_ECMWF_ERA5_API_KEY

RAIDER_ECMWF_ERA5_UID

These are how our secrets will be accessed.

@cmarshak I checked cdsapi and ecwmfi and use the ones API's are looking for.

I guess this is to use take into account different credentials for HRES model. I will update it to the ones above

@cmarshak Comments Applied :D

jlmaurer · 2023-02-14T02:51:40Z

@mgovorcin @dbekaert I think this looks good to me!

jhkennedy

This looks great! One tiny suggestion that you can take or leave.

tools/RAiDER/models/credentials.py

cmarshak · 2023-02-14T22:28:49Z

@mgovorcin @jlmaurer - when this is merged, can Raider be released to include these updates? Would it be possible to have this released this week? I am hoping ASF has the release to include ERA5 and HRES via Hyp3.

jhkennedy · 2023-02-14T23:26:21Z

@cmarshak for you to start using it via HyP3, it just needs to be merged to dev as Hyp3 is using the test tagged container for the INSAR_ISCE_TEST job:
https://github.com/ASFHyP3/hyp3/blob/develop/job_spec/INSAR_ISCE_TEST.yml#L85-L86

cmarshak · 2023-02-14T23:29:39Z

Cool - not worried about release then. Thanks for clarifying, Joe.

mgovorcin added 2 commits February 2, 2023 15:49

added options to input api credentials to calcDelaysGUNW

87a393e

added chmod 0600 after writing file

d3cc347

cmarshak requested a review from jhkennedy February 3, 2023 00:04

mgovorcin added 2 commits February 2, 2023 16:10

bug fix

3adea76

little clean up

b7760d8

cmarshak approved these changes Feb 3, 2023

View reviewed changes

dbekaert reviewed Feb 3, 2023

View reviewed changes

cmarshak mentioned this pull request Feb 3, 2023

Update the INSAR_ISCE job definitions for new features from JPL ASFHyP3/hyp3#1446

Open

mgovorcin added 3 commits February 3, 2023 15:42

did little clean up

00ed6bb

clean up

65be8e5

clean up 2

9b66e73

update change log, added tests

d57639c

mgovorcin added 2 commits February 6, 2023 17:51

changed os with pathlib, and envs

1451b21

typo

2aad714

jhkennedy approved these changes Feb 14, 2023

View reviewed changes

tools/RAiDER/models/credentials.py Outdated Show resolved Hide resolved

mgovorcin and others added 3 commits February 14, 2023 10:10

Merge branch 'dev' into dev

d68ba5c

final fixes

547ea1a

change log

4de70d9

dbekaert merged commit eaac0b8 into dbekaert:dev Feb 16, 2023

dbekaert mentioned this pull request Feb 16, 2023

[Feature/Enhancement] Use ENV Variables for HRES and ERA5 credentials in GUNW entrypoint #483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

calcDelaysGUNW - added api credentials check and writing locally [Issue: #483] #484

calcDelaysGUNW - added api credentials check and writing locally [Issue: #483] #484

mgovorcin commented Feb 2, 2023 •

edited

Loading

cmarshak left a comment •

edited

Loading

cmarshak Feb 3, 2023

cmarshak commented Feb 3, 2023 •

edited

Loading

cmarshak commented Feb 3, 2023

dbekaert Feb 3, 2023

dbekaert Feb 3, 2023 •

edited

Loading

cmarshak Feb 3, 2023 •

edited

Loading

mgovorcin Feb 3, 2023 •

edited

Loading

dbekaert Feb 3, 2023

mgovorcin commented Feb 3, 2023

cmarshak commented Feb 3, 2023

mgovorcin commented Feb 4, 2023 •

edited

Loading

cmarshak commented Feb 6, 2023 •

edited

Loading

cmarshak commented Feb 6, 2023

mgovorcin commented Feb 6, 2023 •

edited

Loading

jlmaurer commented Feb 14, 2023

jhkennedy left a comment

cmarshak commented Feb 14, 2023

jhkennedy commented Feb 14, 2023

cmarshak commented Feb 14, 2023

calcDelaysGUNW - added api credentials check and writing locally [Issue: #483] #484

calcDelaysGUNW - added api credentials check and writing locally [Issue: #483] #484

Conversation

mgovorcin commented Feb 2, 2023 • edited Loading

Description

Type of change

Checklist:

cmarshak left a comment • edited Loading

Choose a reason for hiding this comment

cmarshak Feb 3, 2023

Choose a reason for hiding this comment

cmarshak commented Feb 3, 2023 • edited Loading

cmarshak commented Feb 3, 2023

dbekaert Feb 3, 2023

Choose a reason for hiding this comment

dbekaert Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

cmarshak Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

mgovorcin Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

dbekaert Feb 3, 2023

Choose a reason for hiding this comment

mgovorcin commented Feb 3, 2023

cmarshak commented Feb 3, 2023

mgovorcin commented Feb 4, 2023 • edited Loading

cmarshak commented Feb 6, 2023 • edited Loading

cmarshak commented Feb 6, 2023

mgovorcin commented Feb 6, 2023 • edited Loading

jlmaurer commented Feb 14, 2023

jhkennedy left a comment

Choose a reason for hiding this comment

cmarshak commented Feb 14, 2023

jhkennedy commented Feb 14, 2023

cmarshak commented Feb 14, 2023

mgovorcin commented Feb 2, 2023 •

edited

Loading

cmarshak left a comment •

edited

Loading

cmarshak commented Feb 3, 2023 •

edited

Loading

dbekaert Feb 3, 2023 •

edited

Loading

cmarshak Feb 3, 2023 •

edited

Loading

mgovorcin Feb 3, 2023 •

edited

Loading

mgovorcin commented Feb 4, 2023 •

edited

Loading

cmarshak commented Feb 6, 2023 •

edited

Loading

mgovorcin commented Feb 6, 2023 •

edited

Loading