Remove dependency on six - we're Py3 only now!#11158
Remove dependency on six - we're Py3 only now!#11158jklymak merged 5 commits intomatplotlib:masterfrom
Conversation
|
We discussed a bit on the weekly call. I'm a little hazy on the exact implementation, but I think that the issue here is not the PR per se, which is against 3.0, but if someone writes code that is meant to be back ported as well as going against master they will not write python-2 compatible code if all the six stuff is taken out. Other devs piped up that they aren't dropping harmless py2 stuff until they end long-term-support of 2.0. I'm not experienced enough with transitions like this to know whether this is all manageable, but my personal vote would be to hold off on this sort of change until we have it all sorted out. |
|
Well, I didn't mean for you to close it yet - I was hoping someone more experienced would weigh in... I'll re-open just because we really should understand the py3 migration path... |
| PyEval_RestoreThread: NULL state bug on win32 | ||
| """ | ||
| _focus = windowing.FocusManager() | ||
| windowing.FocusManager() |
There was a problem hiding this comment.
That actually relies on _focus being garbage collected at the end of the function (IOW FocusManager has a non-trivial __del__, would better be rewritten as a contextmanager nowadays but that's another story).
There was a problem hiding this comment.
Oh... dear. It's actually not used in many places, but I'll convert it in a seperate pull.
| return parts | ||
|
|
||
|
|
||
| def remove_coding(text): |
There was a problem hiding this comment.
This isn't public API, is it? It looks to me like an internal helper for the Sphinx extension, and nobody downstream should be importing that.
There was a problem hiding this comment.
Far too many things that should not be public API unfortunately are.
|
I am 👍 on this going in (assuming tests pass and public API is properly deprecated). As per https://matplotlib.org/devel/coding_guide.html#branches-and-backports
Mercifully the critical bugs have been rare and the rate of regressions being reported has gone down quite a bit. If we are going to hedge on keeping master branch mostly py2 compatible we don't get much by going py3 only! |
|
I’m fine w that. But what’s the path if I need to fix a regression in 2.2.3? Code versus master and then fix the backport manually adding back in any six code needed? |
|
My main concern is about changes like these is that it will make it more
likely for other backports to encounter conflicts, which makes it more work
for us to perform backports. And it isn't like six is keeping us from
taking advantage of py3k features. We are still going ahead with kwarg-only
arguments (although, I am a bit wary of that causing merge conflicts, too).
But usage of six is all over the codebase.
…On Wed, May 2, 2018 at 7:27 PM, Jody Klymak ***@***.***> wrote:
I’m fine w that. But what’s the path if I need to fix a regression in
2.2.3? Code versus master and then fix the backport manually adding back in
any six code needed?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-OvhBWXyMbsUr9jpTgnftc7t3pOqks5tukDPgaJpZM4Tvk2D>
.
|
|
If I recall correctly the main argument for dropping py2 support is to relieve strain from developpers so they can spend their time on implementing new features instead of working around incompatibility issues. |
|
If someone really wants to revert the removal of future imports that I did, go ahead, I honestly don't care that much. |
|
I'm also in favor of stripping out six now. It improves readability. I doubt it will substantially impede critical backports. (Many of the uses of six--many instances of 'six.iteritems' for example--are not even necessary for compatibility.) |
|
Again, I am not super worried about backport because they should only be for critical bugs and regressions. If the backports are getting harder (and they are going to get harder independent if we strip six out or not) we then we do less non-critical backports.
I would say no, we should not be making any concessions to py2 compatibility in new code. |
|
Thanks @efiring, @WeatherGod , @tacaswell @anntzer and @ImportanceOfBeingErnest I think that clarifies things for me a fair bit; to paraphrase:
|
|
Thanks for all your comments - I will keep working on this as the majority opinion in in favor. My main motivation is to remove a layer of unnecessary complexity - contributing to Matplotlib (or just reading the code to debug a plot) sometimes feels like death by a thousand papercuts. Removing |
|
Mind that this is not only about code, it is also about documentation. An error in the docs can be as critical as an error in the code from a user's perspective. I recently submitted a PR to fix the documentation, which currently has a major part of it completely undocumented (the axes_grid1 and axisartist toolkits). This could not be backported to the current doc branch because some Ideally there should be the option to pull/push from/to the p2 branch independently with a clear manual on how to do that. There should be ci testing available to make sure fixes don't break anything. You can then leave it to the people doing the PRs to make sure everything runs smoothly. On a by-case basis critical fixes can be brought to that branch by the dev team, but essentially you have the freedom to completely transfer responsibility (appart from reviewing) to the contributors. The question whether or not a bug is critical is then simply a question of whether someone is willing to do that extra work (which usually isn't that much anyways). |
|
Surely anyone could Backports aren't automatic, and taking six out doesn't make them harder. |
|
Actually, we do have an automation system for backports that merge cleanly.
It is pretty awesome.
…On Thu, May 3, 2018 at 9:09 AM, Zac Hatfield-Dodds ***@***.*** > wrote:
Surely anyone could got cherry-pick the commits, fix merge conflicts, and
open a PR against the 2.2 branch though? I don't see what makes this harder
than any other contribution.
Backports aren't automatic, and taking six out doesn't make them harder.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-PbUwmKX2LN3rRqEXB-utTa8GlRWks5tuwGYgaJpZM4Tvk2D>
.
|
|
I've seen that and it's great, but a clean merge is not always a correct merge - there should still be a human in the loop! We're getting off topic though - I'd appreciate it if we could stick at least mostly to review of code. I'll push an update tomorrow with a first round of fixes... |
|
I think a PR that changes 70 files is an appropriate place to discuss the practical implications of the PR for back porting. |
I agree errors in documentation as as or more important than bugs, however we should apply the same criteria to documentation as we do to code. Those modules have never been documented so adding them is not fixing a regression from 2.x series.
We should broaden the number of people who feel comfortable doing manual backports then. We have some minimal instructions at https://matplotlib.org/devel/coding_guide.html#manual-backports What do we need to add to that to make it clearer?
You can checkout and open PRs against 2.2.x and CI is still enabled on that branch. However, if we start to accumulate lots of development on the v2.2.x branch we will then have to deal with forward porting and backporting will only become more difficult so I am not excited about this.
Reviewing is where our bottleneck is. So far no one has volunteered to be the v2.2 release manager. If someone does I will be happy to defer to what ever they want to do on many of these positions. |
|
@tacaswell I will comment on the respective PR that I mentionned in a minute, not to clutter this discussion here. I think there has to be a point in time from which on the two branches are simply coexisting and diverging. Up to that point it does not make sense to disassemble the compatible code for pure reason of beautification or ease of read. After that point, master branch can be completely freed from anything that people don't like about py2. (Although my argument about resources spent on that work would still apply.) If then some bugfix concerns both and is critical enough, it needs two PRs, one for each branch. While creating a double PR can be delegated to the contributor, merging can not. Also, if a bug is fixed in master, it should in most cases be trivial to fix it in the 2.2 branch as well, mostly replicating the fix and working around the differences between py2/py3. If reviewing is a bottleneck, one smight think about delegating more of that as well. Maybe it can be separated from the rights to merge? |
There's nothing stopping non-committers from writing reviews, it works just fine today. They just don't appear as the green check mark. I'm really not buying this argument that BUG FIXES, specifically regressions from >=2.0, are going to have such a large surface area that the removal of six is going to create this huge burden for back ports. Honestly, this feels like trepidation at the dropping of 2.7 (from those who still have to use it) rather than a solid technical concern. |
|
Honestly, this feels like trepidation at the dropping of 2.7 (from those
who still have to use it) rather than a solid technical concern.
This has *nothing* to do with trepidations at the dropping of 2.7. We are
already well-committed down that path with the changes in master as it
stands. I have put up *zero* resistance to the transistions to keyword-only
arguments and such (even gave a +1 in another issue to rework a bit of
mplot3d code to facilitate that).
Unlike changes to our imports and changes to the function call signatures,
removing six is a change that is spread through-out the codebase. The
chances for a backport conflict increases, which makes for extra work that
would not have been needed if six is left in the codebase right now. Also,
as noted before, it becomes more likely that we will accidentally backport
broken py27 code if we don't code using six for the near-term future. Also,
not removing six does not impede adoption of py3k features.
The only benefit I have seen suggested is improved readability. Quite
frankly, I have gotten so used to six the past few years, that I really
don't notice it anymore. Now, compare that against increased work for
backports and accidental introduction of subtle bugs for what is supposed
to be bugfix versions, I really don't see the value proposition here.
As for release manager for v2.2.x, I think we all know that I was going to
take that role, I was just hoping someone else would step up instead...
…On Thu, May 3, 2018 at 5:32 PM, Ryan May ***@***.***> wrote:
If reviewing is a bottleneck, one smight think about delegating more of
that as well. Maybe it can be separated from the rights to merge?
There's nothing stopping non-committers from writing reviews, it works
just fine today. They just don't appear as the green check mark.
I'm really not buying this argument that *BUG FIXES*, specifically
regressions from >=2.0, are going to have such a large surface area that
the removal of six is going to create this huge burden for back ports.
Honestly, this feels like trepidation at the dropping of 2.7 (from those
who still have to use it) rather than a solid technical concern.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-AEjNjnqP5KvRIvN68JGT1x5QfCyks5tu3d9gaJpZM4Tvk2D>
.
|
|
On 2018/05/03 11:47 AM, Benjamin Root wrote:
Also,
as noted before, it becomes more likely that we will accidentally backport
broken py27 code if we don't code using six for the near-term future.
To me, this idea of continuing to code using six on master makes no
sense at all, in the context of master as a py3-only codebase--which it
already is, as was planned. I don't think developers on master should
be burdened by having to keep track of whether six would be needed if
the code were backported.
As for breakage: the backports will still be tested against py27.
If there is a backport conflict related to six, it should be pretty
obvious and easy to fix.
There should not be a large number of backports if we stick to Thomas's
criteria.
|
| else: | ||
| if six.PY2 and isinstance(s, six.text_type): | ||
| s = s.encode("utf-8") | ||
| s = str(s) |
There was a problem hiding this comment.
attn @anntzer
This logic does not seem identical to be, but I am not sure of the context.
There was a problem hiding this comment.
The else block is a no-op on Python 3, so it can be removed. That leaves the former block, and we can simplify the condition by observing that str(some_string) is a no-op too, and therefore calling s = str(s) unconditionally.
There was a problem hiding this comment.
Pretty sure the call to str/six.text_types was only necessary because of unicode/str issues; now s should always be a str and we shoudn't even have to call str(s).
|
Ping @tacaswell - I've rebased on master to resolve a merge conflict. Would you mind reviewing the other half of the diff so we can merge this before another conflict comes up? |
| # save dir for next time | ||
| rcParams['savefig.directory'] = os.path.dirname( | ||
| six.text_type(fname)) | ||
| str(fname)) |
lib/matplotlib/patches.py
Outdated
| for argname, argdefault in zip(args[1:], defaults)] | ||
| spec = inspect.getfullargspec(cls.__init__) | ||
| if spec.defaults: | ||
| argstr = ",".join("%s=%s" % pair for pair in zip( |
There was a problem hiding this comment.
I would still break at the for here (and the , if necessary).
| return base ** lx | ||
|
|
||
|
|
||
| def nearest_long(x): |
There was a problem hiding this comment.
round does banker's rounding in Python 3; nearest_long does not.
There was a problem hiding this comment.
...yep, that's not round. It's int(x +/- .5). /facepalm.
There was a problem hiding this comment.
Sorry for being unclear; it's not int either, it's whatever the other kind of rounding is.
There was a problem hiding this comment.
Yep, round-half-to-zero. Fixed by inlining the logic in the two places it was needed, and just using round when we know the value is very close to an integer.
There was a problem hiding this comment.
Also, public api, yada yada.
6132674 to
de30254
Compare
lib/matplotlib/ticker.py
Outdated
| return self._non_decade_format(sign_string, base, fx, usetex) | ||
| elif usetex: | ||
| return r'$%s%s^{%d}$' % ( | ||
| sign_string, base, int(fx + (-.5 if fx < 0 else .5))) |
There was a problem hiding this comment.
actually fx is close to int here (the "not is_x_decade" case is handled just above) so it has already been rounded above, so you don't need to call any of nearest_long, round, or your inlined version.
20ba8d4 to
245bba9
Compare
|
Hi @anntzer & @tacaswell - I've rebased on master, split it into separate commits for easier reviewing, and deprecated instead of removing the various potentially-public functions. Ready for final review! Please leave a fresh comment if I've missed anything - this has been a long thread and I'm keen to get this merged before I have to rebase again 😄 |
lib/matplotlib/tests/test_basic.py
Outdated
| else: | ||
| builtins = sys.modules['__builtin__'] | ||
|
|
||
| builtins = sys.modules['builtins'] |
|
Technically you also need an entry in doc/next_api_changes/ but I'm not going to hold up the PR for that (and likewise I can handle the remaining comments myself in a separate PR, treat that as a future todo :p). |
lib/matplotlib/patches.py
Outdated
| if spec.defaults: | ||
| argstr = ",".join("%s=%s" % pair | ||
| for pair in zip(spec.args[-len(spec.defaults):], | ||
| spec.defaults)) |
There was a problem hiding this comment.
would write this as
",".join(map("{}={}".format, spec.args[-len(spec.defaults):], spec.defaults))
(the rarely seen but ever useful implicit zip-and-star in multi-arg map).
(would also leave a space after the comma, but that's a separate point)
| else: | ||
| pwd = os.getcwd() | ||
| pwd = os.getcwd() | ||
| old_sys_path = list(sys.path) |
There was a problem hiding this comment.
would rewrite as sys.path.copy(), otherwise it's not immediately clear what's happening.
| sys.stdout = io.StringIO() | ||
| else: | ||
| sys.stdout = cStringIO.StringIO() | ||
| sys.stdout = io.StringIO() |
There was a problem hiding this comment.
see if contextlib.redirect_stdout works here
There was a problem hiding this comment.
probably would, but that's a larger change so I'm leaving it for later.
| six.exec_("__name__ = '__main__'", ns) | ||
| code = remove_coding(code) | ||
| six.exec_(code, ns) | ||
| exec("__name__ = '__main__'", ns) |
There was a problem hiding this comment.
ns["__name__"] = "__main__"
|
Thanks @anntzer! I've done the easy bits here 😄 I think So... merge please? |
|
@Zac-HD Remember we a) have a really long review queue; this is 10-days old, @anntzer has some things that are 10 months old I'm sure he'd like reviewed as well; b) we require two reviews before things get merged. So you'll have to get someone else excited about going through this 😉 Its not going to be me, because my knowledge of py2->py3 is pretty minimal. @tacaswell started, but is often quite busy, but he seemed interested so I'm sure he'll get to it. |
|
Ping @jklymak, with many thanks 😄 |
|
Merged. Thanks a lot, that looked like a lot of work! Hopefully our tests catch any possible ways this could go wrong. |
This should be pretty simple (if tedious) to review -
sixprovides very simple translations so each snippet is obvious; there's just a lot of them!