Repository navigation
Conversation
GitHub Actions
Inactive
b0c2502 to
9856a34
Compare
Measure the whole update in UTF-8 bytes against 1,048,576 when it carries an error, instead of only the error object against 32,768. Include the error payload in history events only when execution data is requested.
9856a34 to
a1cc27d
Compare
yaythomas
left a comment
yaythomas
left a comment
There was a problem hiding this comment.
The new boundary tests make the size rule clear. The history changes omit error payloads when callers exclude execution data. I found issues in Unicode handling, successful-result handling, type checking, and the wait-history test. I think these need changes before approval. The commit subject also exceeds the repository’s 50-character limit.
| if len(payload) > MAX_ERROR_PAYLOAD_SIZE_BYTES: | ||
| msg: str = f"Error object size must be less than {MAX_ERROR_PAYLOAD_SIZE_BYTES} bytes." | ||
| payload = json.dumps( | ||
| update.to_dict(), separators=(",", ":"), ensure_ascii=False |
There was a problem hiding this comment.
With ensure_ascii=False, an unpaired surrogate in an exception message remains in payload. payload_size_bytes() then raises UnicodeEncodeError because it uses strict UTF-8 encoding.
I reproduced this with ErrorObject.from_message("No such file: \udcff"). Can you make the byte-count helper handle this string without raising and add this case to checkpoint_test.py? Using errors="replace" is one option.
| assert error is not None | ||
| assert error.truncated is (not include) | ||
| if include: | ||
| assert error.payload.message == "boom" |
There was a problem hiding this comment.
EventError.payload has type ErrorObject | None. Mypy rejects the direct .message access on this line. Can you assert error.payload is not None before checking its message?
| _assert_error_redaction(getattr(event, event_details_attr).error, include) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("include", [True, False]) |
There was a problem hiding this comment.
This test passes a CANCEL update directly to Event.create_wait_event(). Executor.get_execution_history() does not pass that update when it builds a WaitCancelled event. So this test does not cover the behavior returned by get_execution_history().
Can you replace this with a test that exercises get_execution_history() and asserts the returned WaitCancelled event?
There was a problem hiding this comment.
EventResult.from_durable_execution_invocation_output() returns durable_execution_invocation_output.result even when include=False. I ran a successful execution and requested its history with include_execution_data=False. The returned ExecutionSucceededDetails.Result still contained the result payload.
Can you apply the include check to this factory? Can you also add an end-to-end test through get_execution_history() that covers successful and failed executions with the flag enabled and disabled?
|
The commit subject contains 70 characters. The repository allows at most 50. The commit-validation job therefore fails. Can you shorten the subject to 50 characters or fewer? |
Issue #, if available:
N/A
Description of changes:
Fix two places where the Python local runner behaves differently than the service.
validators/checkpoint.pymodel.pyTruncated: true.Truncated: trueeven when data was requested.And updated tests to match new behaviour.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.