Skip to content

Check for grpc status details before indexing & unpacking #801

New issue

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

By clicking “No 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? No Sign in to your account

Merged
merged 3 commits into from
Mar 27, 2025

Conversation

THardy98
Copy link
Contributor

DWISOTT, fixes bug

  1. Closes [Bug] We assume an element is present in an error details list #791

  2. How was this tested:
    Added small integration test, mock RPC error using an interceptor.

  3. Any docs updates needed?
    No

@THardy98 THardy98 requested a review from a team as a code owner March 26, 2025 22:25
@CLAassistant
Copy link

CLAassistant commented Mar 26, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.



# Verify correcting issue #791
async def test_start_update_with_start_empty_details_interceptor(client: Client):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing this test pass with the changes to temporalio/client.py reverted.

uv run pytest -s tests/worker/test_update_with_start.py::test_start_update_with_start_empty_details_interceptor

Is what I'm running.

Copy link
Contributor Author

@THardy98 THardy98 Mar 27, 2025

Choose a reason for hiding this comment

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

Agh - the interceptor raises an error before we make any calls or run any handlers... so we're testing nothing but the test code 🤦

I've replaced this by instead mocking the workflow service call. It does indeed fail without the details check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Maybe this is a nit but did you look into making it not print out the exception below? I find that irrelevant stack traces are one of the main things that make it hard to understand CI logs, so it would be nice if we could avoid it.

tests/worker/test_update_with_start.py .

================================================================================================= 1 passed in 0.54s ==================================================================================================
Future exception was never retrieved
future: <Future finished exception=RPCError('empty details')>
Traceback (most recent call last):
  File "/Users/dan/src/temporalio/sdk-python/tests/worker/test_update_with_start.py", line 843, in test_start_update_with_start_empty_details
    await client.start_update_with_start_workflow(
  File "/Users/dan/src/temporalio/sdk-python/temporalio/client.py", line 1048, in start_update_with_start_workflow
    return await self._start_update_with_start(
  File "/Users/dan/src/temporalio/sdk-python/temporalio/client.py", line 1120, in _start_update_with_start
    return await self._impl.start_update_with_start_workflow(input)
  File "/Users/dan/src/temporalio/sdk-python/temporalio/client.py", line 6113, in start_update_with_start_workflow
    raise err
  File "/Users/dan/src/temporalio/sdk-python/temporalio/client.py", line 6059, in start_update_with_start_workflow
    return await self._start_workflow_update_with_start(
  File "/Users/dan/src/temporalio/sdk-python/temporalio/client.py", line 6149, in _start_workflow_update_with_start
    await self._client.workflow_service.execute_multi_operation(multiop_req)
  File "/Users/dan/src/temporalio/sdk-python/tests/worker/test_update_with_start.py", line 836, in __call__
    raise self.empty_details_err
temporalio.service.RPCError: empty details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent a bit of time, but was satisfied when poe test didn't produce this - only when running the single test, but I'll take another look

Copy link
Contributor Author

@THardy98 THardy98 Mar 27, 2025

Choose a reason for hiding this comment

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

can't seem to fix this, but that's probably my lack of Python speaking
we await the update call so I'm not sure where the future is coming from, and my understanding was that pytest.raises catches expected errors raised from the code block nested within it, so double misunderstanding for me

@cretz my feeling is that this is something that could be fixed easily, if it isn't, any opposition to just suppressing the warning?
I'd rather not spend more time mentally walking in circles

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, no worries! It wasn't obvious to me either. Definitely not blocking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that said, I think I've got a solution. This was helpful:

    loop = asyncio.get_event_loop()
    loop.set_debug(True)

#804

@THardy98 THardy98 force-pushed the fix_update_index_error branch from c9e6fd7 to 3de4acf Compare March 27, 2025 18:11
@THardy98 THardy98 requested a review from dandavison March 27, 2025 19:05
@THardy98 THardy98 merged commit d219870 into main Mar 27, 2025
14 checks passed
@THardy98 THardy98 deleted the fix_update_index_error branch March 27, 2025 22:27
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] We assume an element is present in an error details list
3 participants