Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#7827 enhancement closed fixed (fixed)

Port twisted.persisted.styles to Python 3

Reported by: hawkowl Owned by: Glyph
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: mesozoic Branch: branches/less-work-to-just-port-it-7827-3
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, glyph

Description (last modified by hawkowl)

Ephemeral+sob is used by a bunch of things, so let's port that.

Change History (26)

comment:1 Changed 6 years ago by hawkowl

Author: hawkowl
Branch: branches/whose-styles-is-it-anyway-py3-7827

(In [44141]) Branching to whose-styles-is-it-anyway-py3-7827.

comment:2 Changed 6 years ago by hawkowl

Keywords: review added

Ported all things that were possible. Pickle changes mean that you can't serialise class methods, though.

Builders spun. Please review.

comment:3 Changed 6 years ago by hawkowl

(Tests are all green)

comment:4 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Thanks for working on this.

Regarding the explicit import of StringIO and not reusing the one from compat, I think that this needs a comment to explain why we explicitly need this import. If NativeStringIO from compat is not good enough maybe we should add a BytesStringIO in compat.


For all3 from twisted/persisted/styles.py is there a big problem if we have the same all for py2 and py3 even if versioned don't work on Python3. The advantage of this is that we have less changes and we don't need to write a test for the changes/logic from all3 ... also, in tests we no longer need to conditionally define VersionedSubClass ... just skip the tests


we don't have tests for unpickleMethod ...

I am running the tests as

$ ./build/py33/bin/python admin/run-python3-tests twisted.test.test_persisted

if I apply this diff, py3 tests still pass

diff --git twisted/persisted/styles.py twisted/persisted/styles.py
index ce3755e..62993af 100644
--- twisted/persisted/styles.py
+++ twisted/persisted/styles.py
@@ -57,7 +57,7 @@ def unpickleMethod(im_name,
         unbound = getattr(im_class, im_name)
         if im_self is None:
             return unbound
-        if _PY3:
+        if not _PY3:
             bound = types.MethodType(unbound, im_self)
         else:
             bound = types.MethodType(unbound.__func__, im_self)


This is not related to the changes in this ticket. I see a lot of skips for Py3 tests. Maybe we can create a test decorator to help up reduce the number if if _PY3: skip from the test code.


Beside the missing coverage for unpickleMethod I think that changes looks good. Thanks! Looking forward for trial on py3 :)

needs-changes

comment:5 Changed 6 years ago by hawkowl

Keywords: review added

Thanks Adi!

  • I put some comments about StringIO in there.
  • There is a big problem, as all is our public API. As there's not things importable, they're not part of the public (working) API.
  • I'm not sure about this. Apparently we're supposed to del them, but... I'm not sure about that, really.

I actually am not confident in my porting for the unpickle stuff. Reverting those changes.

Builders spun. Please review.

comment:6 Changed 6 years ago by hawkowl

Owner: hawkowl deleted

comment:7 Changed 6 years ago by Jean-Paul Calderone

We need the old StringIO here because existing code uses it

This might not be as informative an explanation as you might think. Can you explain why the existing code isn't satisfied with BytesIO and why that existing code isn't being ported? Isn't this ticket about porting this module?

comment:8 Changed 6 years ago by Jean-Paul Calderone

Ephemeral is used by a bunch of things, so let's port that.

Are you aware that the decision was previously made not to port Ephemeral?

comment:9 Changed 6 years ago by hawkowl

  • I ran into some problems where StringIO is BytesIO == False and I wasn't sure on how to fix that. Since all this stuff is more or less useless and should be deprecated, I decided to not spend more time on it.
  • I was aware that it was decided that most of it shouldn't be used, but https://twistedmatrix.com/trac/ticket/6911 doesn't mention it (other than it's hard to implement Ephmeral which I think I've tackled). I see that it's used in t.i._baseprocess (which was why I tackled this as a dependency), pb/banana, and IRC support. If it's reasonable to remove the subclassing of BaseProcess (and reasonable further down the line to remove the use of Eph in pb + IRC, if not now -- I am not sure why it may be essential for those yet), and instead deprecate t.p.styles, I would be happy to do so.

comment:10 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Resolution: wontfix
Status: newclosed

Oh. So "Opportunistic" actually means "incomplete". Maybe you should write that on the ticket next time.

#6911 says "Also this code is all kind of unused and should probably be removed instead of ported." That seems like a pretty good hint to me.

If it's reasonable to remove the subclassing of BaseProcess (and reasonable further down the line to remove the use of Eph in pb + IRC, if not now -- I am not sure why it may be essential for those yet), and instead deprecate t.p.styles, I would be happy to do so.

Removing using of Ephemeral is what we've already done elsewhere in Twisted. I don't know why BaseProcess or PB or IRC would be any different.

comment:11 Changed 6 years ago by Glyph

Author: hawkowlhawkowl, glyph
Branch: branches/whose-styles-is-it-anyway-py3-7827branches/less-work-to-just-port-it-7827-2

(In [44211]) Branching to less-work-to-just-port-it-7827-2.

comment:12 Changed 6 years ago by Glyph

Resolution: wontfix
Status: closedreopened

comment:13 Changed 6 years ago by Glyph

Keywords: review added

As I said on the other ticket, I think it will be less work to just port twisted.persisted completely than to keep running into it as a blocker that some other thing imports. I do believe we should get rid of it eventually, but it seems to be more effort to keep conflating the process of removing it from modules from the process of porting to py3.

Since exarkun raised the concern that an incomplete port would not be useful, I have tried to make all the existing tests pass. That's probably not sufficient; I haven't looked at coverage yet. But it ought to get us most of the way there.

comment:14 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set to Glyph
Status: reopenednew

Thanks for the changes. They look good.


I think that this code needs a comment as I find it hard to read... and so does hawkowl

*([self.my_class]*(bytes is str))

In unpickleMethod , I think that except AttributeError should keep the comment as otherwise I find the code hard to read.


See twistedchecker errors.

(view as text)
************* Module twisted.persisted.aot
C0103:225,26:indentify: Invalid name "IGNORED" for type variable (should match ([a-z_][a-zA-Z0-9]*)$)
************* Module twisted.persisted.styles
W9202: 47,0:_methodFunction: Missing epytext markup @param for argument "classObject"
W9202: 47,0:_methodFunction: Missing epytext markup @param for argument "methodName"
W9204: 47,0:_methodFunction: Missing epytext markup @return for return value
W9209: 80,0:_pickleFunction: Empty docstring
W9011: 82,0: Blank line contains whitespace
C0301: 84,0: Line too long (81/79)
W9013: 87,0: Expected 3 blank lines, found 2
W9209: 87,0:_unpickleFunction: Empty docstring
************* Module twisted.test.test_persisted
C0301:278,0: Line too long (83/79)

Changes looks good. I tried to do heuristic changes in the new code and see if tests still pass but I failed... so it looks like coverage is OK.

Since we don't have an aggregated coverage for py2 and py3 and trial is not yet ported on Py3 is ugly to get a converage report.

There is a missing news file.

I think that after fixing the current twistedchecker errors the code is good to merge.

Thanks!

comment:15 Changed 6 years ago by hawkowl

Hi Glyph!

  • aot.py, crefutil.py, sob.py, styles.py, test_persisted.py, test_sob.py are missing future imports.
  • aot.py:225 -- Could these variables be expanded/commented? I can't figure out what they really are.
  • aot.py:381 -- wtf is *([im_class]*(bytes is str))
  • styles.py:81,88 -- Needs docstrings
  • test_persisted.py:19 -- I think you want BytesIO or maybe twisted.python.compat.NativeStringIO if you really want a StringIO on Py3
  • No news file

Otherwise... it all looks good. I ran coverage locally, and the only things I noticed:

  • styles.py 275-286 are uncovered
  • crefutil.py is spotty overall, mostly _Container, _Tuple, _Defer.addDependent.
  • aot.py has a bunch of uncovered error conditions
  • sob.py is nearly completely covered apart from a handful of error handlers.

Please address the dot points above (plus Adi's) and please consider if we're missing any useful coverage, then please put up for review again.

comment:16 Changed 6 years ago by hawkowl

Description: modified (diff)
Summary: Opportunistic port of twisted.persisted.stylesPort twisted.persisted.styles to Python 3

(Fixing the title a bit)

comment:17 Changed 6 years ago by mesozoic

Cc: mesozoic added

comment:18 Changed 6 years ago by Glyph

Branch: branches/less-work-to-just-port-it-7827-2branches/less-work-to-just-port-it-7827-3

(In [45049]) Branching to less-work-to-just-port-it-7827-3.

comment:19 Changed 6 years ago by Glyph

Hi Glyph!

future imports

All added.

aot.py:225 -- Could these variables be expanded/commented? I can't figure out what they really are.

Done. It's really just the documentation for generate_tokens, but no reason to make this more obscure than it has to be.

aot.py:381 -- wtf is *([im_class]*(bytes is str))

^______________________^

(Seriously though I pulled this out to a function and did it in a less clever way so it's clearer what's going on.)

styles.py:81,88 -- Needs docstrings

Fixed.

test_persisted.py:19 -- I think you want BytesIO or maybe twisted.python.compat.NativeStringIO if you really want a StringIO on Py3

So, I'm really just trying to test the code that was there before, which was code that let us pickle an old-school StringIO.StringIO. Which is not a particularly great idea, honestly ;-). On py3, those tests weren't doing anything meaningful since our registered copier wouldn't even be invoked. I added another test case that made it super explicit what should happen when we pickle on py2 and unpickle on py3.

No news file

I went with a .misc topfile because this is only being ported as an internal implementation detail of other things, and I don't want someone to read the changelog and say "ooh, twisted.persisted, what's that!" and start using it.

Otherwise... it all looks good. I ran coverage locally, and the only things I noticed:

Thanks for doing this. We should probably have a combined coverage builder...

styles.py 275-286 are uncovered

I don't understand how this is happening. I can reproduce it, but twisted.test.test_persisted.VersionTests.test_versionUpgrade is meant to test exactly this case, and it is covered. I am still working this out.

crefutil.py is spotty overall, mostly _Container, _Tuple, _Defer.addDependent.

OK, so, crefutil.py is now at 100%. Some of that code was just dead, and I removed it. I had to think about it way too hard for a branch that was meant to be a quick hack, but given that crefutil needs to live on for PB (it's used by Jelly) and is probably not going to just die when the rest of twisted.persisted does, it was worth some effort.

aot.py has a bunch of uncovered error conditions

Most of these are not very interesting and AOT is destined for the grave anyway; it imports on py3 which is the main thing. I will probably fix it up a bit more as I look into the test_versionUpgrade problem and wait for the builds.

sob.py is nearly completely covered apart from a handful of error handlers.

OK, those are covered now.

Please address the dot points above (plus Adi's) and please consider if we're missing any useful coverage, then please put up for review again.

The one point adi seemed to raise separately was this:

In unpickleMethod, I think that except AttributeError should keep the comment as otherwise I find the code hard to read.

I restored that and refactored the code to make it more readable, and clearer what except AttributeError is actually supposed to be catching (that try covered way too much code).

comment:20 Changed 6 years ago by Glyph

Keywords: review added
Owner: Glyph deleted

Definitely more work than I anticipated, but still less work than waiting for the whole thing to get deprecated and removed.

comment:21 Changed 6 years ago by Glyph

Tests are passing. I still have no idea what's going on with test_versionUpgrade but I am starting to suspect a bug in coverage.py. In any case I think it would be more appropriate to address that in a follow-up, since coverage is the same in 2 and 3.

comment:22 Changed 6 years ago by hawkowl

From IRC:

[10:04am] hawkowl:
glyph: https://github.com/twisted/twisted/compare/trunk...less-work-to-just-port-it-7827-3#diff-b41e9c4ea4fa457ba43cb0a7ced808d2R537 doesnt follow the pattern that the others do
[10:04am] hawkowl:
(eg https://github.com/twisted/twisted/compare/trunk...less-work-to-just-port-it-7827-3#diff-b41e9c4ea4fa457ba43cb0a7ced808d2R482 )
[10:05am] hawkowl:
glyph: also https://github.com/twisted/twisted/compare/trunk...less-work-to-just-port-it-7827-3#diff-c63f5ff5e98cbacbd79a6e22635df6d0R115
[10:06am] glyph:
hawkowl: oh, it's still in there?
[10:06am] hawkowl:
glyph: yep
[10:06am] hawkowl:
well in one spot
[10:06am] hawkowl:
glyph: also https://github.com/twisted/twisted/compare/trunk...less-work-to-just-port-it-7827-3#diff-9af8b3542062dbdd36722f6419b22f12R198 says "This is only called on Python 2" but the others arent

comment:23 Changed 6 years ago by hawkowl

Keywords: review removed
Owner: set to Glyph

Otherwise, I think it looks good. At least, better than before. Please fix up the little issues I put above and then merge.

comment:24 Changed 6 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [45146]) Merge less-work-to-just-port-it-7827-3: port styles to py3

Author: glyph

Reviewer: hawkowl, adiroiban

Fixes: #7827

twisted.persisted is not a good package. don't use it. But, some things within Twisted still use it, and attempting to work around its absence while porting those things to python 3 was taking more time than just porting the code, so that's what this change does. some parts of it (such as crefutil) are actually used by reasonable things we intend to keep maintaining, such as jelly, so this brings the quality of the code up a little bit and improves coverage as well.

comment:25 Changed 6 years ago by Daniel Graña

Just to let you know this ticket fired a regression described in #7989

comment:26 Changed 6 years ago by Glyph

Thanks for updating the link, dangra.

Note: See TracTickets for help on using tickets.