Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Creates secret_manager module and adds read_secret #6

Merged
merged 4 commits into from
Feb 25, 2022

Conversation

desertaxle
Copy link
Member

No description provided.

@desertaxle desertaxle requested a review from ahuang11 February 25, 2022 17:59
@desertaxle desertaxle changed the title Creates secret_manager module and adds read_secret Creates secret_manager module and adds read_secret Feb 25, 2022
get_secret_value = partial(client.get_secret_value, **get_secret_value_kwargs)
response = await to_thread.run_sync(get_secret_value)
except ClientError:
logger.exception("Unable to get value for secret %s", secret_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work with just: raise ValueError(f"Unable to get value for secret {secret_name}")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is a good opportunity to talk about how we want to handle exceptions in shared tasks. I generally will allow exceptions to bubble up because they will contain more information about how to resolve the error and we don't have to code for all possible error cases. What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't raise still create a full traceback?

def test():
    try:
        1 / 0
    except Exception:
        print("Failed")
        raise

test()

results in

---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
Input In [6], in <module>
      5         print("Failed")
      6         raise
----> 8 test()

Input In [6], in test()
      1 def test():
      2     try:
----> 3         1 / 0
      4     except Exception:
      5         print("Failed")

ZeroDivisionError: division by zero

vs

def test():
    try:
        1 / 0
    except Exception:
        raise ZeroDivisionError("Failed")

test()

results in

---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
Input In [7], in test()
      2 try:
----> 3     1 / 0
      4 except Exception:

ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

ZeroDivisionError                         Traceback (most recent call last)
Input In [7], in <module>
      4     except Exception:
      5         raise ZeroDivisionError("Failed")
----> 7 test()

Input In [7], in test()
      3     1 / 0
      4 except Exception:
----> 5     raise ZeroDivisionError("Failed")

ZeroDivisionError: Failed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although maybe I should try with orion + logger

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the first traceback that you posted. I think that raising another exception during error handling tends to obfuscate the original exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original version:

from prefect import flow
from prefect import get_run_logger

@flow
def test():
    logger = get_run_logger()
    try:
        1 / 0
    except Exception:
        logger.exception("Failed")
        raise

test()
11:18:50.838 | INFO    | prefect.engine - Created flow run 'vigorous-kestrel' for flow 'test'
11:18:50.839 | INFO    | Flow run 'vigorous-kestrel' - Using task runner 'ConcurrentTaskRunner'
11:18:50.853 | ERROR   | Flow run 'vigorous-kestrel' - Failed
Traceback (most recent call last):
  File "/var/folders/tb/fksvh8ms2hl5xnzwrftnx66r0000gq/T/ipykernel_59130/4129042268.py", line 8, in test
    1 / 0
ZeroDivisionError: division by zero
11:18:50.854 | ERROR   | Flow run 'vigorous-kestrel' - Encountered exception during execution:
Traceback (most recent call last):
  File "/Users/andrew/orion/src/prefect/engine.py", line 445, in orchestrate_flow_run
    result = await run_sync_in_worker_thread(flow_call)
  File "/Users/andrew/orion/src/prefect/utilities/asyncio.py", line 51, in run_sync_in_worker_thread
    return await anyio.to_thread.run_sync(context.run, call, cancellable=True)
  File "/Users/andrew/mambaforge/envs/azure/lib/python3.9/site-packages/anyio/to_thread.py", line 28, in run_sync
    return await get_asynclib().run_sync_in_worker_thread(func, *args, cancellable=cancellable,
  File "/Users/andrew/mambaforge/envs/azure/lib/python3.9/site-packages/anyio/_backends/_asyncio.py", line 818, in run_sync_in_worker_thread
    return await future
  File "/Users/andrew/mambaforge/envs/azure/lib/python3.9/site-packages/anyio/_backends/_asyncio.py", line 754, in run
    result = context.run(func, *args)
  File "/var/folders/tb/fksvh8ms2hl5xnzwrftnx66r0000gq/T/ipykernel_59130/4129042268.py", line 8, in test
    1 / 0
ZeroDivisionError: division by zero
11:18:50.872 | ERROR   | Flow run 'vigorous-kestrel' - Finished in state Failed('Flow run encountered an exception.')
Failed(message='Flow run encountered an exception.', type=FAILED, result=ZeroDivisionError('division by zero'), flow_run_id=3af5dbf4-0dea-4221-a513-985b4bcf48e9)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The raise version with {e}

@flow
def test():
    logger = get_run_logger()
    try:
        1 / 0
    except Exception as e:
        raise ZeroDivisionError(f"<ADDITIONAL HELPER INFO>: {e}")

test()
11:18:32.612 | INFO    | prefect.engine - Created flow run 'primitive-jerboa' for flow 'test'
11:18:32.612 | INFO    | Flow run 'primitive-jerboa' - Using task runner 'ConcurrentTaskRunner'
11:18:32.629 | ERROR   | Flow run 'primitive-jerboa' - Encountered exception during execution:
Traceback (most recent call last):
  File "/var/folders/tb/fksvh8ms2hl5xnzwrftnx66r0000gq/T/ipykernel_59130/2472474932.py", line 5, in test
    1 / 0
ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/andrew/orion/src/prefect/engine.py", line 445, in orchestrate_flow_run
    result = await run_sync_in_worker_thread(flow_call)
  File "/Users/andrew/orion/src/prefect/utilities/asyncio.py", line 51, in run_sync_in_worker_thread
    return await anyio.to_thread.run_sync(context.run, call, cancellable=True)
  File "/Users/andrew/mambaforge/envs/azure/lib/python3.9/site-packages/anyio/to_thread.py", line 28, in run_sync
    return await get_asynclib().run_sync_in_worker_thread(func, *args, cancellable=cancellable,
  File "/Users/andrew/mambaforge/envs/azure/lib/python3.9/site-packages/anyio/_backends/_asyncio.py", line 818, in run_sync_in_worker_thread
    return await future
  File "/Users/andrew/mambaforge/envs/azure/lib/python3.9/site-packages/anyio/_backends/_asyncio.py", line 754, in run
    result = context.run(func, *args)
  File "/var/folders/tb/fksvh8ms2hl5xnzwrftnx66r0000gq/T/ipykernel_59130/2472474932.py", line 7, in test
    raise ZeroDivisionError(f"<ADDITIONAL HELPER INFO>: {e}")
ZeroDivisionError: <ADDITIONAL HELPER INFO>: division by zero
11:18:32.646 | ERROR   | Flow run 'primitive-jerboa' - Finished in state Failed('Flow run encountered an exception.')
Failed(message='Flow run encountered an exception.', type=FAILED, result=ZeroDivisionError('<ADDITIONAL HELPER INFO>: division by zero'), flow_run_id=d247c848-4551-4339-9e69-9962236480b5)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which of those do you prefer? Log + raise or raise new error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log + raise seems cleaner.

tests/test_secrets_manager.py Outdated Show resolved Hide resolved
tests/test_secrets_manager.py Outdated Show resolved Hide resolved
tests/test_secrets_manager.py Outdated Show resolved Hide resolved
tests/test_secrets_manager.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ahuang11 ahuang11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aws_credentials: AwsCredentials needs documentation

@desertaxle
Copy link
Member Author

aws_credentials: AwsCredentials needs documentation

It's documented now!

@desertaxle desertaxle merged commit 95cf08e into main Feb 25, 2022
@desertaxle desertaxle deleted the secrets-manager branch February 25, 2022 20:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants