Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More workflow fixes #631

Merged
merged 14 commits into from
May 29, 2024

Conversation

robertbartel
Copy link
Contributor

Additional fixes throughout the model exec workflow found in testing.

@robertbartel robertbartel added bug Something isn't working maas MaaS Workstream labels May 22, 2024
@robertbartel robertbartel force-pushed the bug/workflow_fixes_02/main branch from d190b83 to 7ba6eed Compare May 22, 2024 20:36
Copy link
Member

@aaraney aaraney left a comment

Choose a reason for hiding this comment

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

Generally, looks great! I left two minor comments that shouldnt take long to address. Thanks, @robertbartel!

try:
return await self._process_request(request=request)
except DmodRuntimeError as e:
raise DmodRuntimeError(f"DMOD error when getting dataset state: {str(e)}")
Copy link
Member

Choose a reason for hiding this comment

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

To help provide more context, let's use raise from.

Suggested change
raise DmodRuntimeError(f"DMOD error when getting dataset state: {str(e)}")
raise DmodRuntimeError(f"DMOD error when getting dataset state: {str(e)}") from e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we get this for free without the explicit usage, since we're inside the handling except block?

Copy link
Member

Choose a reason for hiding this comment

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

Yes and no. The call stack will propagate like you are suggesting, however the from keyword is giving context that the exception we are raising now is a result of, in this case, e. So, the stack trace will be more appropriately formatted. For example:

def raises():
    raise RuntimeError("some runtime exception")
def normal_catch_and_reraise():
    try:
        raises()
    except Exception as e:
        raise RuntimeError(f"normal catch and reraise: {e}")

normal_catch_and_reraise()

Traceback (most recent call last):
  File "/home/user/e.py", line 6, in normal_catch_and_reraise
    raises()
  File "/home/user/e.py", line 2, in raises
    raise RuntimeError("some runtime exception")
RuntimeError: some runtime exception

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/user/e.py", line 16, in <module>
    normal_catch_and_reraise()
  File "/home/user/e.py", line 8, in normal_catch_and_reraise
    raise RuntimeError(f"normal catch and reraise: {e}")
RuntimeError: normal catch and reraise: some runtime exception
def catch_and_reraise_from():
    try:
        raises()
    except Exception as e:
        raise RuntimeError("catch and reraise from") from e

catch_and_reraise_from()

Traceback (most recent call last):
  File "/home/user/e.py", line 12, in catch_and_reraise_from
    raises()
  File "/home/user/e.py", line 2, in raises
    raise RuntimeError("some runtime exception")
RuntimeError: some runtime exception

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/user/e.py", line 16, in <module>
    catch_and_reraise_from()
  File "/home/user/e.py", line 14, in catch_and_reraise_from
    raise RuntimeError("catch and reraise from") from e
RuntimeError: catch and reraise from

Notice the presence of The above exception was the direct cause of the following exception: in the second stack trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. Reviewing the documentation (too quickly) yesterday, I read the part about an implicit mechanism when already being handled, and missed where that actually involves a different exception object attribute. I've made a change, so please re-review when you have a moment.

@robertbartel robertbartel force-pushed the bug/workflow_fixes_02/main branch from 7ba6eed to 0ebac5e Compare May 28, 2024 17:54
aaraney
aaraney previously approved these changes May 28, 2024
@robertbartel robertbartel merged commit e5aa5d5 into NOAA-OWP:master May 29, 2024
8 checks passed
@robertbartel robertbartel deleted the bug/workflow_fixes_02/main branch May 29, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working maas MaaS Workstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants