Initial Python 3 compatibility fixes #23256

gwynne · 2019-03-13T01:17:30Z

This PR adds compatibility with Python versions 2 and 3 for the following parts of Swift:

Various .gyb files in the stdlib
gyb
gyb_sourcekit_support
gyb_syntax_support
line-directive
update_checkout
build_swift
swift_build_support
bug_reducer
Several additional miscellaneous utilities
Initial support for the primary test driver
A few bits and pieces in the test suite, validation test suite, unit tests, and benchmarks.

Note: All of llvm and clang, as well as build-script, are already fully compatible with Python 3.

Note: An upstream issue with lit.py's Windows support is blocking Python 3 compatibility for the test suites at this time.

This PR does NOT provide complete Python 3 support; considerable further work is needed. Thoughts on whether this PR should be broken into multiple pieces and if so in what way would be most appreciated.

Note: Unlike Python 2, in Python 3 `unittest discover` loads the module, forcing the manual inclusion of a known path to swift_build_support in update_checkout Note: Renamed `ParserError` to `ArgParserError` to avoid confusion with `parser` module in build_swift Note: The syntax for declaring a metaclass changed between Python 2 and 3 in a severely incompatible way; it is not possible to (sanely) use the Python 3 syntax with a Python 2 interpreter. Here we take inspiration from the six module's `with_metaclass()` function to directly specify the metaclass in a way that, while ugly, is fully cross-compatible.

…ne-directive

Note: Includes multiple whitespace and minor style changes to satisfy python_lint.py

Note: Retain old array shuffle by using random explicitly (Python 3 changed default algorithm) - test success depends on exact ordering, this seems wrong but fixing it is out of scope here.

gwynne · 2019-03-13T01:40:06Z

Tests for this (beyond verifying the existing config stays working) will be tricky. The test suite doesn't run with Python 3 yet and I'm not aware of any infrastructure for Python 3 CI.

gwynne · 2019-03-13T01:40:23Z

@swift-ci please smoke test

compnerd · 2019-03-13T02:07:33Z

Nothing here stands out as an issue. Personally, I am a fan of separate changes when possible so that they can get merged earlier rather than later.

drodriguez

Seems that we are working in the same things and finding the same problems.

In #23038 I have many of this fixes. I wanted to break them apart per script, so it will be easier to review and accept, but all at the same time is also a good approach.

If you don’t mind, I will leave a couple of notes here and there.

I can review later which pieces I seem to have that these changes are missing, and see if they are interesting.

drodriguez · 2019-03-13T23:05:16Z

benchmark/scripts/Benchmark_Driver

@@ -37,6 +38,9 @@ import time

 from compare_perf_tests import LogParser

+if not hasattr(sys.modules['__builtin__'], 'reduce'):
+    reduce = functools.reduce
+


Maybe it is easier to use functools.reduce in both Python 2 and 3.

from functools import reduce

(This happens in a couple more places in other files, I will use the same solution there)

Yeah, I was learning as I went along, so didn't always pick up the best solutions, especially in some of my earlier changes.

Can you adopt that throughout? It seems cleaner.

drodriguez · 2019-03-13T23:23:28Z

benchmark/scripts/Benchmark_Driver

+            is_text = isinstance(msg, bytes)
+
+        try:
+            if is_text and getattr(stream, 'encoding', None):


bytes in Python 3 do not have encoding. Maybe you wanted to compare with isinstance(msg, str)?

Python 3.6.3rc1+ (default, Feb 5 2019, 15:51:57) [GCC 5.x 20180625 5.5.0+] on linux Type "help", "copyright", "credits" or "license" for more information. >>> b''.encode() Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'bytes' object has no attribute 'encode'

If I remember correctly it was specifically a check to find out whether the attribute was present or not - keep in mind I was maintaining compatibility with both 2 and 3, not just updating to 3.

Sorry, I mistyped. Corrected: bytes in Python 3 do not have encode(…). str does.

drodriguez · 2019-03-13T23:27:33Z

benchmark/scripts/test_Benchmark_Driver.py

-from StringIO import StringIO
+try:
+    from StringIO import StringIO # py2
+except ModuleNotFoundError:


Minor: ModuleNotFoundError was introduced in Python 3.6. Python 2 should find StringIO just fine, but maybe you can change this with ImportError (the supertype), which should work in any Python 2 and 3.

(This also happens in more places below)

I was getting no such module errors from Python 3 because StringIO is io.StringIO in 3.

What I wanted to say is that ModuleNotFoundError was introduced in Python 3.6. ModuleNotFoundError is a subtype of ImportError. It will be more portable to use ImportError, so anything from Python 3.0 to Python 3.6 can import io.StringIO.

drodriguez · 2019-03-13T23:34:51Z

stdlib/public/core/SIMDVectorTypes.swift.gyb

@@ -65,7 +71,7 @@ public struct SIMD${n}<Scalar> : SIMD where Scalar: SIMDScalar {
    self.init(${', '.join('xyzw'[:n])})
  }

-%  for i in range(n):
+%  for i in list(range(n)):


In the three cases above, list(range(n)) can be just range(n). range returns a list in Python 2, and an iterator in Python 3. Both can be used directly by for … in ….

Right, these shouldn't need any changes.

drodriguez · 2019-03-13T23:36:26Z

stdlib/public/core/SIMDVectorTypes.swift.gyb

+try:
+	xrange
+except NameError:
+	xrange = range


In my version I simply change the xrange to range. In Py3, range works as Py2's xrange, and for the usages here, range in Py2 is not generating very long list, and the list is completely iterated. It makes the code easier to read, IMO. The only change was in L156.

I tend to agree; there was some reason I went with this approach that I've forgotten now

Yeah, please just use range for these cases; I think there's only like one instance of xrange in this file anyway, it should just be converted to keep the diffs minimal.

drodriguez · 2019-03-14T00:13:26Z

utils/remote-run

                                               quote(local_file))
                         for local_file, remote_file
                         in local_to_remote_files.viewitems())
        self.run_sftp(sftp_commands)

    def run_remote(self, command, remote_env={}):
-        env_strings = ['{0}={1}'.format(k,v) for k,v in remote_env.viewitems()]
+        env_strings = ['{0}={1}'.format(k,v) for k,v in remote_env.items()]


Besides this viewitems, there's a viewvalues in L44, another viewitems in L51 and L67, a viewkeys in L56. Didn’t you have problems with those in Python 3?

I don't know that I ever finished getting this particular script working 😅

drodriguez · 2019-03-14T00:15:11Z

utils/scale-test

@@ -69,7 +69,7 @@ def write_input_file(args, ast, d, n):

 def ensure_tmpdir(d):
    if d is not None and not os.path.exists(d):
-        os.makedirs(d, 0700)
+        os.makedirs(d, 0o700)


I found another one in L84 as well.

drodriguez · 2019-03-14T00:16:14Z

utils/swift_build_support/tests/test_shell.py

@@ -73,7 +73,7 @@ def test_capture(self):

        self.assertEqual(
            shell.capture(["sh", "-c", "echo foo && false"],
-                          allow_non_zero_exit=True), "foo\n")
+                          allow_non_zero_exit=True).decode('ascii'), "foo\n")


I solved this with a b"foo\n" instead.

drodriguez · 2019-03-14T00:17:11Z

utils/update_checkout/__init__.py

@@ -1,4 +1,3 @@
-
-from update_checkout import main
+from update_checkout.update_checkout import main


If you don't want the absolute import, it can also be done with from .update_checkout import main.

This is another case of "the only solution I found that worked in both 2 and 3"

from .update_checkout import main __all__ = ["main"]

Just to let you know that worked when Im trying to compile the tensorflow branch... dont know if compatible with python2, changint the 2 __init__.py files for from update_checkout.update_checkout import main seem to not work on my end??

drodriguez · 2019-03-14T00:18:46Z

utils/update_checkout/update_checkout/update_checkout.py

+SCRIPT_FILE = os.path.abspath(__file__)
+SCRIPT_DIR = os.path.dirname(SCRIPT_FILE)
+sys.path.append(os.path.dirname(os.path.dirname(SCRIPT_DIR)))
+


Is this for fixing the test update_checkout.test-sh? I changed the test instead to provide a working directory with the -t parameter.

# RUN: %{python} -m unittest discover -s %swift_src_root/utils/update_checkout -t %swift_src_root/utils/

This was to deal with importing swift_build_support, which lives one directory up from update-checkout and wasn't found by the default paths in 3.

Yes, I understand why you add it. In my tests with Python 3, when running the script by itself, I didn’t need any changes here. But when running from the tests, I found out that the modules weren’t in the search path. I used the solution I pointed above, since it doesn’t modify the code for all cases.

stephentyrone · 2019-03-23T18:09:18Z

benchmark/scripts/Benchmark_Driver

                                num_iters=1, quantile=20)
-                       for _ in range(self.args.independent_samples)])
+                       for _ in range(self.args.independent_samples)]
+        return reduce(lambda m, res: (m.merge(res), m)[1], result_sets)


The original version of this seems less magical and easier for non-python folks to grok.

stephentyrone · 2019-03-23T18:11:17Z

benchmark/scripts/Benchmark_Driver

@@ -414,6 +418,7 @@ class BenchmarkDoctor(object):
        name = measurements['name']
        setup, ratio = BenchmarkDoctor._setup_overhead(measurements)
        setup = 0 if ratio < 0.05 else setup
+


Please remove whitespace-only changes.

stephentyrone · 2019-03-23T18:16:19Z

stdlib/public/core/Tuple.swift.gyb

@@ -111,7 +111,7 @@ public func >=(lhs: (), rhs: ()) -> Bool {
 %   equatableTypeParams = ", ".join(["{} : Equatable".format(c) for c in typeParams])

 %   originalTuple = "(\"a\", {})".format(", ".join(map(str, range(1, arity))))
-%   greaterTuple = "(\"a\", {})".format(", ".join(map(str, range(1, arity - 1) + [arity])))
+%   greaterTuple = "(\"a\", {})".format(", ".join(map(str, list(range(1, arity - 1)) + [arity])))


Don't need this change, either.

Python 3 definitely went bananas without this change.

I think this change is needed. In Python 3, range will return an iterable, which cannot be added a list. The list(…) transforms the iterable into a list, which will work.

Yes, because if you enter arity=3; "(\"a\", {})".format(", ".join(map(str, range(1, arity - 1) + [arity]))) to an interpreter it will cause TypeError: unsupported operand type(s) for +: 'range' and 'list' also https://stackoverflow.com/a/47472763 which leads to xrange() was renamed to range() in Python 3 and range(...) replaced with list(range(...))

shahmishal · 2020-10-01T06:59:35Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

gwynne added 9 commits March 12, 2019 18:08

Python 2/3 compat: gyb, gyb_sourcekit_support, gyb_syntax_support, li…

e722385

…ne-directive

Python 2/3 compat: Swift stdlib

aa3b4eb

Python 2/3 compat: various utils

4c8be84

Note: Includes multiple whitespace and minor style changes to satisfy python_lint.py

Python 2/3 compat: tests driver

7e02439

Python 2/3 compat: unittests/Reflection

3dad59e

Python 2/3 compat: validation tests

d0c24fd

Python 2/3 compat: benchmarks

b5ec4a9

Python 2/3 compat: bug_reducer

b2fde82

Note: Retain old array shuffle by using random explicitly (Python 3 changed default algorithm) - test success depends on exact ordering, this seems wrong but fixing it is out of scope here.

gwynne requested review from compnerd, gottesmm, stephentyrone and jrose-apple March 13, 2019 01:17

drodriguez reviewed Mar 14, 2019

View reviewed changes

drodriguez mentioned this pull request Mar 14, 2019

Resolves [SR-9941] [SR-1805] remove python implicit import as it is deprecated #23272

Closed

nafu mentioned this pull request Mar 22, 2019

[SR-10150] Specify supported python version in README #23488

Merged

stephentyrone reviewed Mar 23, 2019

View reviewed changes

shahmishal closed this Oct 5, 2020

gwynne deleted the gwynne/python-3-compatibility branch April 9, 2023 00:05

		@@ -1,4 +1,3 @@

		from update_checkout import main
		from update_checkout.update_checkout import main

Initial Python 3 compatibility fixes #23256

Initial Python 3 compatibility fixes #23256

Uh oh!

Conversation

gwynne commented Mar 13, 2019

Uh oh!

gwynne commented Mar 13, 2019

Uh oh!

gwynne commented Mar 13, 2019

Uh oh!

compnerd commented Mar 13, 2019

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyoc213 Apr 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahmishal commented Oct 1, 2020

Uh oh!

Uh oh!

tyoc213 Apr 7, 2019 •

edited

Loading