You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "jh1231223 (via GitHub)" <gi...@apache.org> on 2025/10/12 08:50:39 UTC
[PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
jh1231223 opened a new pull request, #36485:
URL: https://github.com/apache/beam/pull/36485
Thanks for reviewing!
**Context / Issue**
- Addresses flaky behavior tracked in #35211.
- In `PortableRunnerTest::test_assert_that`, a failing `assert_that` raised on a worker thread was only surfaced as a `PytestUnhandledThreadExceptionWarning`, so the test could pass intermittently.
**What this change does**
- Captures `threading.excepthook` during the test run.
- Wraps `beam.Pipeline.run(...).wait_until_finish()` so that any captured worker-thread exception is rethrown on the main thread at the end of the run.
- Updates `test_assert_that` to use this shim and assert the expected failure via `assertRaisesRegex`, turning the flaky pass into a deterministic failure for the negative case.
**Why**
Ensures the failure path for `assert_that` is exercised reliably and reported as a proper test failure, rather than a warning emitted from a background thread.
**Summary**
The failure originally occurred on a worker thread rather than the main thread, causing pytest to emit a PytestUnhandledThreadExceptionWarning instead of failing the test. This fix intercepts those background-thread exceptions via threading.excepthook and re-throws them on the main thread after wait_until_finish(), so pytest now treats them as proper test failures. The warning no longer appears because the exception is no longer “unhandled,” eliminating the nondeterministic pass/fail behavior.
**Verification**
Stress run x200 (no flakes):
```
for i in {1..200}; do
echo "Run $i"
pytest -q apache_beam/runners/portability/portable_runner_test.py::PortableRunnerTest::test_assert_that \
-W error::pytest.PytestUnhandledThreadExceptionWarning -s --maxfail=1 || break
done
```
Result: `200/200` passes; no thread-exception warnings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36485:
URL: https://github.com/apache/beam/pull/36485#issuecomment-3394140859
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm merged PR #36485:
URL: https://github.com/apache/beam/pull/36485
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on PR #36485:
URL: https://github.com/apache/beam/pull/36485#issuecomment-3394078443
## Summary of Changes
Hello @jh1231223, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves a long-standing flakiness issue in the PortableRunner tests where exceptions originating in worker threads were not properly propagated to the main thread, leading to `PytestUnhandledThreadExceptionWarning` instead of a test failure. The solution involves intercepting these background thread exceptions and re-raising them on the main thread once the pipeline execution concludes. This ensures that test failures are reliably reported, making the test suite more robust and predictable.
### Highlights
* **Flaky Test Fix**: Addresses a known flaky test behavior in `PortableRunnerTest::test_assert_that` (tracked in #35211) where `assert_that` failures on worker threads were only surfaced as warnings.
* **Exception Handling**: Introduces a mechanism to capture exceptions from worker threads using `threading.excepthook`.
* **Pipeline Run Wrapping**: Wraps `beam.Pipeline.run(...).wait_until_finish()` to rethrow any captured worker-thread exceptions on the main thread after the pipeline completes.
* **Test Determinism**: Updates `test_assert_that` to utilize this new exception propagation, converting an intermittently passing test into a deterministic failure for the negative case, thus eliminating flakiness.
<details>
<summary><b>Using Gemini Code Assist</b></summary>
<br>
The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips.
<b>Invoking Gemini</b>
You can request assistance from Gemini at any point by creating a comment using either `/gemini <command>` or `@gemini-code-assist <command>`. Below is a summary of the supported commands on the current page.
Feature | Command | Description
--- | --- | ---
Code Review | `/gemini review` | Performs a code review for the current pull request in its current state.
Pull Request Summary | `/gemini summary` | Provides a summary of the current pull request in its current state.
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help | `/gemini help` | Displays a list of available commands.
<b>Customization</b>
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a `.gemini/` folder in the base of the repository. Detailed instructions can be found [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github).
<b>Limitations & Feedback</b>
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up [here](https://google.qualtrics.com/jfe/form/SV_2cyuGuTWsEw84yG).
<b>You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the [Gemini Code Assist IDE Extension](https://cloud.google.com/products/gemini/code-assist).</b>
</details>
[^1]: Review the [Privacy Notices](https://policies.google.com/privacy), [Generative AI Prohibited Use Policy](https://policies.google.com/terms/generative-ai/use-policy), [Terms of Service](https://policies.google.com/terms), and learn how to configure Gemini Code Assist in GitHub [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Gemini can make mistakes, so double check it and [use code with caution](https://support.google.com/legal/answer/13505487).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #36485:
URL: https://github.com/apache/beam/pull/36485#discussion_r2426051241
##########
sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py:
##########
@@ -112,9 +138,10 @@ def create_pipeline(self, is_drain=False):
def test_assert_that(self):
# TODO: figure out a way for fn_api_runner to parse and raise the
# underlying exception.
- with self.assertRaisesRegex(Exception, 'Failed assert'):
- with self.create_pipeline() as p:
- assert_that(p | beam.Create(['a', 'b']), equal_to(['a']))
+ with patch_portable_runner_for_test(): # pylint: disable=not-context-manager
Review Comment:
Thanks - this is a good find.
With that said, I think that fixing the test is probably the wrong move here. If this test is flaky, it indicates that the underlying runner is not doing the right thing (the runner _should_ fail this test everytime without any patches). So I think we need to address the behavior on the underlying runner, not in the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36485:
URL: https://github.com/apache/beam/pull/36485#issuecomment-3394498028
Assigning reviewers:
R: @damccorm for label python.
Note: If you would like to opt out of this review, comment `assign to next reviewer`.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36485:
URL: https://github.com/apache/beam/pull/36485#issuecomment-3395997758
Reviewers are already assigned to this PR: @damccorm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "jh1231223 (via GitHub)" <gi...@apache.org>.
jh1231223 commented on code in PR #36485:
URL: https://github.com/apache/beam/pull/36485#discussion_r2431092648
##########
sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py:
##########
@@ -112,9 +138,10 @@ def create_pipeline(self, is_drain=False):
def test_assert_that(self):
# TODO: figure out a way for fn_api_runner to parse and raise the
# underlying exception.
- with self.assertRaisesRegex(Exception, 'Failed assert'):
- with self.create_pipeline() as p:
- assert_that(p | beam.Create(['a', 'b']), equal_to(['a']))
+ with patch_portable_runner_for_test(): # pylint: disable=not-context-manager
Review Comment:
Totally agree that the issue isn't the test.
I changed PortableRunner.wait_until_finish() so it re-raises worker errors instead of just logging them on the message stream. Before, failures could get buried in logs and the timing made the run look “successful” sometimes, which caused the flake. Now the error bubbles up, the job fails deterministically, and the test passes without any test changes.
The changes are in portable_runner.py, plus a tiny companion tweak in local_job_service.py.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #36485:
URL: https://github.com/apache/beam/pull/36485#issuecomment-3394116471
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/36485?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.07%. Comparing base ([`fb80584`](https://app.codecov.io/gh/apache/beam/commit/fb8058454c0e97c9fdb2147177e868f883bff3fc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a26f249`](https://app.codecov.io/gh/apache/beam/commit/a26f249aec4425a899ecba5c9f7d4f7db0c6ae0b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #36485 +/- ##
============================================
- Coverage 55.07% 55.07% -0.01%
Complexity 1666 1666
============================================
Files 1059 1059
Lines 165387 165387
Branches 1190 1190
============================================
- Hits 91088 91083 -5
- Misses 72124 72129 +5
Partials 2175 2175
```
| [Flag](https://app.codecov.io/gh/apache/beam/pull/36485/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [python](https://app.codecov.io/gh/apache/beam/pull/36485/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `80.98% <ø> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/beam/pull/36485?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PortableRunner tests: surface worker-thread exceptions on main thread after wait_until_finish() (fixes #35211) [beam]
Posted by "jh1231223 (via GitHub)" <gi...@apache.org>.
jh1231223 commented on PR #36485:
URL: https://github.com/apache/beam/pull/36485#issuecomment-3395995305
assign set of reviewers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org