Opened 2 years ago

Closed 5 months ago

#5929 task closed fixed (fixed)

Port remaining parts of twisted.python.reflect to Python 3

Reported by: exarkun Owned by: rwall
Priority: normal Milestone: Python-3.x
Component: core Keywords: py3k
Cc: thijs Branch: branches/python3-reflect-5929-2
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

Many parts of twisted.python.reflect were individually ported to Python 3 as part of the Python 3 minimal milestone. These were moved into twisted.python._reflectpy3 due to technical limitations of the approach taken to porting.

For the full Python 3 port, all the rest of twisted.python.reflect needs to be ported (except for deprecated APIs that we can delete instead). As part of this, twisted/python/_reflectpy3.py should be eliminated and its contents put back into twisted/python/reflect.py.

The test suite, similarly divided, should also be re-unified. One minor but important part of this will be fixing the import of _fullyQualifiedName in twisted.python.test.test_reflectpy3. It is currently not possible to import twisted.python.reflect.fullyQualifiedName, but once twisted.python.reflect is ported it will be, and that spelling should be preferred over twisted.python._deprecatepy3._fullyQualifiedName for the testing of fullyQualifiedName.

Attachments (10)

python3-reflect-5929-1.patch (1.3 KB) - added by multani 15 months ago.
python3-reflect-5929-1.2.patch (102.4 KB) - added by multani 15 months ago.
python3-reflect-5929-2.patch (102.3 KB) - added by multani 15 months ago.
python3-reflect-5929-3.patch (102.2 KB) - added by multani 11 months ago.
python3-reflect-5929-4.patch (84.9 KB) - added by multani 11 months ago.
python3-reflect-5929-5.patch (84.6 KB) - added by multani 10 months ago.
python3-reflect-5929-6.1.patch (9.0 KB) - added by multani 10 months ago.
Port to Python 3 - 1/2
python3-reflect-5929-6.2.patch (76.1 KB) - added by multani 10 months ago.
Merge py3 modules to their origines - 2/2
python3-reflect-5929-7.patch (12.1 KB) - added by multani 6 months ago.
python3-reflect-5929-8.patch (6.2 KB) - added by multani 5 months ago.

Download all attachments as: .zip

Change History (65)

comment:1 Changed 18 months ago by thijs

  • Cc thijs added
  • Type changed from enhancement to task

comment:2 Changed 15 months ago by multani

I started to port the rest of t.p.reflect to Python according to the indications in this ticket.

I have a few questions though, before uploading a first patch:

  • it is said: (except for deprecated APIs that we can delete instead). There are several deprecations from Twisted 11.0.0 and 12.1.0. Most of them are portable (I already ported them actually), but should they be really deleted instead?
  • I'm not sure how I should handle the move of _fullyQualifiedName from t.p.deprecate back to t.p.reflect, since deprecate uses fullyQualifiedName and reflect use the deprecation functions from deprecate so there will be a circular dependency between them. Or maybe I misunderstood something?
  • Finally, I still have a test failure with Python 3 regarding the tests from twisted.test.test_reflect.GetClass http://twistedmatrix.com/trac/browser/trunk/twisted/test/test_reflect.py#L296 "Old-style" classes don't exist anymore in Python 3, and both classes as written in the test are reflected as the same kind of class. I will probably propose a fix that basically offers a specific version of this class for Python 3 which will expect the same result for both kind of classes. Is there's a preferred way to handle it?

The current state of the patch is available on a Git branch at https://github.com/multani/twisted/tree/python3-reflect-5929

comment:2 Changed 15 months ago by multani

I started to port the rest of t.p.reflect to Python according to the indications in this ticket.

I have a few questions though, before uploading a first patch:

  • it is said: (except for deprecated APIs that we can delete instead). There are several deprecations from Twisted 11.0.0 and 12.1.0. Most of them are portable (I already ported them actually), but should they be really deleted instead?
  • I'm not sure how I should handle the move of _fullyQualifiedName from t.p.deprecate back to t.p.reflect, since deprecate uses fullyQualifiedName and reflect use the deprecation functions from deprecate so there will be a circular dependency between them. Or maybe I misunderstood something?
  • Finally, I still have a test failure with Python 3 regarding the tests from twisted.test.test_reflect.GetClass http://twistedmatrix.com/trac/browser/trunk/twisted/test/test_reflect.py#L296 "Old-style" classes don't exist anymore in Python 3, and both classes as written in the test are reflected as the same kind of class. I will probably propose a fix that basically offers a specific version of this class for Python 3 which will expect the same result for both kind of classes. Is there's a preferred way to handle it?

The current state of the patch is available on a Git branch at https://github.com/multani/twisted/tree/python3-reflect-5929

comment:2 Changed 15 months ago by multani

I started to port the rest of t.p.reflect to Python according to the indications in this ticket.

I have a few questions though, before uploading a first patch:

  • it is said: (except for deprecated APIs that we can delete instead). There are several deprecations from Twisted 11.0.0 and 12.1.0. Most of them are portable (I already ported them actually), but should they be really deleted instead?
  • I'm not sure how I should handle the move of _fullyQualifiedName from t.p.deprecate back to t.p.reflect, since deprecate uses fullyQualifiedName and reflect use the deprecation functions from deprecate so there will be a circular dependency between them. Or maybe I misunderstood something?
  • Finally, I still have a test failure with Python 3 regarding the tests from twisted.test.test_reflect.GetClass http://twistedmatrix.com/trac/browser/trunk/twisted/test/test_reflect.py#L296 "Old-style" classes don't exist anymore in Python 3, and both classes as written in the test are reflected as the same kind of class. I will probably propose a fix that basically offers a specific version of this class for Python 3 which will expect the same result for both kind of classes. Is there's a preferred way to handle it?

The current state of the patch is available on a Git branch at https://github.com/multani/twisted/tree/python3-reflect-5929

comment:2 Changed 15 months ago by multani

I started to port the rest of t.p.reflect to Python according to the indications in this ticket.

I have a few questions though, before uploading a first patch:

  • it is said: (except for deprecated APIs that we can delete instead). There are several deprecations from Twisted 11.0.0 and 12.1.0. Most of them are portable (I already ported them actually), but should they be really deleted instead?
  • I'm not sure how I should handle the move of _fullyQualifiedName from t.p.deprecate back to t.p.reflect, since deprecate uses fullyQualifiedName and reflect use the deprecation functions from deprecate so there will be a circular dependency between them. Or maybe I misunderstood something?
  • Finally, I still have a test failure with Python 3 regarding the tests from twisted.test.test_reflect.GetClass http://twistedmatrix.com/trac/browser/trunk/twisted/test/test_reflect.py#L296 "Old-style" classes don't exist anymore in Python 3, and both classes as written in the test are reflected as the same kind of class. I will probably propose a fix that basically offers a specific version of this class for Python 3 which will expect the same result for both kind of classes. Is there's a preferred way to handle it?

The current state of the patch is available on a Git branch at https://github.com/multani/twisted/tree/python3-reflect-5929

comment:3 Changed 15 months ago by multani

  • Keywords py3k review added

[Sorry for the duplicated messages, Trac got stuck waiting for the first one, and I did POST several others thinking the first ones weren't working :[ ]

I solved the first two points after the answer of exarkun on the mailing list (which basically said to remove deprecated objects), which allowed me to move back fullyQualifiedName back to t.p.reflect

I proposed a not-so-good fix for the 3rd point, I'm open to better suggestion.

Changed 15 months ago by multani

Changed 15 months ago by multani

Changed 15 months ago by multani

comment:2 Changed 15 months ago by multani

I started to port the rest of t.p.reflect to Python according to the indications in this ticket.

I have a few questions though, before uploading a first patch:

  • it is said: (except for deprecated APIs that we can delete instead). There are several deprecations from Twisted 11.0.0 and 12.1.0. Most of them are portable (I already ported them actually), but should they be really deleted instead?
  • I'm not sure how I should handle the move of _fullyQualifiedName from t.p.deprecate back to t.p.reflect, since deprecate uses fullyQualifiedName and reflect use the deprecation functions from deprecate so there will be a circular dependency between them. Or maybe I misunderstood something?
  • Finally, I still have a test failure with Python 3 regarding the tests from twisted.test.test_reflect.GetClass http://twistedmatrix.com/trac/browser/trunk/twisted/test/test_reflect.py#L296 "Old-style" classes don't exist anymore in Python 3, and both classes as written in the test are reflected as the same kind of class. I will probably propose a fix that basically offers a specific version of this class for Python 3 which will expect the same result for both kind of classes. Is there's a preferred way to handle it?

The current state of the patch is available on a Git branch at https://github.com/multani/twisted/tree/python3-reflect-5929

comment:2 Changed 15 months ago by multani

I started to port the rest of t.p.reflect to Python according to the indications in this ticket.

I have a few questions though, before uploading a first patch:

  • it is said: (except for deprecated APIs that we can delete instead). There are several deprecations from Twisted 11.0.0 and 12.1.0. Most of them are portable (I already ported them actually), but should they be really deleted instead?
  • I'm not sure how I should handle the move of _fullyQualifiedName from t.p.deprecate back to t.p.reflect, since deprecate uses fullyQualifiedName and reflect use the deprecation functions from deprecate so there will be a circular dependency between them. Or maybe I misunderstood something?
  • Finally, I still have a test failure with Python 3 regarding the tests from twisted.test.test_reflect.GetClass http://twistedmatrix.com/trac/browser/trunk/twisted/test/test_reflect.py#L296 "Old-style" classes don't exist anymore in Python 3, and both classes as written in the test are reflected as the same kind of class. I will probably propose a fix that basically offers a specific version of this class for Python 3 which will expect the same result for both kind of classes. Is there's a preferred way to handle it?

The current state of the patch is available on a Git branch at https://github.com/multani/twisted/tree/python3-reflect-5929

comment:3 Changed 14 months ago by radix

  • Keywords review removed

multani:

  1. It's fine to delete them instead of trying to update them, but that should be done in different tickets/branches.
  1. You could leave it in deprecate if it's a problem.
  1. You should skip the Python2-only tests based on the value of _PY3.

Changed 11 months ago by multani

Changed 11 months ago by multani

comment:4 follow-up: Changed 11 months ago by multani

  • Keywords review added

I created #6689 which removes the deprecated part of twisted.python.reflect.

The latest version of the patch depends on the patch from #6689, which should be reviewed and merged first. This addresses my previous first point.

I managed to move back fullyQualifiedName to its original module, and I proposed a (somewhat arguable) fix for the tests in twisted.test.test_reflect.GetClass.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 11 months ago by thijs

  • Keywords review removed
  • Owner set to multani

Replying to multani:

I created #6689 which removes the deprecated part of twisted.python.reflect.

The latest version of the patch depends on the patch from #6689, which should be reviewed and merged first. This addresses my previous first point.

Thanks for your work on this. Can you put this back in review once #6689 landed (to avoid confusion)?

comment:12 Changed 10 months ago by thijs

(In [40032]) Merge reflect-deprecate-6689: Remove deprecated classes from
twisted.python.reflect.

Author: multani
Reviewer: thijs, exarkun
Fixes: #6689
Refs: #5929

Accessor, AccessorType, OriginalAccessor, PropertyAccessor,
Settable and Summer in twisted.python.reflect, deprecated
since Twisted 12.1.0, are now removed.

comment:13 in reply to: ↑ 5 ; follow-up: Changed 10 months ago by multani

  • Keywords review added

Replying to thijs:

Replying to multani:

I created #6689 which removes the deprecated part of twisted.python.reflect.

The latest version of the patch depends on the patch from #6689, which should be reviewed and merged first. This addresses my previous first point.

Thanks for your work on this. Can you put this back in review once #6689 landed (to avoid confusion)?

thjis, thanks for taking care of the patches. Now that #6689 landed, I'm putting this ticket back in review.

comment:14 Changed 10 months ago by thijs

(In [40041]) Re-merge reflect-deprecate-6689: Remove deprecated classes from twisted.python.reflect.

Author: multani
Reviewer: thijs, exarkun, hawkowl
Fixes: #6689
Refs: #5929

Accessor, AccessorType, OriginalAccessor, PropertyAccessor, Settable and Summer
in twisted.python.reflect, deprecated since Twisted 12.1.0, are now removed.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 10 months ago by thijs

  • Keywords review removed

Replying to multani:

thjis, thanks for taking care of the patches. Now that #6689 landed, I'm putting this ticket back in review.

Thanks multani, could you base a new patch on the latest trunk now that #6689 has landed, it doesn't apply cleanly anymore.

patching file setup3.py
patching file twisted/internet/_sslverify.py
patching file twisted/internet/abstract.py
patching file twisted/internet/base.py
patching file twisted/internet/task.py
patching file twisted/internet/tcp.py
patching file twisted/internet/test/reactormixins.py
patching file twisted/protocols/tls.py
patching file twisted/python/_reflectpy3.py
patching file twisted/python/compat.py
patching file twisted/python/components.py
patching file twisted/python/deprecate.py
patching file twisted/python/failure.py
patching file twisted/python/log.py
patching file twisted/python/reflect.py
Hunk #1 FAILED at 7.
Hunk #2 succeeded at 113 (offset 2 lines).
Hunk #3 succeeded at 226 (offset 34 lines).
Hunk #4 succeeded at 252 (offset 34 lines).
1 out of 4 hunks FAILED -- saving rejects to file twisted/python/reflect.py.rej
patching file twisted/python/test/test_deprecate.py
patching file twisted/python/test/test_reflectpy3.py
patching file twisted/python/test/test_util.py
patching file twisted/test/test_failure.py
patching file twisted/test/test_reflect.py
Hunk #1 succeeded at 5 with fuzz 2.
Hunk #2 succeeded at 16 (offset -1 lines).
Hunk #3 succeeded at 770 (offset -1 lines).
Hunk #4 succeeded at 792 (offset -1 lines).
Hunk #5 succeeded at 821 (offset -1 lines).
patching file twisted/trial/reporter.py
patching file twisted/trial/test/test_assertions.py
patching file twisted/web/resource.py
patching file twisted/web/server.py

Changed 10 months ago by multani

comment:16 in reply to: ↑ 15 Changed 10 months ago by multani

  • Keywords review added

Replying to thijs:

Replying to multani:

thjis, thanks for taking care of the patches. Now that #6689 landed, I'm putting this ticket back in review.

Thanks multani, could you base a new patch on the latest trunk now that #6689 has landed, it doesn't apply cleanly anymore.

python3-reflect-5929-5.patch is rebase on top of trunk [40041].

Coverage of twisted.python.reflect is not quite good, I suppose it will have to be improve in order for this patch to land?

The diff is quite big, but most of the changes are actually moving lines around between the former py3 modules back to where they belonged.

comment:17 follow-up: Changed 10 months ago by exarkun

For the other modules we've made these kinds of changes to, we've completed the port separately from re-merging the two modules.

comment:18 in reply to: ↑ 17 Changed 10 months ago by multani

Replying to exarkun:

For the other modules we've made these kinds of changes to, we've completed the port separately from re-merging the two modules.

You mean, you usually re-merged the two modules in one patch/ticket and then did the port in another patch/ticket?

comment:19 follow-up: Changed 10 months ago by exarkun

You mean, you usually re-merged the two modules in one patch/ticket and then did the port in another patch/ticket?

Not exactly "usually". I think there were only two other split modules. :)

But yes - except in the other order, finish the port in one ticket and then merge the two modules in another.

I'm not saying you have to do this. Just observing it's what we did before and it results in two smaller patches.

Changed 10 months ago by multani

Port to Python 3 - 1/2

Changed 10 months ago by multani

Merge py3 modules to their origines - 2/2

comment:20 in reply to: ↑ 19 Changed 10 months ago by multani

Replying to exarkun:

You mean, you usually re-merged the two modules in one patch/ticket and then did the port in another patch/ticket?

Not exactly "usually". I think there were only two other split modules. :)

But yes - except in the other order, finish the port in one ticket and then merge the two modules in another.

I'm not saying you have to do this. Just observing it's what we did before and it results in two smaller patches.

I took some time this morning to split the big patch into two 'smaller' ones:

  • the first one do the actual porting to Python 3 and is quite small
  • the second one do the merging of the artificial py3 modules back to where they belonged. They are bigger but should be only be code moving from places to places.

Hopefully, it will make the job easier for reviewer!

comment:21 follow-up: Changed 10 months ago by exarkun

I guess I didn't communicate very well in my earlier comment. The second patch - which is not a Python 3 porting patch, but instead is a patch that merges the split up modules back into a single module - should be attached to the second ticket, #6239 (which is not a duplicate of this ticket - proof of which is the fact that resolving it requires extra changes beyond those required to resolve this ticket).

There's generally no reason to split up a change for resolving a *single* ticket into multiple patch files - that just makes life more difficult for everyone. However, there are many good reasons (enumerated and discussed on countless other tickets, the mailing list, the internet in general) to make changes as small as they can be while still being self-contained - and for resolving tickets independently.

I hope that python3-reflect-5929-6.1.patch is self-contained (I haven't read it) because I am going to ask for reviewers of this ticket to only consider that change. The second part should be considered separately for #6239.

comment:22 in reply to: ↑ 21 Changed 10 months ago by multani

Replying to exarkun:

Thanks for the clarifications about this ticket and #6239 exarkun.

I hope that python3-reflect-5929-6.1.patch is self-contained (I haven't read it) because I am going to ask for reviewers of this ticket to only consider that change. The second part should be considered separately for #6239.

python3-reflect-5929-6.1.patch should be self-contained and only concerned about Python 3 porting. The other patch, on the other hand, clearly depends on the first one and it's not going to apply without it.

comment:23 Changed 8 months ago by rwall

  • Author set to rwall
  • Branch set to branches/python3-reflect-5929

(In [40866]) Branching to 'python3-reflect-5929'

comment:24 Changed 8 months ago by rwall

(In [40867]) Apply python3-reflect-5929-6.1.patch from multani. Refs #5929

comment:25 Changed 8 months ago by rwall

(In [40869]) Apply python3-reflect-5929-6.1.patch from multani...this time to the correct branch. Refs #5929

comment:27 follow-up: Changed 8 months ago by rwall

  • Keywords review removed

Code Review

Notes:

  • Patch applied cleanly with a few offsets
    [richard@zorin python3-reflect-5929]$ patch -p0 < ~/Downloads/python3-reflect-5929-6.1.patch
    patching file setup3.py
    Hunk #1 succeeded at 80 (offset 1 line).
    Hunk #2 succeeded at 176 (offset 2 lines).
    patching file twisted/python/compat.py
    patching file twisted/python/reflect.py
    patching file twisted/test/test_reflect.py
    
  • Forced Build:

Points:

  1. There are various things from the Python3 porting checklist which haven't been done.(wiki:Plan/Python3#Reviewerchecklist)
    1. Warning when running with python -3
      python3-reflect-5929/twisted/test/test_reflect.py:12: DeprecationWarning: the ihooks module has been removed in Python 3.0
      
      Although I don't understand how the tests pass on the Python3 builder in that case.
    2. Need to add "from future import division, absolute_import" to all ported modules and test modules.
    3. Not sure whether you should use u"" in various string concatenations in reflect.py
    4. Are you sure that InstanceType can safely be replaced by object? Somewhere I read that an isinstance(x, InstanceType) might be being used to distinguish between old and new style classes rather than eg instances versus classes. I'm not sure. Maybe log into #twisted-dev and ask for a second opinion about that change.
  2. twistedchecker reports some new long lines
  3. Add a .misc newsfile.

Please address or answer the numbered points above and submit an updated patch
against the branch I created.

Thanks for persevering. Hopefully the next patch will be ok to merge.

-RichardW.

comment:28 in reply to: ↑ 27 ; follow-up: Changed 8 months ago by multani

  • Author rwall deleted
  1. Are you sure that InstanceType can safely be replaced by object? Somewhere I read that an isinstance(x, InstanceType) might be being used to distinguish between old and new style classes rather than eg instances versus classes. I'm not sure. Maybe log into #twisted-dev and ask for a second opinion about that change.

I'm not sure to exactly understand what you said here.
Python 3 doesn't have the concept of "old" vs "new" style classes, so
there's no possibility of making a distinction (there are all "new"
style classes).
On Python 2.x, twisted.reflect.compat.InstanceType will be an alias
towards types.InstanceType so the behavior should be the same as
before.
If you have an example in mind, I would appreciate it.

However, after re-reading the patch to see where InstanceType would
be a problem, I discovered that the code still refers to
types.ClassType which doesn't exist on Python 3.

I started to add more tests to trigger the bug under Python 3 and fix
it, but it seems to me that it's already kind of broken under Python 2
as well.

For example:

>>> from twisted.python import reflect
>>> class F: pass
... 
>>> reflect.getcurrent(F)
<class __main__.F at 0xb74baf8c>
>>> class G(object): pass
... 
>>> reflect.getcurrent(G)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "twisted/python/reflect.py", line 92, in getcurrent
    assert type(clazz) == types.ClassType, 'must be a class...'
AssertionError: must be a class...
>>> 

twisted.python.reflect.isinst also uses types.ClassType and
getcurrent but it looks like it might be not as broken. I'm not so
sure how to test it though, especially the IS branch.

The more I look at it, the more I'm willing to push those two functions
into a not _PY3 branch since I don't think they make sense with
Python 3 at all.

Any thoughts?

comment:29 in reply to: ↑ 28 Changed 8 months ago by multani

Replying to multani:

I started to add more tests to trigger the bug under Python 3 and fix
it, but it seems to me that it's already kind of broken under Python 2
as well.

... or it was made to work only with old-style class, but being "part of the public API" and without documentation, it's hard to tell.

The more I look at it, the more I'm willing to push those two functions
into a not _PY3 branch since I don't think they make sense with
Python 3 at all.

Specifically, I would like to remove getcurrent and make isinst an alias to the builtin isinstance (if it makes sense to keep isinst at all) from the Python 3 version.
Another possibility, if those functions has to stay for the time being, is to mark them as being deprecated and have something like this for Python 3:

getcurrent = lambda clazz: clazz
isinst = isinstance
# + deprecation warning for those who wants to use these functions

comment:30 follow-up: Changed 8 months ago by itamar

I suggest doing the easy thing for these two. I.e. don't even bother porting to Python 3, open ticket for deprecating on Python 2. There's lots of useless junk in this module.

In general anything deprecated or likely to be deprecated needn't be ported.

comment:31 Changed 8 months ago by itamar

(I can't find any code *using* isinst).

comment:32 in reply to: ↑ 30 ; follow-up: Changed 8 months ago by multani

Replying to itamar:

I suggest doing the easy thing for these two. I.e. don't even bother porting to Python 3, open ticket for deprecating on Python 2. There's lots of useless junk in this module.

In general anything deprecated or likely to be deprecated needn't be ported.

OK, cool :)

So, I created #6829 to add deprecation warnings for those 2 functions.

Replying to rwall:

  1. Warning when running with python -3
    python3-reflect-5929/twisted/test/test_reflect.py:12: DeprecationWarning: the ihooks module has been removed in Python 3.0
    
    Although I don't understand how the tests pass on the Python3 builder in that case.

This warning must have been here before already. I created #6856 to address this issue.

comment:33 in reply to: ↑ 32 Changed 8 months ago by multani

Replying to multani:

Replying to itamar:

I suggest doing the easy thing for these two. I.e. don't even bother porting to Python 3, open ticket for deprecating on Python 2. There's lots of useless junk in this module.

In general anything deprecated or likely to be deprecated needn't be ported.

OK, cool :)

So, I created #6829 to add deprecation warnings for those 2 functions.

I meant #6859

comment:34 follow-up: Changed 7 months ago by rwall

Hi multani,

hynek and I were discussing this in #twisted-dev and wondered what the state of this ticket is. I think there's still work to do - re the points in comment:27 ? It sounded like you'd addressed some of those and then found some new bugs, which itamar decided to sidestep by deprecating some of the old APIs. Is there a new patch available for review? (I couldn't see anything new in the ticket history)

Can you also add a little comment to #6239 (which is up for review) explaining its dependencies. Does it require this branch to be merged first? Does it also require the deprecations in #6859?

Perhaps #6239 should be taken removed from the review queue for now.

Thanks again for your work on this.

comment:35 in reply to: ↑ 34 Changed 7 months ago by multani

Replying to rwall:

Hi multani,

hynek and I were discussing this in #twisted-dev and wondered what the state of this ticket is. I think there's still work to do - re the points in comment:27 ? It sounded like you'd addressed some of those and then found some new bugs, which itamar decided to sidestep by deprecating some of the old APIs. Is there a new patch available for review? (I couldn't see anything new in the ticket history)

Can you also add a little comment to #6239 (which is up for review) explaining its dependencies. Does it require this branch to be merged first? Does it also require the deprecations in #6859?

Perhaps #6239 should be taken removed from the review queue for now.

Thanks again for your work on this.

Sorry for the mess between all those tickets.

  • I addressed all the points by rwall from comment:27 and I have patch which is ready to be send, but ...
  • ... I'm currently waiting for a review on #6856 (I may need to update my current patch, and I think we don't need another one on this (#5929) ticket)
  • as for #6859, I asked on IRC a couple of days ago and hynek (I think) told me to get #5929 merged first (with the functions discussed there available only for Python 2.x) and then propose a deprecation patch in #6859. I'll update this ticket as well to reflect the dependency.

Changed 6 months ago by multani

comment:36 Changed 6 months ago by multani

  • Keywords review added

Thanks for the review rwall!

Now that #6856 has been merged, I'm uploading a new patch which answers rwall remarks.

The patch has been rebased on top of trunk, and it has the changes which have been commited into branches/python3-reflect-5929 as well as the fixes made for the comments below.

I know rwall asked to produce an updated patch against branches/python3-reflect-5929 but it conflicts with the modification from #6856 so I thought it would be better to produce a new, clean patch.

If really preferred, I can send the patch against the branch branches/python3-reflect-5929 in Subversion

Replying to rwall:

Code Review

Points:

  1. There are various things from the Python3 porting checklist which haven't been done.(wiki:Plan/Python3#Reviewerchecklist)
    1. Warning when running with python -3
      python3-reflect-5929/twisted/test/test_reflect.py:12: DeprecationWarning: the ihooks module has been removed in Python 3.0
      
      Although I don't understand how the tests pass on the Python3 builder in that case.

This has been removed in #6856.

  1. Need to add "from __future__ import division, absolute_import" to all ported modules and test modules.

Done.

  1. Not sure whether you should use u"" in various string concatenations in reflect.py

I guessed you meant in objgrep for instance? I would say it's probably safe in most, if not all cases, since repr() and __dict__ returns native strings (str() on Python 2.x and also str() on Python 3.x) which fits with the strings which are concatenated with.

  1. Are you sure that InstanceType can safely be replaced by object? Somewhere I read that an isinstance(x, InstanceType) might be being used to distinguish between old and new style classes rather than eg instances versus classes. I'm not sure. Maybe log into #twisted-dev and ask for a second opinion about that change.

Following the previous messages in comment:28 and comment:29 :

  • I marked the 2 functions discussed above (getcurrent and isinst) Python 2.x only
  • I'm going to propose a patch on #6859 when this ticket gets merged
  1. twistedchecker reports some new long lines

Done

  1. Add a .misc newsfile.

Done

comment:37 Changed 6 months ago by rwall

  • Owner changed from multani to rwall
  • Status changed from new to assigned

Reviewing...

comment:38 Changed 6 months ago by rwall

  • Author set to rwall
  • Branch changed from branches/python3-reflect-5929 to branches/python3-reflect-5929-2

(In [41456]) Branching to 'python3-reflect-5929-2'

comment:39 Changed 6 months ago by rwall

(In [41457]) Apply python3-reflect-5929-7.patch from multani. Refs #5929.

comment:40 follow-up: Changed 6 months ago by rwall

  • Owner rwall deleted
  • Status changed from assigned to new

Thanks multani,

I started reviewing this, but got bogged down trying to understand objgrep.

I've come to the conclusion that I don't grasp this code well enough to complete
the review (maybe I'm too tired). I'll assign it back to noone so that someone
else can have a go.

Here are my notes so far.

Notes:

  1. Created new branch and applied python3-reflect-5929-7.patch

Points:

  1. Some build failures:
    1. https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/python3-reflect-5929-2
    2. This one needs fixing: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1741/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
  1. source:branches/python3-reflect-5929-2/twisted/test/test_reflect.py
    1. test_boundMethod
      1. I was playing with objgrep to try and understand the intent of this test, but its still not clear to me. eg
In [25]: from twisted.names import dns

In [26]: from twisted.python import reflect

In [27]: m = dns.Message()

In [28]: reflect.objgrep(m.encode, m.encode.__self__.__class__, reflect.isSame)
Out[28]: ['.__self__.__class__', '.__self__.__class__']
  1. I asked exarkun in #twisted-dev and he said "m.encode.self.class is reachable from m.encode via two paths ... but they're the same path (seems like a bug)"
    1. Is it because it's still finding the class via im_class?
    2. The same seems to be true on trunk too, so I guess it doesn't matter here.

comment:41 follow-up: Changed 5 months ago by glyph

So, I constructed the same example as rwall, and got this:

>>> objgrep(m.encode, m.encode.__self__.__class__, isSame)
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "./twisted/python/reflect.py", line 269, in objgrep
    objgrep(start.__self__, goal, eq, path+'.__self__', *args)
  File "./twisted/python/reflect.py", line 277, in objgrep
    objgrep(start.__class__, goal, eq, path+'.__class__', *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 264, in objgrep
    objgrep(v, goal, eq, path+'['+repr(k)+']', *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 267, in objgrep
    objgrep(start[idx], goal, eq, path+'['+str(idx)+']', *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 263, in objgrep
    objgrep(k, goal, eq, path+'{'+repr(v)+'}', *args)
  File "./twisted/python/reflect.py", line 279, in objgrep
    objgrep(start(), goal, eq, path+'()', *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 264, in objgrep
    objgrep(v, goal, eq, path+'['+repr(k)+']', *args)
  File "./twisted/python/reflect.py", line 277, in objgrep
    objgrep(start.__class__, goal, eq, path+'.__class__', *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 267, in objgrep
    objgrep(start[idx], goal, eq, path+'['+str(idx)+']', *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 267, in objgrep
    objgrep(start[idx], goal, eq, path+'['+str(idx)+']', *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 263, in objgrep
    objgrep(k, goal, eq, path+'{'+repr(v)+'}', *args)
  File "./twisted/python/reflect.py", line 279, in objgrep
    objgrep(start(), goal, eq, path+'()', *args)
  File "./twisted/python/reflect.py", line 275, in objgrep
    objgrep(v, goal, eq, path+'.'+k, *args)
  File "./twisted/python/reflect.py", line 267, in objgrep
    objgrep(start[idx], goal, eq, path+'['+str(idx)+']', *args)
  File "./twisted/python/reflect.py", line 274, in objgrep
    for k, v in start.__dict__.items():
RuntimeError: dictionary changed size during iteration

which suggests to me that this needs a bit more work :-).

comment:42 Changed 5 months ago by glyph

  • Keywords review removed
  • Owner set to multani

Okay, so, objgrep breaks even my brain sometimes, but, let me complete rwall's review:

  1. .im_self and .im_class used to be two separate attributes. They still are, on Python 2. Given that objgrep is often used to debug extremely tricky GC interactions (let's face it: if you had better options, you would have used them before trying objgrep) it would be best to retain this behavior. In python 3, you can just skip the .__self__.__class__ step, since if you go via .__self__, you'll get to .__class__.

Thanks a bunch for helping us port this module.

comment:43 in reply to: ↑ 41 Changed 5 months ago by multani

Replying to glyph:

So, I constructed the same example as rwall, and got this:

>>> objgrep(m.encode, m.encode.__self__.__class__, isSame)
Traceback (most recent call last):
...
  File "./twisted/python/reflect.py", line 274, in objgrep
    for k, v in start.__dict__.items():
RuntimeError: dictionary changed size during iteration

which suggests to me that this needs a bit more work :-).

So, I had a look, and I managed to reproduce without dns.Message, which seems to be caused by zope.interface. I changed twisted.test.test_reflect.ObjectGrep.test_boundMethod with the following:

from zope.interface import Interface, implementer
class I(Interface):
    pass

@implementer(I)
class Dummy:
    def dummy(self):
        pass

o = Dummy()
m = o.dummy

objgrep(m, m.__self__.__class__, isSame)

And it triggers the same traceback as glyph. Strangely, if I had a print(start) before the switches in the objgrep() function, there's no traceback anymore, but I guess it's not an acceptable solution ._.

comment:44 Changed 5 months ago by multani

Hum, I'm a bit puzzled. There's something going on with zope.interface while inspecting an interface's __dict__:

import sys

from twisted.python import reflect
from zope.interface import Interface, implementer

class I(Interface):
    pass

@implementer(I)
class Dummy:
    def encode(self):
        pass

m = Dummy()

#from twisted.names import dns
#m = dns.Message()

for i in range(400):
    try:
        print(reflect.objgrep(m.encode, m.encode.__self__, reflect.isSame))
        break
    except RuntimeError as e:
        (type, value, tb) = sys.exc_info()
        while tb != None:
            last_tb = tb
            tb = tb.tb_next

        l = last_tb.tb_frame.f_locals
        print(i, e, l['start'])

This piece of code basically reproduce glyph's test with a simpler object and prints the dict object which causes the RuntimeError: dictionary changed size during iteration error. Running this under Python 3 breaks between 13 and 22 times with a different object each time, before giving (finally!) a correct result.

Trying with the dns.Message as originally rwall did is even more interesting, as it displays a bunch of zope.interface's interfaces as well as Twisted's own interfaces before finally returning a result (from about 30 to 70 iterations needed).

I don't observe this behavior with Python 2(.7)

I guess we are observing here the difference between Python 2.7 dict.items()'s which returns a list of key/value and Python 3 dict.items()'s which returns a view:

  • the first one is built and stable when dict.items() is called
  • whereas the second one gets updated when more interfaces are discovered when objgrep-ing the values of this dict, which causes the RuntimeError glyph is seeing.

I can reproduce the same kind of bug if I replace dict.items() by dict.viewitems() on Python 2.7 (with some exception, since the actual dict being iterated over is sometimes a dictproxy which lacks this method.)


If my reasoning is correct, I can mimick the behavior of Python 2.7 in Python 3 by calling list(start.__dict__.items()), but this is barely sufficient:

  • it looks like it hides the problem, since the test I made just above shows that the loop over start.__dict__.items() isn't exactly stable. By this, I mean that potentially objgrep() *may* return different or incomplete things from the object being grep-ed
  • this is even more true since printing the result of reflect.objgrep(m.encode, m.encode.__self__.__class__, reflect.isSame) with the fix I proposed with Python 3 produces a different result each time.

So, I will need advices to know how to continue from this.

comment:45 follow-up: Changed 5 months ago by itamar

objgrep is not a dependency of any production code in Twisted. If you're having trouble porting it, let's just deal with rest of reflect.py in this ticket and move porting objgrep into a different ticket.

comment:46 in reply to: ↑ 45 ; follow-up: Changed 5 months ago by glyph

Replying to itamar:

objgrep is not a dependency of any production code in Twisted. If you're having trouble porting it, let's just deal with rest of reflect.py in this ticket and move porting objgrep into a different ticket.

Sorry, yes, absolutely. This should have been my comment as well. I'm happy to help that other ticket sorted out too, but this one is much more important and is blocking lots of other useful functionality.

comment:47 in reply to: ↑ 46 Changed 5 months ago by multani

Replying to glyph:

Replying to itamar:

objgrep is not a dependency of any production code in Twisted. If you're having trouble porting it, let's just deal with rest of reflect.py in this ticket and move porting objgrep into a different ticket.

Sorry, yes, absolutely. This should have been my comment as well. I'm happy to help that other ticket sorted out too, but this one is much more important and is blocking lots of other useful functionality.

Ok, thanks for the comments, itamar and glyph. I'm going to propose a new patch without objgrep for Python 3. What would be the best way to propose this, considering the situation?

  1. Should I add a new test with the broken behavior exhibited by implementing an interface and skipping it or marking it as "expected failure" when running the tests with Python 3?
  2. Should I let objgrep in the current state, which mostly works in Python 3 but not exactly?
  3. Should I completely disable objgrep and its tests when running Python 3 with a big if _PY3: block? (or should I skip/mark as failure the tests with Python 3)

I'm inclining towards 1+2 but I'm not sure how Twisted wants to handle this.

comment:48 Changed 5 months ago by itamar

Option 3 is best - including half finished changes just adds to the burden on the reviewer.

Changed 5 months ago by multani

comment:49 in reply to: ↑ 40 ; follow-up: Changed 5 months ago by multani

  • Keywords review added

I just uploaded an eighth version of the patch, on top of rwall's branches/python3-reflect-5929-2 branch.

Replying to rwall:

Thanks multani,

I started reviewing this, but got bogged down trying to understand objgrep.

I've come to the conclusion that I don't grasp this code well enough to complete
the review (maybe I'm too tired). I'll assign it back to noone so that someone
else can have a go.

Here are my notes so far.

Notes:

  1. Created new branch and applied python3-reflect-5929-7.patch

Points:

  1. Some build failures:
    1. https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/python3-reflect-5929-2

I didn't do anything related to this failure because:

  • it doesn't have anything to do with my branch
  • I can't reproduce it here
  • it fails on one Buildbot but runs successfully on the other
  • it has something to do with threads

So I expected some kind of spurious error. I will dig more if there's still a failure.

  1. This one needs fixing: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1741/steps/run-twistedchecker/logs/new%20twistedchecker%20errors

Done

  1. source:branches/python3-reflect-5929-2/twisted/test/test_reflect.py
    1. test_boundMethod
      1. I was playing with objgrep to try and understand the intent of this test, but its still not clear to me. eg

[...]

  1. I asked exarkun in #twisted-dev and he said "m.encode.self.class is reachable from m.encode via two paths ... but they're the same path (seems like a bug)"

As said in the comments after yours, it looks like objgrep() has some quirks. In the new version of the patch, this function is deactivated with a _PY3 condition, tests are skipped. I created #6986 to address the problem and finish the port of this function.

comment:50 Changed 5 months ago by rwall

  • Owner changed from multani to rwall
  • Status changed from new to assigned

comment:51 Changed 5 months ago by rwall

(In [41751]) Apply python3-reflect-5929-8.patch from multani. Refs #5929.

comment:52 in reply to: ↑ 49 Changed 5 months ago by rwall

  • Keywords review removed
  • Owner changed from rwall to multani
  • Status changed from assigned to new

Replying to multani:

I just uploaded an eighth version of the patch, on top of rwall's branches/python3-reflect-5929-2 branch.

Thanks multani,

I applied your last patch and ran another build.

There are a couple of twistedchecker errors and some trailing whitespace.

I think the branch now looks fine, Glyph seemed generally satisfied with the
branch and you've raised tickets for future porting objgrep...as itamar
suggested.

So I'll fix those TwistedChecker warnings myself and merge if the builders all
go green.

-RichardW.

comment:53 Changed 5 months ago by rwall

  • Owner changed from multani to rwall
  • Status changed from new to assigned

comment:54 Changed 5 months ago by rwall

Hmm. I fixed one twistedchecker warning and it reports another long line in code which hasn't been changed in this branch.

I think the twistedchecker ratchet builder is confused, so I'll ignore.

comment:55 Changed 5 months ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [41754]) Merge python3-reflect-5929-2

Author: multani
Reviewers: radix, thijs, rwall, glyph
Fixes: #5929
Refs: #6239, #6986, #6859

Port most of twisted.python.reflect to Python3.

Porting objgrep posed some problems and has been moved to a separate ticket #6986.

Porting getcurrent and isinst also posed problems and it was decided to deprecate them in #6859.

Note: See TracTickets for help on using tickets.