Futurize for Py3 #57

MrSaints · 2017-07-08T00:59:57Z

As per the suggestion #43 (comment), this PR runs futurize against the source with small fixes to ensure the Python 2 tests continues to pass. I thought I make this PR to get Python 3 support moving (even if it is in the smallest way possible).

CLAassistant · 2017-07-08T01:00:04Z

All committers have signed the CLA.

coveralls · 2017-07-08T01:21:19Z

Coverage increased (+0.1%) to 94.598% when pulling ad2bfa7 on arachnys:py3-support into b3b4812 on uber:master.

yurishkuro · 2017-07-08T17:38:37Z

Thanks for this PR, @MrSaints!

This change looks good, mostly. The two concerns I have are:

The use of old_div seems unnecessary. There aren't that many occurrences, can we replace them with / for floats and // for ints? It is usually apparent from the context which numbers we're dealing with.
The change from iteritems()/iterkeys() to items()/keys() is not good, since in Py2 the latter will cause creation of new arrays. Can we replace all instances with respective proxy methods from six?

And there are some lint errors in the build:

flake8 jaeger_client crossdock tests
jaeger_client/codecs.py:25:1: E402 module level import not at top of file
jaeger_client/codecs.py:26:1: E402 module level import not at top of file
jaeger_client/codecs.py:27:1: E402 module level import not at top of file
jaeger_client/codecs.py:27:22: E401 multiple imports on one line
jaeger_client/codecs.py:29:1: E402 module level import not at top of file
jaeger_client/codecs.py:33:1: E402 module level import not at top of file
jaeger_client/codecs.py:38:1: E402 module level import not at top of file
jaeger_client/metrics.py:28:1: E302 expected 2 blank lines, found 0
make: *** [lint] Error 1

MrSaints · 2017-07-09T01:04:12Z

Cheers for the quick feedback @yurishkuro. I've addressed them in the new commits.

yurishkuro · 2017-07-09T03:04:45Z

there seem to be some errors in the build

coveralls · 2017-07-09T09:34:51Z

Coverage increased (+0.2%) to 94.609% when pulling b8cc36c on arachnys:py3-support into b3b4812 on uber:master.

yurishkuro

lgtm. I would like @black-adder to have a second look, then we can merge. I left a couple minor comments about old_div - don't think we need it, so let's not pollute the code.

I tried (#58) testing this version with py 3.6, but it bombs spectacularly, so clearly more work is needed (perhaps it's the thrift lib). I opened an overall issue #59 to support Python 3.x, could you comment there with the remaining steps after this PR is merged?

yurishkuro · 2017-07-09T21:42:45Z

jaeger_client/sampler.py

@@ -19,9 +19,12 @@
 # THE SOFTWARE.

 from __future__ import absolute_import
+from __future__ import division


Q: is this import necessary?

I'm not too sure. Seems like it.

The future division statement, spelled from __future__ import division , will change the / operator to mean true division throughout the module.

yurishkuro · 2017-07-09T21:43:53Z

tests/test_sampler.py

@@ -198,7 +201,7 @@ def test_adaptive_sampler():
    sampled, tags = sampler.is_sampled(MAX_INT-10, "new_op")
    assert sampled
    assert tags == get_tags('probabilistic', 0.51)
-    sampled, tags = sampler.is_sampled(MAX_INT+(MAX_INT/4), "new_op")
+    sampled, tags = sampler.is_sampled(MAX_INT+(old_div(MAX_INT,4)), "new_op")


can we change back to /?

yurishkuro · 2017-07-09T21:44:06Z

tests/test_sampler.py

@@ -207,7 +210,7 @@ def test_adaptive_sampler():
    sampled, tags = sampler.is_sampled(MAX_INT-10, "new_op_2")
    assert sampled
    assert tags == get_tags('probabilistic', 0.51)
-    sampled, _ = sampler.is_sampled(MAX_INT+(MAX_INT/4), "new_op_2")
+    sampled, _ = sampler.is_sampled(MAX_INT+(old_div(MAX_INT,4)), "new_op_2")


can we change back to /?

yurishkuro · 2017-07-09T21:44:16Z

tests/test_sampler.py

@@ -18,6 +19,8 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 # THE SOFTWARE.

+from builtins import range
+from past.utils import old_div


can we remove this?

yurishkuro · 2017-07-09T21:44:53Z

tests/test_sampler.py

@@ -167,7 +170,7 @@ def test_guaranteed_throughput_probabilistic_sampler():
    sampled, tags = sampler.is_sampled(MAX_INT-10)
    assert sampled
    assert tags == get_tags('probabilistic', 0.51)
-    sampled, tags = sampler.is_sampled(MAX_INT+(MAX_INT/4))
+    sampled, tags = sampler.is_sampled(MAX_INT+(old_div(MAX_INT,4)))


can we use /?

black-adder · 2017-07-10T03:56:04Z

jaeger_client/thrift.py

@@ -117,7 +118,7 @@ def timestamp_micros(ts):
    :param ts:
    :return:
    """
-    return long(ts * 1000000)
+    return int(ts * 1000000)


If someone on a 32bit architecture uses this, we'll get integer overflow no?
unix seconds 1499659236
microseconds 1499659236000000 >>> 2^32

We can probably switch between int, and long using six.

do we need to make that change for this PR?

I've made that change, I'm unsure if the int behaves as expected for py3, but we can do that in a different PR.

coveralls · 2017-07-10T07:46:17Z

Coverage increased (+0.2%) to 94.695% when pulling 4ea03fe on arachnys:py3-support into b3b4812 on uber:master.

MrSaints · 2017-07-10T09:31:06Z

I've made the changes @yurishkuro, but I'm not too sure what's going on with one of the Travis COVER=1 test: INTERNALERROR> RuntimeError: dictionary changed size during iteration

black-adder · 2017-07-10T14:47:29Z

The code coverage issue looks related to: https://bitbucket.org/ned/coveragepy/issues/581/44b1-44-breaking-in-ci we probably want to use an older version of coverage until it's fixed

coveralls · 2017-07-10T16:20:15Z

Coverage increased (+0.3%) to 94.702% when pulling 50f762e on arachnys:py3-support into b3b4812 on uber:master.

yurishkuro · 2017-07-10T17:30:35Z

long_fn = int if six.PY3 else long

@MrSaints a couple questions:

Will ^^ actually parse/compile with Py3? I thought 3.x decommissioned long.
from reading the PEP 237 and SO question it seems the overflow on ints has been eliminated in favor of automatic upgrading of the value from a machine word to a malloc-ed memory, making this transparent to the end user. I don't know if we can find a 32bit machine to test this, any thoughts?

Even on 64bit machine the automatic type conversion can be seen:

>>> import sys;
>>> sys.maxint
9223372036854775807
>>> type(sys.maxint)
<type 'int'>
>>> type(sys.maxint + 1)
<type 'long'>
>>> type(sys.maxint * 100)
<type 'long'>
>>> (type(sys.maxint * 100), sys.maxint * 100)
(<type 'long'>, 922337203685477580700L)

i.e. an int value times 100 is auto-converted to long.

coveralls · 2017-07-10T17:42:33Z

Coverage increased (+0.1%) to 94.641% when pulling 1816512 on arachnys:py3-support into 19e2ce6 on uber:master.

MrSaints · 2017-07-10T17:45:12Z

(1) Whoops @yurishkuro. I've cleaned it up.
(2) We can do more vigorous testing with Python 3. I believe it should be fine if it does not change the existing behaviour in Python 2.

yurishkuro · 2017-07-10T17:52:17Z

Attempting to fix the coverage issue here #61

yurishkuro · 2017-07-10T18:00:56Z

@MrSaints can you rebase or merge from master to pick up #61?

Tests are passing.

This is to introduce more safe changes to support Python 3. It is likely to result in tests failures.

Use `six.PY3` to force non-unicode strings in Python 2.

…x.iterkeys()` This is in response to CR feedback as using `items()`, and `keys()` in Py2 will result in the creation of new arrays. Thus, this commit will use proxy methods from `six` instead.

…ests

Use `long` for py2, and `int` for py3. Otherwise we will get an integer overflow for py2 32bit.

MrSaints · 2017-07-10T18:47:22Z

@yurishkuro Cheers for the fix. And rebased.

coveralls · 2017-07-10T18:59:38Z

Coverage increased (+0.1%) to 94.641% when pulling 532eba6 on arachnys:py3-support into 8d94fa8 on uber:master.

black-adder

lgtm

MrSaints · 2017-07-11T14:05:59Z

We good to merge?

MrSaints · 2017-07-11T16:25:27Z

Cheers for all the feedback, and for pushing this out @yurishkuro @black-adder

Groxx · 2017-10-25T20:47:14Z

Does this install_aliases() call taint the entire environment that imports it, not just this source file? If yes, it seems like this may be breaking anything expecting e.g. a specific html library (or a specific version), like https://pypi.python.org/pypi/html
Or enum, since enum and enum34 (which overrides enum) are extremely different.

olevold · 2017-10-27T13:57:48Z

I still get ImportError: No module named 'ttypes'

MrSaints · 2017-10-27T13:59:23Z

Are you running on Python 3 @olevold ? There's still work that needs to be done (e.g. on Thrift side) to get it working on Python 3.

olevold · 2017-10-27T14:00:10Z

Ok, I thought it had been solved. Sorry

MrSaints closed this Jul 9, 2017

MrSaints reopened this Jul 9, 2017

This was referenced Jul 9, 2017

[DO NOT MERGE] Test with python 3.6 #58

Closed

Support Python 3.x #59

Closed

yurishkuro approved these changes Jul 9, 2017

View reviewed changes

black-adder reviewed Jul 10, 2017

View reviewed changes

MrSaints added 9 commits July 10, 2017 19:46

[py3] Add future as a dependency

7167d2b

[py3] futurize(stage1): modernized py2 code w/o dependencies

1a92529

Tests are passing.

[py3] futurize(stage2): builtins, and utilities from future

677d2af

This is to introduce more safe changes to support Python 3. It is likely to result in tests failures.

[py3] futurize(stage2): fix broken tests

3e3e2fa

[py3] fix TypeError: a bytes-like object is required, not 'str'

93087ee

Use `six.PY3` to force non-unicode strings in Python 2.

[py3] Replace unnecessary old_div usage with /

40e47ce

[py3] Replace iteritems() / iterkeys() -> six.iteritems() / `si…

040413e

…x.iterkeys()` This is in response to CR feedback as using `items()`, and `keys()` in Py2 will result in the creation of new arrays. Thus, this commit will use proxy methods from `six` instead.

[py3] fix linting errors due to import order

6000666

[py3] remove unused urllib dependencies from codecs.py, and fix t…

e663972

…ests

MrSaints added 2 commits July 10, 2017 19:46

[py3] revert uses of old_div in favour of standard / divisor

e21d2f9

[py3] Revert change from long -> int

532eba6

Use `long` for py2, and `int` for py3. Otherwise we will get an integer overflow for py2 32bit.

black-adder approved these changes Jul 10, 2017

View reviewed changes

yurishkuro merged commit 0392cce into jaegertracing:master Jul 11, 2017

MrSaints deleted the py3-support branch July 11, 2017 16:25

yurishkuro mentioned this pull request Aug 11, 2017

SyntaxError in Python 3 #41

Closed

Futurize for Py3 #57

Futurize for Py3 #57

Conversation

MrSaints commented Jul 8, 2017

CLAassistant commented Jul 8, 2017 • edited Loading

coveralls commented Jul 8, 2017

yurishkuro commented Jul 8, 2017

MrSaints commented Jul 9, 2017

yurishkuro commented Jul 9, 2017

coveralls commented Jul 9, 2017 • edited Loading

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 10, 2017

MrSaints commented Jul 10, 2017

black-adder commented Jul 10, 2017

coveralls commented Jul 10, 2017 • edited Loading

yurishkuro commented Jul 10, 2017

coveralls commented Jul 10, 2017

MrSaints commented Jul 10, 2017

yurishkuro commented Jul 10, 2017

yurishkuro commented Jul 10, 2017

MrSaints commented Jul 10, 2017

coveralls commented Jul 10, 2017

black-adder left a comment

Choose a reason for hiding this comment

MrSaints commented Jul 11, 2017

MrSaints commented Jul 11, 2017

Groxx commented Oct 25, 2017 • edited Loading

olevold commented Oct 27, 2017

MrSaints commented Oct 27, 2017

olevold commented Oct 27, 2017

CLAassistant commented Jul 8, 2017 •

edited

Loading

coveralls commented Jul 9, 2017 •

edited

Loading

coveralls commented Jul 10, 2017 •

edited

Loading

Groxx commented Oct 25, 2017 •

edited

Loading