You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "jrmccluskey (via GitHub)" <gi...@apache.org> on 2024/05/03 15:14:29 UTC
[PR] Fix trivial inference tests for Python 3.12 support [beam]
jrmccluskey opened a new pull request, #31170:
URL: https://github.com/apache/beam/pull/31170
Implements new operations for trivial inference ahead of Python 3.12 support to fix existing unit tests. These operations can be seen listed at https://docs.python.org/3/library/dis.html
Not every operation has been implemented, but not every operation comes up in the context of us doing type introspection on functions. A few of these operations were actually introduced in 3.11 and weren't used in our test cases until 3.12!
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `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, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589743990
##########
sdks/python/apache_beam/typehints/opcodes.py:
##########
@@ -347,6 +388,8 @@ def load_attr(state, arg):
Will replace with Any for builtin methods, but these don't have bytecode in
CPython so that's okay.
"""
+ if (sys.version_info.major, sys.version_info.minor) >= (3, 12):
+ arg = arg >> 1
Review Comment:
linked
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589726617
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -641,6 +654,18 @@ def infer_return_type_func(f, input_types, debug=False, depth=0):
# No-op introduced in 3.11. Without handling this some
# instructions have functionally > 2 byte size.
pass
+ elif opname == 'RETURN_CONST':
+ # Introduced in 3.12. Handles returning constants directly
+ # instead of having a LOAD_CONST before a RETURN_VALUE.
+ returns.add(state.const_type(arg))
+ state = None
+ elif opname == 'CALL_INTRINSIC_1':
+ int_op = intrinsic_one_ops.int_one_ops[arg]
+ if debug:
+ print("Executing intrinsic one op", int_op.__name__.upper())
+ int_op(state, arg)
+ elif opname == 'CALL_INTRINSIC_2':
Review Comment:
I'm inclined to think I can remove the intrinsic 2 call block here for now so if there's a failure later we will get the clear "cannot handle operation x" failure. I think this was a holdover from when I was anticipating having to implement most of the new calls to get tests green.
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589735777
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -143,6 +143,8 @@ def copy(self):
return FrameState(self.f, self.vars, self.stack, self.kw_names)
def const_type(self, i):
+ print(self.co.co_consts)
Review Comment:
Yeah, leftover from some checking. This weirdly broke a test in the snippets directory that relies on stdout, which is maybe bad test design but did a good job catching these
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589470721
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -143,6 +143,8 @@ def copy(self):
return FrameState(self.f, self.vars, self.stack, self.kw_names)
def const_type(self, i):
+ print(self.co.co_consts)
Review Comment:
leftover / `if debug` ?
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -432,6 +433,8 @@ def infer_return_type_func(f, input_types, debug=False, depth=0):
elif op in dis.haslocal:
print('(' + co.co_varnames[arg] + ')', end=' ')
elif op in dis.hascompare:
+ if (sys.version_info.major, sys.version_info.minor) >= (3, 12):
+ arg = arg >> 4
Review Comment:
for my knowledge, where does this come from? Perhaps there is a link you could add in a comment.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
Review Comment:
Py2 docs shouldn't be relevant anymore.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
+FrameState object, the second the integer opcode argument.
+
+Bytecodes with more complicated behavior (e.g. modifying the program counter)
+are handled inline rather than here.
+
+For internal use only; no backwards-compatibility guarantees.
+"""
+# pytype: skip-file
+
+from . import opcodes
+
+
+def intrinsic_1_invalid(state, arg):
+ pass
+
+
+def intrinsic_print(state, arg):
+ pass
+
+
+def intrinsic_import_star(state, arg):
+ pass
+
+
+def intrinsic_stopiteration_error(state, arg):
+ pass
+
+
+def intrinsic_async_gen_wrap(state, arg):
+ pass
+
+
+def intrinsic_unary_positive(state, arg):
+ opcodes.unary_positive(state, arg)
+ pass
+
+
+def intrinsic_list_to_tuple(state, arg):
+ opcodes.list_to_tuple(state, arg)
+ pass
+
+
+def intrinsic_typevar(state, arg):
+ pass
+
+
+def intrinsic_paramspec(state, arg):
+ pass
+
+
+def intrinsic_typevartuple(state, arg):
+ pass
+
+
+def intrinsic_subscript_generic(state, arg):
+ pass
+
+
+def intrinsic_typealias(state, arg):
+ pass
+
+
+int_one_ops = [
Review Comment:
Could you add a comment what defines the order of ops in this sequence? How can we make sure that this list stays up to date with future Python versions potentially adding more intrinsic ops? I worry that the order might become skewed but tests we won't notice the skew.
Is it possible to add a unit test that runs on Python 3.12+ which would fail if INT_ONE_OPS doesn't match the list of intrinsic ops known to `dis`? If that's not easy, maybe a unit test that would just fail on Python 3.13 with an instruction to: revisit intrinsic_one_ops.INT_ONE_OPS, make necessary changes, and change the test to fail starting next Python minor version. Then, it will make use manually look at this list once in a while.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
+FrameState object, the second the integer opcode argument.
+
+Bytecodes with more complicated behavior (e.g. modifying the program counter)
+are handled inline rather than here.
+
+For internal use only; no backwards-compatibility guarantees.
+"""
+# pytype: skip-file
+
+from . import opcodes
+
+
+def intrinsic_1_invalid(state, arg):
+ pass
+
+
+def intrinsic_print(state, arg):
+ pass
+
+
+def intrinsic_import_star(state, arg):
+ pass
+
+
+def intrinsic_stopiteration_error(state, arg):
+ pass
+
+
+def intrinsic_async_gen_wrap(state, arg):
+ pass
+
+
+def intrinsic_unary_positive(state, arg):
+ opcodes.unary_positive(state, arg)
+ pass
+
+
+def intrinsic_list_to_tuple(state, arg):
+ opcodes.list_to_tuple(state, arg)
+ pass
+
+
+def intrinsic_typevar(state, arg):
+ pass
+
+
+def intrinsic_paramspec(state, arg):
+ pass
+
+
+def intrinsic_typevartuple(state, arg):
+ pass
+
+
+def intrinsic_subscript_generic(state, arg):
+ pass
+
+
+def intrinsic_typealias(state, arg):
+ pass
+
+
+int_one_ops = [
Review Comment:
```suggestion
INT_ONE_OPS = tuple([
```
(to make it immutable and and to show it's a constant.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
Review Comment:
```suggestion
https://docs.python.org/3/library/dis.html . The first argument is a (mutable)
```
(to make it clickable)
##########
sdks/python/apache_beam/typehints/opcodes.py:
##########
@@ -347,6 +388,8 @@ def load_attr(state, arg):
Will replace with Any for builtin methods, but these don't have bytecode in
CPython so that's okay.
"""
+ if (sys.version_info.major, sys.version_info.minor) >= (3, 12):
+ arg = arg >> 1
Review Comment:
for my knowledge, where does this come from? Perhaps there is a link you could add in a comment.
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
+FrameState object, the second the integer opcode argument.
+
+Bytecodes with more complicated behavior (e.g. modifying the program counter)
+are handled inline rather than here.
+
+For internal use only; no backwards-compatibility guarantees.
+"""
+# pytype: skip-file
+
+from . import opcodes
+
+
+def intrinsic_1_invalid(state, arg):
+ pass
+
+
+def intrinsic_print(state, arg):
+ pass
+
+
+def intrinsic_import_star(state, arg):
+ pass
+
+
+def intrinsic_stopiteration_error(state, arg):
+ pass
+
+
+def intrinsic_async_gen_wrap(state, arg):
+ pass
+
+
+def intrinsic_unary_positive(state, arg):
+ opcodes.unary_positive(state, arg)
+ pass
+
+
+def intrinsic_list_to_tuple(state, arg):
+ opcodes.list_to_tuple(state, arg)
+ pass
+
+
+def intrinsic_typevar(state, arg):
+ pass
+
+
+def intrinsic_paramspec(state, arg):
+ pass
+
+
+def intrinsic_typevartuple(state, arg):
+ pass
+
+
+def intrinsic_subscript_generic(state, arg):
+ pass
+
+
+def intrinsic_typealias(state, arg):
+ pass
+
+
+int_one_ops = [
+ intrinsic_1_invalid,
+ intrinsic_print,
+ intrinsic_import_star,
+ intrinsic_stopiteration_error,
+ intrinsic_async_gen_wrap,
+ intrinsic_unary_positive,
+ intrinsic_list_to_tuple,
+ intrinsic_typevar,
+ intrinsic_paramspec,
+ intrinsic_typevartuple,
+ intrinsic_subscript_generic,
+ intrinsic_typealias
+]
Review Comment:
```suggestion
])
```
##########
sdks/python/apache_beam/typehints/opcodes.py:
##########
@@ -188,6 +204,31 @@ def store_subscr(unused_state, unused_args):
pass
+def binary_slice(state, args):
+ _ = state.stack.pop()
+ _ = state.stack.pop()
+ base = Const.unwrap(state.stack.pop())
+ if base is str:
+ out = base
+ elif isinstance(base, typehints.IndexableTypeConstraint):
+ out = base
+ else:
+ out = element_type(base)
Review Comment:
For my understanding, what is the interpretation of
`out = element_type(base)`?
does this say: take a container, and return the type of individual element that lives in the container? Wouldn't a slice always return a container as opposed to individual elements?
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -641,6 +654,18 @@ def infer_return_type_func(f, input_types, debug=False, depth=0):
# No-op introduced in 3.11. Without handling this some
# instructions have functionally > 2 byte size.
pass
+ elif opname == 'RETURN_CONST':
+ # Introduced in 3.12. Handles returning constants directly
+ # instead of having a LOAD_CONST before a RETURN_VALUE.
+ returns.add(state.const_type(arg))
+ state = None
+ elif opname == 'CALL_INTRINSIC_1':
+ int_op = intrinsic_one_ops.int_one_ops[arg]
+ if debug:
+ print("Executing intrinsic one op", int_op.__name__.upper())
+ int_op(state, arg)
+ elif opname == 'CALL_INTRINSIC_2':
Review Comment:
Should we add a comment sth like: all currently available binary intrinsic ops are not relevant for type-checking, and as i mentioned above, also have some safeguard to revisit this logic if we can detect that the list of intrinsic ops has changed.
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589744150
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
Review Comment:
done (did it on my branch instead of taking the suggestion)
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1591557754
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops_test.py:
##########
@@ -30,7 +30,7 @@ class IntrinsicOneOpsTest(unittest.TestCase):
def testOperationsOrder(self):
if sys.version_info >= (3, 12):
dis_order = dis.__dict__['_intrinsic_1_descs']
- beam_order = list()
+ beam_order = []
Review Comment:
nit: use [list comprehensions](https://docs.python.org/3/tutorial/datastructures.html#list-comprehensions) for a more Pythonic syntax. e.g.
`beam_ops = [fn.__name_upper() for fn in intrinsic_one_ops.INT_ONE_OPS]`
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on PR #31170:
URL: https://github.com/apache/beam/pull/31170#issuecomment-2093395082
R: @tvalentyn
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1591564522
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops_test.py:
##########
@@ -0,0 +1,40 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Tests for apache_beam.typehints.intrinsic_one_ops."""
+
+# pytype: skip-file
+
+import dis
+import sys
+import unittest
+
+from apache_beam.typehints import intrinsic_one_ops
+
+
+class IntrinsicOneOpsTest(unittest.TestCase):
+ def testOperationsOrder(self):
Review Comment:
```suggestion
def test_unary_intrinsic_ops_are_in_the_same_order_as_in_cpython(self):
(self):
```
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1592593676
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops_test.py:
##########
@@ -30,7 +30,7 @@ class IntrinsicOneOpsTest(unittest.TestCase):
def testOperationsOrder(self):
if sys.version_info >= (3, 12):
dis_order = dis.__dict__['_intrinsic_1_descs']
- beam_order = list()
+ beam_order = []
Review Comment:
done
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops_test.py:
##########
@@ -0,0 +1,40 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Tests for apache_beam.typehints.intrinsic_one_ops."""
+
+# pytype: skip-file
+
+import dis
+import sys
+import unittest
+
+from apache_beam.typehints import intrinsic_one_ops
+
+
+class IntrinsicOneOpsTest(unittest.TestCase):
+ def testOperationsOrder(self):
Review Comment:
done
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589736261
##########
sdks/python/apache_beam/typehints/trivial_inference.py:
##########
@@ -432,6 +433,8 @@ def infer_return_type_func(f, input_types, debug=False, depth=0):
elif op in dis.haslocal:
print('(' + co.co_varnames[arg] + ')', end=' ')
elif op in dis.hascompare:
+ if (sys.version_info.major, sys.version_info.minor) >= (3, 12):
+ arg = arg >> 4
Review Comment:
This one is funny in that it was actually an undocumented change as best as I could tell. The comparison operations are similar to the intrinsic functions here in that the arg for a comparison op is an integer index into a list of operations that you index into. The output is always a boolean here anyway so we don't necessarily care what the operation itself is, but for debugging purposes we get the operation to print anyway. The problem was that these indices got bit shifted (a trick this package really likes to do for operations in different versions), so our print statement here was failing with an out-of-range index.
I'll try to add some comments around some of these changes since they're a little arbitrary, but for the most part it really just breaks down to "this argument's representation was changed in the package so we have to change it here for this version"
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31170:
URL: https://github.com/apache/beam/pull/31170#issuecomment-2093396701
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589745606
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
+FrameState object, the second the integer opcode argument.
+
+Bytecodes with more complicated behavior (e.g. modifying the program counter)
+are handled inline rather than here.
+
+For internal use only; no backwards-compatibility guarantees.
+"""
+# pytype: skip-file
+
+from . import opcodes
+
+
+def intrinsic_1_invalid(state, arg):
+ pass
+
+
+def intrinsic_print(state, arg):
+ pass
+
+
+def intrinsic_import_star(state, arg):
+ pass
+
+
+def intrinsic_stopiteration_error(state, arg):
+ pass
+
+
+def intrinsic_async_gen_wrap(state, arg):
+ pass
+
+
+def intrinsic_unary_positive(state, arg):
+ opcodes.unary_positive(state, arg)
+ pass
+
+
+def intrinsic_list_to_tuple(state, arg):
+ opcodes.list_to_tuple(state, arg)
+ pass
+
+
+def intrinsic_typevar(state, arg):
+ pass
+
+
+def intrinsic_paramspec(state, arg):
+ pass
+
+
+def intrinsic_typevartuple(state, arg):
+ pass
+
+
+def intrinsic_subscript_generic(state, arg):
+ pass
+
+
+def intrinsic_typealias(state, arg):
+ pass
+
+
+int_one_ops = [
Review Comment:
I'll see if the dis package provides a utility to get that list easily so we can check against it. Otherwise documenting why the order is what it is and adding some check for python versions might be the best way to gate it off.
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn merged PR #31170:
URL: https://github.com/apache/beam/pull/31170
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589742323
##########
sdks/python/apache_beam/typehints/opcodes.py:
##########
@@ -188,6 +204,31 @@ def store_subscr(unused_state, unused_args):
pass
+def binary_slice(state, args):
+ _ = state.stack.pop()
+ _ = state.stack.pop()
+ base = Const.unwrap(state.stack.pop())
+ if base is str:
+ out = base
+ elif isinstance(base, typehints.IndexableTypeConstraint):
+ out = base
+ else:
+ out = element_type(base)
Review Comment:
Hmm that's actually an implementation interpretation I hadn't thought of. The documentation for the op is here - https://docs.python.org/3/library/dis.html#opcode-BINARY_SLICE and I was adapting the BINARY_SUBSCR code (which is doing similar work with a single index arg instead of a slice.) I think most cases will be caught by the indexable type constraint (so the type of the container is what we return) but that else case might not be handling things correctly. I'll see if I can get a bit more of an understanding on this op code specifically.
--
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] Fix trivial inference tests for Python 3.12 support [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #31170:
URL: https://github.com/apache/beam/pull/31170#discussion_r1589759004
##########
sdks/python/apache_beam/typehints/intrinsic_one_ops.py:
##########
@@ -0,0 +1,98 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Defines the actions intrinsic bytecodes have on the frame.
+
+Each function here corresponds to a bytecode documented in
+https://docs.python.org/2/library/dis.html or
+https://docs.python.org/3/library/dis.html. The first argument is a (mutable)
+FrameState object, the second the integer opcode argument.
+
+Bytecodes with more complicated behavior (e.g. modifying the program counter)
+are handled inline rather than here.
+
+For internal use only; no backwards-compatibility guarantees.
+"""
+# pytype: skip-file
+
+from . import opcodes
+
+
+def intrinsic_1_invalid(state, arg):
+ pass
+
+
+def intrinsic_print(state, arg):
+ pass
+
+
+def intrinsic_import_star(state, arg):
+ pass
+
+
+def intrinsic_stopiteration_error(state, arg):
+ pass
+
+
+def intrinsic_async_gen_wrap(state, arg):
+ pass
+
+
+def intrinsic_unary_positive(state, arg):
+ opcodes.unary_positive(state, arg)
+ pass
+
+
+def intrinsic_list_to_tuple(state, arg):
+ opcodes.list_to_tuple(state, arg)
+ pass
+
+
+def intrinsic_typevar(state, arg):
+ pass
+
+
+def intrinsic_paramspec(state, arg):
+ pass
+
+
+def intrinsic_typevartuple(state, arg):
+ pass
+
+
+def intrinsic_subscript_generic(state, arg):
+ pass
+
+
+def intrinsic_typealias(state, arg):
+ pass
+
+
+int_one_ops = [
Review Comment:
Okay I can pull the table out from the module in a slightly-janky way but it does become testable. That way if ops change orders or new ones are added we can be aware of it.
--
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