Skip to content
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

Fix FlatMapTuple typehint bug #33307

Merged
merged 13 commits into from
Dec 6, 2024
Merged

Fix FlatMapTuple typehint bug #33307

merged 13 commits into from
Dec 6, 2024

Conversation

hjtran
Copy link
Contributor

@hjtran hjtran commented Dec 6, 2024

(fixes #33014)
FlatMapTuple throws false positive type hint errors in certain situations, e.g.

 def identity(x: Tuple[str, int]) -> Tuple[str, int]:
    return x
  with beam.Pipeline() as p:
    with beam.Pipeline() as p:
      A = (p
                | "Generate input" >> beam.Create([('P1', [2])])
                | "Flat" >> beam.FlatMapTuple(lambda k, vs: [(k, v) for v in vs])
                | "Identity" >> beam.Map(identity))

raises the following exception:

E         apache_beam.typehints.decorators.TypeCheckError: The transform 'Identity' requires PCollections of type 'Tuple[<class 'str'>, <class 'int'>]' but was applied to a PCollection of type 'Tuple[<class 'type'>, <class 'int'>]' (produced by the transform 'Flat').

This is a downstream effect of a bug in the trivial_inference.py code. I couldn't really figure out the underlying bug but we can work around it by adding a tuple coercion before unpacking in FlatMapTuple.

I've created a spinoff issue for the underlying inference issue: #33308

@github-actions github-actions bot added the python label Dec 6, 2024
@hjtran hjtran changed the title Flatmaptuplebug Fix FlatMapTuple typehint bug #33104 Dec 6, 2024
@hjtran hjtran changed the title Fix FlatMapTuple typehint bug #33104 Fix FlatMapTuple typehint bug Dec 6, 2024
@hjtran
Copy link
Contributor Author

hjtran commented Dec 6, 2024

R: @jrmccluskey

Copy link
Contributor

github-actions bot commented Dec 6, 2024

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

Copy link
Contributor

@jrmccluskey jrmccluskey left a comment

Choose a reason for hiding this comment

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

LGTM mod the linting fixes

str getting turned into a generic type is interesting, I will see what I can dig up next week as far as the underlying bug

@hjtran hjtran marked this pull request as ready for review December 6, 2024 17:18
@hjtran
Copy link
Contributor Author

hjtran commented Dec 6, 2024

I can't seem to get this isorted correctly. It's not a part of the pre-commit hook and the isort version isn't compatible with my py3.11. @jrmccluskey would you mind doing it before merging if it's easy for you env?

@liferoad
Copy link
Collaborator

liferoad commented Dec 6, 2024

Have you tried to manually change the import orders?

2024-12-06T18:03:21.6023142Z ERROR: /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python/apache_beam/typehints/trivial_inference_test.py Imports are incorrectly sorted.
2024-12-06T18:03:21.6025546Z Command exited with non-zero status 1
2024-12-06T18:03:21.6026715Z 613.13user 32.61system 1:24.90elapsed 760%CPU (0avgtext+0avgdata 874112maxresident)k
2024-12-06T18:03:21.6028325Z 10928inputs+1120outputs (8major+2025317minor)pagefaults 0swaps
2024-12-06T18:03:21.6030229Z --- /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python/apache_beam/typehints/trivial_inference_test.py:before	2024-12-06 17:57:32.050442
2024-12-06T18:03:21.6033655Z +++ /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python/apache_beam/typehints/trivial_inference_test.py:after	2024-12-06 18:03:19.361962
2024-12-06T18:03:21.6035359Z @@ -21,8 +21,8 @@
2024-12-06T18:03:21.6035826Z  
2024-12-06T18:03:21.6036256Z  import types
2024-12-06T18:03:21.6036738Z  import unittest
2024-12-06T18:03:21.6037179Z +
2024-12-06T18:03:21.6037906Z  import apache_beam as beam
2024-12-06T18:03:21.6038613Z -
2024-12-06T18:03:21.6039092Z  from apache_beam.typehints import row_type
2024-12-06T18:03:21.6039735Z  from apache_beam.typehints import trivial_inference
2024-12-06T18:03:21.6040397Z  from apache_beam.typehints import typehints
2024-12-06T18:03:21.6041736Z ERROR: /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python/apache_beam/typehints/typed_pipeline_test.py Imports are incorrectly sorted.
2024-12-06T18:03:21.6043773Z --- /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python/apache_beam/typehints/typed_pipeline_test.py:before	2024-12-06 17:57:32.054441
2024-12-06T18:03:21.6045808Z +++ /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python/apache_beam/typehints/typed_pipeline_test.py:after	2024-12-06 18:03:19.484289
2024-12-06T18:03:21.6047036Z @@ -22,8 +22,8 @@
2024-12-06T18:03:21.6047434Z  import typing
2024-12-06T18:03:21.6047893Z  import unittest
2024-12-06T18:03:21.6048591Z  from typing import Tuple
2024-12-06T18:03:21.6049002Z +
2024-12-06T18:03:21.6049370Z  import apache_beam as beam
2024-12-06T18:03:21.6049765Z -
2024-12-06T18:03:21.6050178Z  from apache_beam import pvalue
2024-12-06T18:03:21.6050718Z  from apache_beam import typehints
2024-12-06T18:03:21.6051435Z  from apache_beam.options.pipeline_options import OptionsContext
2024-12-06T18:03:21.6053182Z lint: exit 1 (84.91 seconds) /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python> time /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python/scripts/run_pylint.sh pid=1228
2024-12-06T18:03:21.6055705Z lint: commands_post[0]> bash /runner/_work/beam/beam/sdks/python/test-suites/tox/pycommon/build/srcs/sdks/python/scripts/run_tox_cleanup.sh

@liferoad liferoad merged commit 1712964 into apache:master Dec 6, 2024
92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: FlatMapTuple with certain callables can result in wrong type hints
3 participants