Fail when main session cannot be loaded. #25617
Conversation
|
Run Python_Transforms PreCommit |
|
Run PythonFormatter PreCommit |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Codecov Report
@@ Coverage Diff @@
## master #25617 +/- ##
=======================================
Coverage 72.81% 72.81%
=======================================
Files 775 775
Lines 102928 102934 +6
=======================================
+ Hits 74948 74956 +8
+ Misses 26525 26523 -2
Partials 1455 1455
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Looks great to me - just left two suggestions inline. Thanks for the super-quick turnaround on this @tvalentyn!
| @@ -120,9 +121,17 @@ def create_harness(environment, dry_run=False): | |||
| 'Could not load main session: %s', exception_details, exc_info=True) | |||
| raise | |||
There was a problem hiding this comment.
Would it also be useful to set deferred_exception here instead raising this CorruptMainSessionException directly?
There was a problem hiding this comment.
are such errors transient or permanent?
There was a problem hiding this comment.
from a comment on your PR introducing this exception I assumed these were transient, if so, we can keep it as is to create less noise.
| exception_details = traceback.format_exc() | ||
| _LOGGER.error( | ||
| 'Could not load main session: %s', exception_details, exc_info=True) | ||
| deferred_exception = CorruptMainSessionException( |
There was a problem hiding this comment.
Not sure if CorruptMainSessionException is the best exception type to use here, as the main session file isn't actually corrupted in this case. Would ImportError be appropriate?
| deferred_exception = CorruptMainSessionException( | |
| deferred_exception = ImportError( |
There was a problem hiding this comment.
How about we rename it to LoadMainSessionException to fit both scenarios.
tvalentyn
force-pushed
the
main_session_error
branch
from
ae21439
to
a384f49
Compare
|
Run Portable_Python PreCommit |
|
Catching up on this - thanks for merging @tvalentyn! Appreciate the super quick turnaround. 👍🏻 |
Sometimes when users don't install runtime dependencies, main session cannot load. Fail with an error message when this happens.
To make sure the error is reported to a runner, delay the failure until SDK startes to process bundles. This mechanism could potentially be reused for other worker startup misconfigurations.
Addresses #25401
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.