-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove type coercion of FinalizeWrite
in iobase.py
#33614
base: master
Are you sure you want to change the base?
Conversation
`_finalize_write`'s output type is `TimestampedValue`, not `str`, so when `runtime_type_check` is set to `True` in `TypeOptions`, it would raise a `SimpleTypeHintError` with the following details: `apache_beam.typehints.decorators.TypeCheckError: According to type-hint expected output should be of type <class 'str'>. Instead, received '<apache_beam.transforms.window.TimestampedValue object at 0x7a303057cd10>', an instance of type <class 'apache_beam.transforms.window.TimestampedValue'>`
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Thanks for the fix. There might be something need clarification here. TimestampedValue is a special value type in Beam. If an element wrapped as a TimestampedValue, in downstream the timestamp becomes a metadata for this element, while the element is still the original one, e.g.
it prints the str itself instead of TimestampedValue(1) Seems this convention conflicts with type check also part of Beam SDK. cc: @jrmccluskey @tvalentyn may have a solution here |
Thanks @liaopeiyuan I would create a bug with a reproducible example. Agree with @Abacn that changing the type coercion might not be the best solution here. I am seeing that some logic in type checking machinery that suggests, TimetampedValue instances should be compared by the type they are enveloping:
I would check why that is not happening. |
_finalize_write
's output type isTimestampedValue
, notstr
, so whenruntime_type_check
is set toTrue
inTypeOptions
, it would raise aSimpleTypeHintError
with the following details:apache_beam.typehints.decorators.TypeCheckError: According to type-hint expected output should be of type <class 'str'>. Instead, received '<apache_beam.transforms.window.TimestampedValue object at 0x7a303057cd10>', an instance of type <class 'apache_beam.transforms.window.TimestampedValue'>
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.md
with 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 or the workflows README to see a list of phrases to trigger workflows.