My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
Hi,
Raymond Hettinger used a regression that I introduced in the builtin sorted() function (in Python 3.6.0) to give me his feedback on my FASTCALL work, but also on Argument Clinic.
Context: http://bugs.python.org/issue29327#msg285848
Since the reported issues is wider than just FASTCALL, including how I contribute to CPython, I decided to discuss the topic with a wider audience. I continue the discussion on python-committers to get the opinion of the other core developers.
Sorry for my very long answer! I tried to answer to each issues reported by Raymond.
Inaccurate summary: I'm a strong supporter of "it's better to ask forgiveness than permission", whereas Raymond considers that I introduced too many regressions with my workflow.
Raymond Hettinger added the comment:
A few random thoughts that may or may not be helpful:
We now have two seasoned developers and one new core developer that collectively are creating many non-trivial patches to core parts of Python at an unprecedented rate of change. The patches are coming in much faster than they can reasonably be reviewed and carefully considered, especially by devs such as myself who have very limited time available. IMO, taken as whole, these changes are destabilizing the language. Python is so successful and widely adopted that we can't afford a "shit happens" attitude. Perhaps that works in corners of the language, infrequently used modules, but it makes less sense when touching the critical paths that have had slow and careful evolution over 26 years.
Besides the volume of patches, one other reason that reviews are hard to come by is that they apply new APIs that I don't fully understand yet. There are perhaps two people on the planet who could currently give thoughtful, correct, and critical evaluation of all those patches. Everyone else is just watching them all fly by and hoping that something good is happening.
Since one or maybe even two years, I noticed that many of my issues were blocked by the lack of reviews. As you wrote, only few developer have the knowledge and background to be able to provide a good review (not only "tests pass, so LGTM") on my changes modifying the Python core.
I also wanted to discuss this topic, but I didn't really know what to propose. Let's take this opportunity to explain how I contribute to CPython, especially how I decide to wait for a review or not.
For each patch that I write, I estimate the risk of regression. You may know that any regression is something unexpected, so such estimation is tricky. Here is my heuristic:
(*) if the patch is trivial (short, non controversal), I push it immediatly.
(*) If I'm less confident, I open an issue and attach the patch. I wait at least one day before pushing.
It's strange, but the process of opening an issue and attaching the patch usually helps to review the code myself (find bugs, or more generally enhance the patch). Maybe because it forces me to review the change one more time?
If the change is not part of a larger patch serie, so doesn't block me to move further, I try to keep the issue open around one week.
The truth is that too few of my patches get a review :-/ Maybe I should wait longer, but then it becomes harder for me to handle many patches.
Maybe it's a tooling issues. Recently, I started to use local branches in a Git repository. It helps a lot of work on parallel on large changes. Before, I only worked in a single directory (default/, the default Mercurial branch) and applied/reverted patches everytime. It's painful, especially when I have to use multiple computers, download again publshed patches, etc. Maybe it will run smoother once CPython will move to Git and GitHub.
By the way, it's painful to squash a long patch serie into a giant patch, much harder to review, where changes don't make sense at all at the first look. Again, a better reviewing tool supporting patch series (GitHub) will help here too.
Not supporting patch series in our reviewing tool also explains why I prefer to push than having to wait for a review. Rebasing manually long patch series stored as giant .patch files is complicated.
(*) If the change changes an API or changes a core component, I wait for at least one review from a core reviewer. Sometimes, I even send an email to python-dev. Again, sometimes I don't get any feedback on the patch nor the email after two weeks :-/ At least, I tried :-) Usually, I get feedback in less than one week, or no feedback at all. I understand that nobody understands my change or nobody cares :-)
I totally understand that most core developers have a little amount of time available to contribute to Python. I'm trying to find a compromise between the risk of introducing regressions and being stuck in my work. This email might help me to adjust my workflow.
By the way, I'm trying to always run the full test suite (./python -m test -rW -j0) before pushing any change. If I suspect that I may have introduced reference leaks, I also run "./python -m test -R 3:3 ..." on the tests related to the modified code to check for memory/reference leaks.
- One other reason for the lack of review comments in the enthusiasm and fervor surrounding the patches. I feel like there is a cost of questioning whether the patches should be done or how they are done, like I am burning little karma every time. Sometimes it feels safest and most cordial to just say nothing and let you make hundreds of semi-reviewed changes to just about every critical part of the language.
"semi-reviewed". Let me be more accurate: yeah, I do push a lot of changes which were not reviewed by anyone (see above).
- Historically, if there was creator or maintainer of the code who was still active, that person would always be consulted and have a final say on whether a change should be applied. Now, we have code constantly being changed without consulting the original author (for example, the recent and catastrophic random initialization bug was due to application of a patch without consulting the author of _randommodule.c and the maintainer of random.py, or this change to sorted(), or the changes to decimal, etc).
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
My problem is that I'm not aware of any explicit list of maintainers. I didn't know that you were the maintainer of the random module before you told me that at the Facebook sprint last september. I didn't expect that the random module had a maintainer, I thought that any core developer would be allowed to modify the code.
Moreover, since I open an issue for most of my changes, it gives an opportunity to maintainers to review changes. Maybe we need more components in the bug tracker to notify maintainers of pending changes?
You mentionned 3 different changes, let me reply.
(1) The random change: http://bugs.python.org/issue29085
I introduced a regression in random.Random.seed(): a typo in the C code has the consequence that the current time and process identifier is used, instead of os.urandom(16), to initialize the Mersenne Twister RNG.
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
Since we are talking about the development process, let me see how the change was made.
Context: The PEP 524 has a long and painful history... Something like more than 500 messages were sent on the bug tracker and python-dev, and nobody was listening to each others, two security experts "rage-quitted" Python because of this mess... I decided to try to fix this issue in a constructive way, so I wrote a PEP. Nick wrote a different PEP, since it was clear that it was possible to handle security in two different incompatible ways. A mailing list was even created just to discuss this bug! A mailing list just for a bug gives an idea of the size of the mess :-)
Well, about the change itself, it was done in http://bugs.python.org/issue27776
The patch was available for review during 19 days (2016-08-18-2016-09-06) and was reviewed by Nick Coghlan. Since Nick wrote a similar PEP, I trusted him to be able to review my change. (Well, anyway I already trust all core developers, but I mean that I was trusting him even more than usual :-))
Since the change has a big impact on security, I had prefer to get a review of more developers, especially our security experts... but as I wrote, two security experts "rage- quitted". Again, this PEP has a long and sad story :-/
Note: you say that you are the maintainer of the random module, but I don't recall having see you in any recent discussions and issues related to os.urandom(), whereas a lot of enhancements and changes were done last 2 years. I made many changes to support new OS functions like getentropy() an getrandom().
Oooookay, let's see the second change, "this change to sorted()", http://bugs.python.org/issue29327
(2) I introduced a bug in sorted(), last August: https://hg.python.org/cpython/rev/15eab21bf934/
Calling sorted(iterable=[]) does crash. To be honest, I didn't imagine that anyone would pass the iterable by keyword, but Serhiy is very good to spot bugs in corner cases :-)
IMHO the regression is subtle.
When I optimized the code to use FASTCALL, I replaced PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1). I checked that all tests passed, so it looks ok to me.
I didn't imagine that anyone would call sorted(iterable=[]), so I didn't notice that PyTuple_GetSlice() can create an empty tuple.
The previous code was wrong since sorted() accepted iterable as a keyword, whereas sort.list() doesn't.
So well, I let you guess if a review would have spot this bug in the large change.
(3) Recently, I ran sed to replace code patterns to use faster ways to call functions: https://hg.python.org/cpython/rev/54a89144ee1d
"Replace PyObject_CallObject(callable, NULL) with _PyObject_CallNoArg(callable)"
I recalled that I modified the _decimal module and that Stefan Krah complained, because he wants to have the same code base on Python 3.5, 3.6 and 3.7. He also mentionned an external test suite which was broken by recent _decimal changes (not sure if my specific change was in cause or not), but I wasn't aware of it.
To be honest, I didn't even notice that I modified _decimal when I ran sed on all .c files. Since the change was straightforward and (IMHO) made the code more readable, I didn't even wait for a review if I recall correctly.
Stefan and me handled this issue privately (he reverted my change), I'm not sure that it's worth it to say more about this "issue" (or even "non-issue").
To be clear, I don't consider that my change introduced a regression.
In general, Guido has been opposed to sweeping changes across the code base for only tiny benefits. Of late, that rule seems to have been lost.
The benefits of FASTCALL mainly apply to fine grained functions which only do a little work and tend to be called frequently in loops. For functions such as sorted(), the calling overhead is dominated by the cost of actually doing the sort. For sorted(), FASTCALL is truly irrelevant and likely wasn't worth the complexity, or the actual bug, or any of the time we've now put in it. There was no actual problem being solved, just a desire to broadly apply new optimizations.
Ok, first, you qualify my FASTCALL changes as code churn. So let me show an example with sorted(): https://hg.python.org/cpython/rev/b34d2ef5c412
Can you elaborate how such change increases the complexity?
Second, "no actual problem being solved"
Since the goal of FASTCALL is to optimize Python, I guess that you consider that the speedup doesn't justify the change. I gave numbers in the issue #29327:
Microbenchmark on sorted() on Python 3.7 compared to 3.5 (before FASTCALL):
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10))' 'sorted(seq)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 1.07 us +- 0.06 us -> [3.7] 958 ns +- 15 ns: 1.12x faster (-11%)
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10)); k=lambda x:x' 'sorted(seq, key=k)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 3.34 us +- 0.07 us -> [3.7] 2.66 us +- 0.05 us: 1.26x faster (-21%)
IMHO such speedup is significant even on a microbenchmark. Can you elaborate what are your criteria to decide if an optimization is worth it?
- Historically, we've relied on core developers showing restraint. Not every idea that pops into their head is immediately turned into a patch accompanied by pressure to apply it. Devs tended to restrict themselves to parts of the code they knew best through long and careful study rather sweeping through modules and altering other people's carefully crafted code.
Should I understand that I should restrict myself to some files? Or not touch some specific parts of Python, like... "your" code like random, itertools and collections modules?
I replied to the 3 issues you mentioned previously and explained how I contribute to Python.
FWIW, I applaud your efforts to reduce call overhead -- that has long been a sore spot for the language.
Guido has long opposed optimizations that increase risk of bugs, introduce complexity, or that affect long-term maintainability. In some places, it looks like FASTCALL is increasing the complexity (replacing something simple and well-understood with a wordier, more intricate API that I don't yet fully understand and will affect my ability to maintain the surrounding code).
I'm sorry, I didn't spent much time on explaing the FASTCALL design nor documenting my changes. It's partially deliberate to make everything related to FASTCALL private. Since it's a huge project modifying a lot of code, I wanted to wait until the APIs and the code stop moving too fast to take time to explain my work and document it.
If you have specific questions, please go ahead.
Shortest summary:
FASTCALL replaces (args: tuple, kwargs: optional dict) with (args: C array, nargs: int, kwnames: tuple of keyword keys). It's a new calling convention which allows to avoid a temporary tuple to pass positional arguments and avoids temporary dictionary to pass keyworkd arguments.
To use FASTCALL, C functions should be converted to the new METH_FASTCALL calling convention
PyObject_Call() can replaced with _PyObject_FastCallKeywords() or _PyObject_FastCallDict() (when we still get kwargs as a dict) in such conversion
Many existing C functions were optimized internally to use FASCALL, so even if you don't modify your code, you will benefit of it (speedup). Typical example: PyFunction_CallFunctionObjArgs().
The most massive change were purely internal and don't affect the most famous C APIs at all. In some cases, to fully benefit of FASTCALL, code should be modified. I'm trying to restrict such changes to Python internals, especially the most used functions.
I expected that the required changes were straightforward enough, it looks like I was wrong, but I don't recall anyone, before you recently, asking for an explanation.
- It was no long ago that you fought tooth-and-nail against a single line patch optimization I submitted. The code was clearly correct and had a simple disassembly to prove its benefit. Your opposition was based on "it increases the complexity of the code, introduces a maintenance cost, and increases the risk of bugs". In the end, your opposition killed the patch. But now, the AC and FASTCALL patches don't seem to mind any of these considerations.
Context: http://bugs.python.org/issue26201
It seems like we need more _explicit_ rules to decide if an optimization is worth it or not. For me, the de facto standard request for an optimization is to prove it with a benchmark. I requested a benchmark, but you refused to provide it.
So I ran my own benchmark and saw that your change made the modified code (PyList_Append()) 6% slower. I'm not sure that my bencmark was correct, but it was a first step to take a decision.
To come back to FASTCALL, your point is that it doesn't provide any speedup.
In most FASTCALL issues that I opened, I provide a script to reproduce my benchmark and the benchmark results. The speedup is usually betwen 10% and 20% faster.
Should I understand that 6% slower is ok, whereas 10-20% faster is not good? Can you please elaborate?
- AC is supposed to be a CPython-only concept. But along the way APIs are being changed without discussion. I don't mind that sorted() now exposes *iterable* as a keyword argument, but it was originally left out on purpose (Tim opined that code would look worse with iterable as a keyword argument). That decision was reversed unilaterally without consulting the author and without a test. Also as AC is being applied, the variable names are being changed. I never really liked the "mp" that used in dicts and prefer the use of "self" better, but it is a gratuitous change that unilaterally reverses the decisions of the authors and makes the code not match any of the surrounding code that uses the prior conventions.
Ah, at least I concur with you on one point :-) Changes to convert functions to AC must not change the API (type of arguments: positional only/keyword/..., default values, etc.) nor provide a worse docstring.
There is an active on-going work to enhance AC to fix issues that you reported, like the default value of positional-only parameters which should not be rendered in the function signature (I created the issue #29299 with a patch). Serhiy is also working on implementing the last major missing feature of AC: support *args and **kwargs parameters (issue #20291).
FYI I wasn't involved in AC changes, I only started to look at AC recently (1 or 2 months ago). Again, I agree that these changes should be carefully reviewed, which is an hard task since required changes are usually large and move a lot of code. We need more eyes to look at these changes!
For the specific case of sorted(), the name of first parameter is already documented in the docstring and documentation in Python 2.7: "iterable". So I guess that you mean that it is now possible to use it as a keyword argument. Well, see the issue #29327 for the long story. This issue is a regression, it was already fixed, and I didn't introduce the API change.
Oh by the way, when I read your comment, I understand that I'm responsible of all regressions. It's true that I introduced regressions, that's where I said "shit happens" (or more politically correct: "it's better to ask forgiveness than permission" ;-)). Since I'm one of the most active contributor in CPython, I'm not surprised of being the one who introduce many (most?) regressions :-) I'm trying to review my changes multiple times, test corner cases, etc. But I'm not perfect.
Sadly, to show its full power, FASTCALL requires changes at many levels of the code. It requires to change at lot of code, but I understood that core developers approved the whole project. Maybe I was wrong? At least, I asked for permissions multiple changes, especially at the start.
- FWIW, the claim that the help is much better is specious. AFAICT, there has never been the slightest problem with "sorted(iterable, key=None, reverse=False) --> new sorted list" which has been clear since the day it was released. It is some of the new strings the are causing problems with users (my students frequently are tripped-up by the / notation for example; no one seems to be able to intuit what it means without it being explained first).
Good news, it seems like you have a good experience in API design, documentation, etc. Join the "Argument Clinic" project to help us to enhance docstrings, function signatures and documentation ;-)
See the good part of the AC on-going work: it's a nice opportunity to also enhance documentation, not only provide a signature.
By the way, to be honest, the main advantage of converting functions to AC is to get a signature. The signature is visible in docstrings which is nice, but it is also very useful to a wide range of tools like (IDE, static checks, etc.).
Conversion to FASTCALL is more a nice effect. At least, it is a good motivation for me to convert mor and more code to AC :-)
AC moves docstring closer to the list of parameters. IHMO it makes the C code simpler to read and understand. It also removes the boring code responsible to "parse" arguments, so it makes the code shorter. But well, this is just my opinion.
- FWIW, I'm trying to be constructive and contribute where I can, but frankly I can't keep up with the volume of churn. Having seen bugs being introduced, it is not inappropriate to ask another dev to please be careful, especially when that dev has been prolific to an unprecedented degree and altering core parts of the language for function calls, to new opcodes, the memory allocators, etc. Very few people on the planet are competent to review these changes, make reasonable assessments about whether the complexity and churn are worth it. An fewer still have the time to keep up with the volume of changes.
Hum, I wasn't involved in bytecode changes.
Well, I reviewed the very good work of Demur Rumed. I recall that you worked on a similar area, trying to fetch bytecode by 16-bit instead of 8-bit. Demur proposed a good design and I recall that the design was approved.
I helped a little bit on the implementation and I pushed the final change, but all credits go to Demur and Serhiy Storshaka! By the way, Serhiy made further efficient enhancements in the bytecode of CALL_FUNCTION instructions.
About memory allocations, I guess that you are referring to my change on PyMem_Malloc() allocator. I discussed the issue on python-dev and waited for approval of my peers before pushing anything, since I know well that it's a critical part of Python: https://mail.python.org/pipermail/python-dev/2016-March/143467.html
I provide all data requested by Marc Andre Lemburg (test the change with common projects, Django, Pillow, numpy) and made further changes (PYTHONMALLOC=debug tool) to help to handle this backward incompatible change (GIL is now required to call PyMem_Malloc).
Hopefully, it seems like nobody noticed this subtle change (GIL now requied): I didn't see any bug report. By the way, I fixed a misused PyMem_Mem() in numpy.
- Please do continue your efforts to improve the language, but also please moderate the rate of change, mitigate the addition complexity, value stability over micro-optimizations, consult the authors and maintainers of code, take special care without code that hasn't been reviewed because that lacks a safety net, and remember that newer devs may be taking cues from you (do you want them making extensive changes to long existing stable code without consulting the authors and with weak LGTM reviews?)
Ok, I will do it.
Thank you for you feedback Raymond. I hope that my email helps you to understand how I work and how I take my decisions.
Victor
Hi, Victor and all.
I'm sorry, I haven't read the whole mail yet. Reading long English is tough job for me.
I notice this mail is very important for me. I'll take time to read this thread. But it may take long time. I want to post some comments before.
I learned many from reviewing Victor's patches. For example, I learned how to add type slot without breaking ABI compatibility in this week.
I learned some surprising corner cases from regressions caused by my patches, or patches I said LGTM too.
So I thanks to Victor, and everyone who reviewed my patch, who run their test with development branch of Python, and Travis-CI (it ease running tests with "3.7-dev" branch.)
About pace of changes, I'll reduce my pace to write patches, because I joined one project in my company. But I'll keep time to watch issue tracker and review patches, though.
Regards,
Hi Victor, hi Raymond,
Le 20/01/2017 à 11:45, Victor Stinner a écrit :
Inaccurate summary: I'm a strong supporter of "it's better to ask forgiveness than permission", whereas Raymond considers that I introduced too many regressions with my workflow.
It's a matter of balance. "Easier to ask forgiveness than permission" is fine for small-scale changes with little ramification or impact on other parts. It certainly wouldn't be appropriate for PEP-sized changes.
It's true that there has been a lot of churn lately, which is a bit difficult to follow. OTOH, I don't follow much anyway, because I'm unmotivated and don't care enough except for very few areas.
Since one or maybe even two years, I noticed that many of my issues were blocked by the lack of reviews. As you wrote, only few developer have the knowledge and background to be able to provide a good review (not only "tests pass, so LGTM") on my changes modifying the Python core.
Yes, this is still an important issue. There are topics on which there is almost never anybody available to do a review, so sometimes you may have to move forward and cross fingers that you're not breaking anything.
It's strange, but the process of opening an issue and attaching the patch usually helps to review the code myself (find bugs, or more generally enhance the patch).
Ha, that's exactly the same thing here. Reading my patch in a Web browser makes me likely to spot problems that I wouldn't have found otherwise.
By the way, it's painful to squash a long patch serie into a giant patch, much harder to review, where changes don't make sense at all at the first look. Again, a better reviewing tool supporting patch series (GitHub) will help here too.
I've never done patch series. They look very painful to maintain, because tooling for them is very primitive (whether with hg or git, AFAIK).
Oh by the way, when I read your comment, I understand that I'm responsible of all regressions. It's true that I introduced regressions, that's where I said "shit happens" (or more politically correct: "it's better to ask forgiveness than permission" ;-)).
If what was lacking was more reviewing of patches properly posted to the tracker, then you're not the only person responsible, we're all collectively responsible. Chastising one of the few persons who puts in a lot of work is unfair.
Regards
Antoine.
On Fri, Jan 20, 2017 at 11:45:29AM +0100, Victor Stinner wrote:
The truth is that too few of my patches get a review :-/ Maybe I should wait longer, but then it becomes harder for me to handle many patches.
The problem here is partly that maintainers don't think that all of the changes are worth spending time on.
I'm not specifically talking about *your* changes here, but Raymond's comment also referred to other committers.
In the code that I maintain (and wrote), about 4/5th of the kind of changes that Raymond complained about were just unnecessary. Some were stylistic preferences, others came with a "potential bug" reasoning that turned out to be wrong.
To be clear, I'm NOT talking about changes like this one:
https://hg.python.org/cpython/rev/7f4c9cfae20c
Those have a clear purpose and don't disturb.
FASTCALL has a purpose, but only if a function is actually a hotspot. For the decimal changes that I reverted this wasn't the case, so it is perhaps "just nice to have for 3.8".
I'm still very interested of course to have an official FASTCALL API for third party modules that don't (and probably should not) use AC.
- Historically, if there was creator or maintainer of the code who was still active, that person would always be consulted and have a final say on whether a change should be applied. Now, we have code constantly being changed without consulting the original author (for example, the recent and catastrophic random initialization bug was due to application of a patch without consulting the author of _randommodule.c and the maintainer of random.py, or this change to sorted(), or the changes to decimal, etc).
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
Hmm, no: for example _collectionsmodule.c is written and maintained by Raymond; Modules/_decimal/* has exactly one author.
My problem is that I'm not aware of any explicit list of maintainers.
https://docs.python.org/devguide/experts.html :)
Stefan Krah
Thanks for this email Victor, it illustrates a lot of pain-points that some core devs have with CPython development process.
I think that we are lucky that we have you and Serhiy who spend so much time to push so many improvements to the CPython internals. I think that while we are working on a new major version of CPython (3.7 now), it's acceptable to push performance optimizations without a lengthy discussion on python-dev and a thorough review by 3+ core developers. An issue on the bug tracker explaining the change and showing some benchmarks should be enough.
Those who want to see the results of Serhiy's and Victor's work can look at https://speed.python.org/comparison/ and see for themselves that 3.7 is already faster than 3.6 and 2.7 in most cases.
To reflect on my own experience: I had a patch to speed up LOAD_GLOBAL, LOAD_ATTR and LOAD_METHOD early in 3.6 development cycle. The patch promised to improve performance 5-20% on some benchmaks. I sent a few emails to python-dev explaining the change, explaining the memory usage changes etc. What I saw is that only one or two people were interested in the change, and almost nobody wanted to actually review the change. I became less motivated, and in the end I decided to focus on other areas and postpone my work on that optimization until later. And later I regretted that: I should have pushed the change, and we would have few months to improve and test it, and we would have an even faster 3.6. (I'll continue my work on the patch soon).
I think that we need to become less conservative about development of CPython internals. At this point it's impossible to make CPython any faster without invasive refactorings, and I think it's OK to trust our core developers to make them.
Any public API changes/enhancements should be discussed on python-dev and thoroughly reviewed though. API design is something that a single person usually cannot do well.
Thank you, Yury
On Jan 20, 2017, at 9:26 AM, Yury Selivanov yselivanov.ml@gmail.com wrote:
I think that we need to become less conservative about development of CPython internals. At this point it's impossible to make CPython any faster without invasive refactorings, and I think it's OK to trust our core developers to make them.
I agree with the sentiment but I'm worried about the implications. Your suggested approach only makes sense if we have a good safety net in place to catch regressions early. Unfortunately, apart from regrtest, buildbots, and dumb luck, we don't. Betas have very limited adoption, too limited to be useful in my experience. I tried and failed to introduce 3.6 during beta phase at work because there was too much risk it would be picked up by unsuspecting users. Other users seem to be in the same camp. Consequently, most x.y.0 releases are relatively lower quality.
I wonder if there's any way for us to incentivize beta usage in some key areas. For example, if every Python 3 project on Travis CI also ran on "3.7-dev" or "nightly", that would already give us better real world coverage. Actually *running* dev builds in production seems too risky for me for the reasons that Raymond and Victor state in this thread: too much lightly reviewed changes.
Summing up, I don't think we can really start "moving fast and breaking things" until we have wider adoption of dev builds in the wild. To put money where my mouth is, I am currently working on introducing the dev version of 3.7 to the build system at work. But this is not going to be enough on its own.
- Ł
On 2017-01-20 5:02 PM, Lukasz Langa wrote:
On Jan 20, 2017, at 9:26 AM, Yury Selivanov yselivanov.ml@gmail.com wrote:
I think that we need to become less conservative about development of CPython internals. At this point it's impossible to make CPython any faster without invasive refactorings, and I think it's OK to trust our core developers to make them. I agree with the sentiment but I'm worried about the implications. Your suggested approach only makes sense if we have a good safety net in place to catch regressions early. Unfortunately, apart from regrtest, buildbots, and dumb luck, we don't. Betas have very limited adoption, too limited to be useful in my experience. I tried and failed to introduce 3.6 during beta phase at work because there was too much risk it would be picked up by unsuspecting users. Other users seem to be in the same camp. Consequently, most x.y.0 releases are relatively lower quality.
I wonder if there's any way for us to incentivize beta usage in some key areas. For example, if every Python 3 project on Travis CI also ran on "3.7-dev" or "nightly", that would already give us better real world coverage. Actually *running* dev builds in production seems too risky for me for the reasons that Raymond and Victor state in this thread: too much lightly reviewed changes.
Summing up, I don't think we can really start "moving fast and breaking things" until we have wider adoption of dev builds in the wild. To put money where my mouth is, I am currently working on introducing the dev version of 3.7 to the build system at work. But this is not going to be enough on its own.
- Ł
Lukasz, I understand what you are saying here. But on the other hand, we should not stop improving CPython. Sometimes stuff will break because of our "loose" safety net, but this is inevitable. It happens virtually in any software project (including other languages and compilers).
My experience shows that even extensive code reviews don't spot all bugs. E.g. I missed a bug in one asyncio patch that caused "loop.sock_connect" to be broken in 3.5.2. Another one: due to a design mistake in PEP 492, we had to change how __aiter__ behaves in 3.5.3 and 3.6. PEP 492 was thoroughly discussed, the patches were reviewed, and we merged everything a few months before we released Python 3.5.0; that wasn't enough apparently.
I'm not suggesting that we should "move fast", merge things without reviews / tests / benchmarks. In Victor's case, he started working on FASTCALL in 3.6, and we already shipped some of his improvements (I reviewed a few FASTCALL patches myself). Now he (with help of Serhiy and Inada-san) simply continues to work on the project, gradually refactoring the code, improving Argument Clinic etc. We are still early in 3.7 development cycle, and I don't think we should stop the project just because we had a couple of minor regressions.
Thank you, Yury
On 21 January 2017 at 09:02, Lukasz Langa lukasz@langa.pl wrote:
On Jan 20, 2017, at 9:26 AM, Yury Selivanov yselivanov.ml@gmail.com wrote: I think that we need to become less conservative about development of CPython internals. At this point it's impossible to make CPython any faster without invasive refactorings, and I think it's OK to trust our core developers to make them.
I agree with the sentiment but I'm worried about the implications. Your suggested approach only makes sense if we have a good safety net in place to catch regressions early. Unfortunately, apart from regrtest, buildbots, and dumb luck, we don't. Betas have very limited adoption, too limited to be useful in my experience. I tried and failed to introduce 3.6 during beta phase at work because there was too much risk it would be picked up by unsuspecting users. Other users seem to be in the same camp. Consequently, most x.y.0 releases are relatively lower quality.
The Red Hat python-maint folks (Peter Viktorin et al) similarly tried to time things so there was a window to provide feedback from Fedora Rawhide on a beta release prior to the final 3.6.0 release, but it unfortunately ended up not being practical, especially with the lack of clarity around when the 3.6 ABI would actually be frozen.
That uncertainty around the release ABI stability also impacted the ability of the manylinux folks to include any of the 3.6 pre-releases in their reference build environment.
To further complicate matters, the user visible syntax and semantics of various subsystems have also been changing at a rate not really seen within a single release series since the 2.3/4/5 era more than a decade ago, where we saw extended slicing support in the builtin types, decorators, with statements, and more.
I think both of these shifts (faster moving internals, increased rate of change in public interfaces) is a symptom of the fact that while we don't have anyone being paid specifically to focus on stability and general maintenance, we *do* have more active folks with paid or personal time to spend on particular problems related to dealing with large code bases and software deployments (reducing CPU usage, memory management, asynchronous flow control, improving static analysis support, security), improving integration with particular deployment platforms (mainly Windows and Linux, but also mobile devices), and iterating on low level areas of personal interest (e.g. the import system, metaclasses, timezones).
In that environment, we need to be careful to remember that we're collectively stewards of the overall language ecosystem in addition to the reference implementation.
That means it isn't just CPython we need to worry about - yes, it's beneficial to the ecosystem as a whole to have significant low level improvements being made to the reference interpreter (since it's still far and away the most popular implementation), but we're the *most* well-resourced of the Python interpreter implementations, and even we have a grand total of zero people paid to work full-time on monitoring and facilitating the core development process (our commercial redistributors appear to be OK with that state of affairs, which implies that their customers are either also fine with it, or else aren't looking too closely at what they're getting for their money).
Other implementations are often still attempting to catch up to where CPython was a year or more ago, before they can even start thinking about where we're going to be a year or more from now.
So if we're hitting review bottlenecks for low level changes, then "I'll commit the change anyway" probably isn't the best path for us to be taking - it's likely to be more beneficial in the long run to take a step back and ask *why* there isn't anyone else with the time and inclination to review the change, and whether or not there might be a way for us to help address the underlying sustainability problems (or advocate internally for our respective employers to start doing so in a more systematic way).
Cheers, Nick.
P.S. The simplest illustration I know of for those underlying sustainability problems is the "up and to the right" nature of the graph of open issues: http://bugs.python.org/issue?@template=stats
The move to GitHub and the introduction of automated pre-merge CI testing will hopefully help streamline the patch review process for simple fixes, but it's far from clear at this point whether or not that will be enough to get the issue metrics moving in a healthier direction.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Le 21/01/2017 à 12:27, Nick Coghlan a écrit :
The Red Hat python-maint folks (Peter Viktorin et al) similarly tried to time things so there was a window to provide feedback from Fedora Rawhide on a beta release prior to the final 3.6.0 release, but it unfortunately ended up not being practical, especially with the lack of clarity around when the 3.6 ABI would actually be frozen.
That uncertainty around the release ABI stability also impacted the ability of the manylinux folks to include any of the 3.6 pre-releases in their reference build environment.
Normally, our "only bug fixes after the 1st beta" should more or less guarantee ABI stability, but in practice that policy is very frequently broken by well-meaning core developers.
Worse, my experience is that trying to remind people of that policy is frequently met with hostility. For example, "performance improvements" are frequently sneaked in after the first beta under the pretext that they don't harm. There will be little progress on this front until there's a serious consensus amongst all active players that the bugfix period after the first beta should really only allow for *bug fixes* (i.e. some behaviour which doesn't conform to the documentation and/or the user's reasonable expectation).
Regards
Antoine.
On 21 January 2017 at 23:32, Antoine Pitrou antoine@python.org wrote:
Le 21/01/2017 à 12:27, Nick Coghlan a écrit :
The Red Hat python-maint folks (Peter Viktorin et al) similarly tried to time things so there was a window to provide feedback from Fedora Rawhide on a beta release prior to the final 3.6.0 release, but it unfortunately ended up not being practical, especially with the lack of clarity around when the 3.6 ABI would actually be frozen.
That uncertainty around the release ABI stability also impacted the ability of the manylinux folks to include any of the 3.6 pre-releases in their reference build environment.
Normally, our "only bug fixes after the 1st beta" should more or less guarantee ABI stability, but in practice that policy is very frequently broken by well-meaning core developers.
In the case of the 3.6 beta cycle, there were genuinely some ABI fixes needed after 3.6b1, so I don't think 3.x.0b1 is the right time to declare "we expect all binary extension modules built with this version to work with 3.x.0".
It does seem reasonable to lock it down around beta 3, though - at that point we've had time for people to do some test builds against a stable API, as well as an initial iteration of fixes for identified regressions.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
As more of an ex-Python committer, I have little to say about current processes. I will make one observation:
On Jan 20, 2017, at 11:45 AM, Victor Stinner victor.stinner@gmail.com wrote:
I introduced a regression in random.Random.seed(): ...
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
I do not like this comment. I feel like it has narrow understanding of who uses Python, of who who depends on this API, and the history of how important it is to have a good RNG seed even for non-security.
Scientific programming uses random numbers. A quick grep of the source code on my machine shows the following example from Biopython's Bio/GA/Mutation/Simple.py (comments and docstrings removed for clarity):
class SinglePositionMutation(object): def __init__(self, mutation_rate=0.001): self._mutation_rate = mutation_rate self._mutation_rand = random.Random() self._switch_rand = random.Random() self._pos_rand = random.Random()
If I understand the bug correctly, under Python 3.6 under Windows these will likely have the same seed. This may (or may not) affect downstream statistics. This may (or may not) require a publication to be retracted. This may (or may not) be a disaster to the researcher who now has to re-do all of the calculations and see if there is a problem.
It's certainly hard to track down all the people who might be affected and beg for their forgiveness, which is what it sounds like you will be doing if you really believe your development philosophy, and aren't using it as an excuse to skimp on testing and coordination.
Back in the 1990s there were a number of papers showing the importance of having a good RNG for physical simulations, even if it isn't cryptographically strong, and the importance of having no correlations between multiple RNGs. Basically, most programmers aren't good enough to identify bad default RNGs and seeds. The general solution put a good-enough default solution in the underlying language implementations. Which all modern languages, including Python, have done. Well, except now.
I do not think it's relevant to comment that only a few developers call Random() directly. The guideline should be the number of people who might be affected. There are many more Biopython users than developers, making the actual severity many times larger than that developer comment implies.
Finally, there's a long history of small, seemingly orthogonal changes in RNG code making large reductions in entropy, such as https://en.wikinews.org/wiki/Predictable_random_number_generator_discovered_... caused by following suggestions made by Valgrind and Purify; the sort that might be called "trivial (short, non controversal)". Anyone touching RNG code has to be aware that it's a sensitive area, and pay extra attention to testing.
In the bug report at http://bugs.python.org/issue29085 you write "Too bad that there is no simple way to write an unit test for that."
Here's a possible test based on the reproducible in the report:
Random = random.Random
for i in range(1000):
rng1 = Random()
rng2 = Random()
assert rng1.getrandbits(128) != rng2.getrandbits(128), "RNG initialization failed"(I do not have a Windows box to test this. It passes 500,000 tests on my Mac.)
If the two Random() calls are in the same clock quantum and so get the same seed then they will have the same bit pattern. The odds of two independent seeds giving the same 128-bit pattern is incredibly low.
For this one bug, I agree with the interpretation that it was handled with a cavalier attitude. I don't feel like it's being treated with the seriousness it should. As far as I can tell there isn't even a regression to prevent something like this from happening again, only a handwaving that such a test is not simple (and why does a unit test for an important requirement have to be simple in order to be added?).
I've forwarded this Python 3.6 bug to the Biopython developers, at https://github.com/biopython/biopython/issues/1044 . I don't think it's a serious problem, but don't know enough to be certain. In any case, the likely easy fix for them is to reuse the same RNG instead of creating independent ones. However, I expect there are other projects which will be more affected by this bug.
Cheers,
Andrew
dalke@dalkescientific.com2017-01-21 0:14 GMT+01:00 Andrew Dalke dalke@dalkescientific.com:
For this one bug, I agree with the interpretation that it was handled with a cavalier attitude. I don't feel like it's being treated with the seriousness it should.
The regression was introduced by http://bugs.python.org/issue27776 which was reviewed by Nick Coghlan. The discussion is about the lack of reviews. The patch was reviewed, but the review didn't spot the bug.
The bug was reported at 2016-12-27, it was fixed and closed 2 days later by Benjamin Peterson: http://bugs.python.org/issue29085
The issue was fixed so fast that I didn't even noticed it. I only a few days later that I get notified indirectly, I don't recall how.
Note: Benjamin didn't attach a patch to the issue nor waited for a review.
What did we wrong for this specific issue, and how can we prevent similar failure next time?
I don't understand why you are saying that I (I or we?) didn't handle the issue seriously. And can you please also elaborate why you consider that my attitude was cavalier on this issue?
If you consider that the bug is serious enough, maybe we should schedule a quick 3.6.1 bugfix release?
About testing, I wrote a minimum test for os.urandom() checking that calling os.urandom(16) twice produces two different random sequences. Lib/test/test_os.py:
def test_urandom_value(self):
data1 = os.urandom(16)
self.assertIsInstance(data1, bytes)
data2 = os.urandom(16)
self.assertNotEqual(data1, data2)Note: the test was added by Georg Brandl in the issue #13703, but I wrote an initial patch adding the test. So it's not like I don't care of the quality of Python RNG ;-)
Would such minimum test be enough to detect the weak RNG seed bug?
I know well RNG issues. I spent a lot of time writing my own RNG library and studying RNG bugs in various libraries and languages. Bugs are common, it's hard to write tests to detect non trivial bugs on RNG. The question is how we can enhance Python on this point.
Victor
On 01/20/2017 04:03 PM, Victor Stinner wrote:
2017-01-21 0:14 GMT+01:00 Andrew Dalke dalke@dalkescientific.com:
For this one bug, I agree with the interpretation that it was handled with a cavalier attitude. I don't feel like it's being treated with the seriousness it should.
I don't understand why you are saying that I (I or we?) didn't handle the issue seriously. And can you please also elaborate why you consider that my attitude was cavalier on this issue?
Victor, an excerpt from your original email:
I introduced a regression in random.Random.seed(): a typo in the C code has the consequence that the current time and process identifier is used, instead of os.urandom(16), to initialize the Mersenne Twister RNG.
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
Going by the last paragraph it doesn't seem that the bug is a big deal to you, that you believe very few people even use random.Random and so very few people will get bitten by it. Andrew has given an example where it can be a very big deal (retracted scientific papers, loss of hundreds of hours of work, etc.), and can definitely fall in the catastrophic category.
-- ~Ethan~
On 20 January 2017 at 21:45, Victor Stinner victor.stinner@gmail.com wrote:
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
My problem is that I'm not aware of any explicit list of maintainers. I didn't know that you were the maintainer of the random module before you told me that at the Facebook sprint last september. I didn't expect that the random module had a maintainer, I thought that any core developer would be allowed to modify the code.
Specific areas of interest and expertise are listed in the Developer's Guide: https://docs.python.org/devguide/experts.html#experts
Typing any of the headings in that table into the Nosy list on bugs.python.org will auto-populate a dropdown with the relevant table entry.
If I'd thought of it at the time, I would have asked Raymond and Mark to double-check we hadn't broken anything in the random module initialisation with the os.urandom() changes for 3.6, but it didn't occur to me to do so since my main focus was on string hashing, the secrets module, and direct use of the os.urandom() and os.getrandom() APIs.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
What I'm picking up from this is (as a gross oversimplification):
- Victor _wants_ code reviews
- Raymond thinks we _need_ code reviews
So the common theme here regardless of whether you agree with Raymond or Victor's approach to development is that we are not getting enough code reviews to go around. To me that's what the systemic issue is that this email is bringing up.
Now I think most of us don't think the solution to the lack of reviews is to lower our standard of what it takes to become a core developer (this doesn't mean we shouldn't do a better job of identifying potential candidates, just that we shouldn't give people commit privileges after a single patch like some projects do). To me that means we need to address why out of 79 core developers only 39 have a single commit over the past year, 30/79 have more than 12 commits over that same time scale, 15/79 people have more than 52 commits, and 2/79 people have over 365 commits ( https://github.com/python/cpython/graphs/contributors?from=2016-01-22&to=2017-01-21&type=c for the stats).
Some of you have said you're waiting for the GitHub migration before you start contributing again, which I can understand (I'm going to be sending an email with an update on that after this email to python-dev & core-workflow). But for those that have not told me that I don't know what it will take to get you involved again. For instance, not to pick on Andrew but he hasn't committed anything but he obviously still cares about the project. So what would it take to get Andrew to help review patches again so that the next time something involving random comes through he feels like taking a quick look?
As I have said before, the reason I took on the GitHub migration is for us core developers. I want our workflow to be as easy as possible so that we can be as productive as possible. But the unspoken goal I have long-term is to get to the point that even dormant core devs want to contribute again, and to the point that everyone reviews a patch/month and more people reviewing a patch/week (although I'll take a patch/year to start). I want to get to the point that every person with commit privileges takes 30 minutes a month to help with reviews and that the majority of folks take 30 minutes a week to review (and please don't think this as a hard rule and if you don't the privileges go away, view this as an aspirational goal). Even if people who don't have time to review the kind of patches Victor is producing which triggered this thread, reviewing documentation patches can be done without deep knowledge of things and without taking much time. That way people who have time to review the bigger, more difficult patches can actually spend their time on those reviews and not worrying about patches fixing a spelling mistake or adding a new test to raise test coverage.
All of this is so that I hope one day we get to the point where all patches require a review no matter who proposed the code change. Now I think we're quite a ways of from being there, but that's my moonshot goal for our workflow: that we have enough quality reviews coming in that we feel that even patches from fellow core developers is worth requiring the extra code check and disbursement of knowledge without feeling like a terrible drag on productivity.
Once the GitHub migration has occurred I'm planning to tackle our Misc/NEWS problem and then automate Misc/ACKS. After that, though, I hope we can take the time to have a hard look at what in our workflow prevents people from making even occasional code reviews so that everyone wants to help out again (and if any of this interests you then please subscribe to core-workflow).
On Fri, 20 Jan 2017 at 02:46 Victor Stinner victor.stinner@gmail.com wrote:
Hi,
Raymond Hettinger used a regression that I introduced in the builtin sorted() function (in Python 3.6.0) to give me his feedback on my FASTCALL work, but also on Argument Clinic.
Context: http://bugs.python.org/issue29327#msg285848
Since the reported issues is wider than just FASTCALL, including how I contribute to CPython, I decided to discuss the topic with a wider audience. I continue the discussion on python-committers to get the opinion of the other core developers.
Sorry for my very long answer! I tried to answer to each issues reported by Raymond.
Inaccurate summary: I'm a strong supporter of "it's better to ask forgiveness than permission", whereas Raymond considers that I introduced too many regressions with my workflow.
Raymond Hettinger added the comment:
A few random thoughts that may or may not be helpful:
- We now have two seasoned developers and one new core developer that
collectively are creating many non-trivial patches to core parts of Python at an unprecedented rate of change. The patches are coming in much faster than they can reasonably be reviewed and carefully considered, especially by devs such as myself who have very limited time available. IMO, taken as whole, these changes are destabilizing the language. Python is so successful and widely adopted that we can't afford a "shit happens" attitude. Perhaps that works in corners of the language, infrequently used modules, but it makes less sense when touching the critical paths that have had slow and careful evolution over 26 years. > > * Besides the volume of patches, one other reason that reviews are hard to come by is that they apply new APIs that I don't fully understand yet. There are perhaps two people on the planet who could currently give thoughtful, correct, and critical evaluation of all those patches. Everyone else is just watching them all fly by and hoping that something good is happening.
Since one or maybe even two years, I noticed that many of my issues were blocked by the lack of reviews. As you wrote, only few developer have the knowledge and background to be able to provide a good review (not only "tests pass, so LGTM") on my changes modifying the Python core.
I also wanted to discuss this topic, but I didn't really know what to propose. Let's take this opportunity to explain how I contribute to CPython, especially how I decide to wait for a review or not.
For each patch that I write, I estimate the risk of regression. You may know that any regression is something unexpected, so such estimation is tricky. Here is my heuristic:
(*) if the patch is trivial (short, non controversal), I push it immediatly.
(*) If I'm less confident, I open an issue and attach the patch. I wait at least one day before pushing.
It's strange, but the process of opening an issue and attaching the patch usually helps to review the code myself (find bugs, or more generally enhance the patch). Maybe because it forces me to review the change one more time?
If the change is not part of a larger patch serie, so doesn't block me to move further, I try to keep the issue open around one week.
The truth is that too few of my patches get a review :-/ Maybe I should wait longer, but then it becomes harder for me to handle many patches.
Maybe it's a tooling issues. Recently, I started to use local branches in a Git repository. It helps a lot of work on parallel on large changes. Before, I only worked in a single directory (default/, the default Mercurial branch) and applied/reverted patches everytime. It's painful, especially when I have to use multiple computers, download again publshed patches, etc. Maybe it will run smoother once CPython will move to Git and GitHub.
By the way, it's painful to squash a long patch serie into a giant patch, much harder to review, where changes don't make sense at all at the first look. Again, a better reviewing tool supporting patch series (GitHub) will help here too.
Not supporting patch series in our reviewing tool also explains why I prefer to push than having to wait for a review. Rebasing manually long patch series stored as giant .patch files is complicated.
(*) If the change changes an API or changes a core component, I wait for at least one review from a core reviewer. Sometimes, I even send an email to python-dev. Again, sometimes I don't get any feedback on the patch nor the email after two weeks :-/ At least, I tried :-) Usually, I get feedback in less than one week, or no feedback at all. I understand that nobody understands my change or nobody cares :-)
I totally understand that most core developers have a little amount of time available to contribute to Python. I'm trying to find a compromise between the risk of introducing regressions and being stuck in my work. This email might help me to adjust my workflow.
By the way, I'm trying to always run the full test suite (./python -m test -rW -j0) before pushing any change. If I suspect that I may have introduced reference leaks, I also run "./python -m test -R 3:3 ..." on the tests related to the modified code to check for memory/reference leaks.
- One other reason for the lack of review comments in the enthusiasm and fervor surrounding the patches. I feel like there is a cost of questioning whether the patches should be done or how they are done, like I am burning little karma every time. Sometimes it feels safest and most cordial to just say nothing and let you make hundreds of semi-reviewed changes to just about every critical part of the language.
"semi-reviewed". Let me be more accurate: yeah, I do push a lot of changes which were not reviewed by anyone (see above).
- Historically, if there was creator or maintainer of the code who was still active, that person would always be consulted and have a final say on whether a change should be applied. Now, we have code constantly being changed without consulting the original author (for example, the recent and catastrophic random initialization bug was due to application of a patch without consulting the author of _randommodule.c and the maintainer of random.py, or this change to sorted(), or the changes to decimal, etc).
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
My problem is that I'm not aware of any explicit list of maintainers. I didn't know that you were the maintainer of the random module before you told me that at the Facebook sprint last september. I didn't expect that the random module had a maintainer, I thought that any core developer would be allowed to modify the code.
Moreover, since I open an issue for most of my changes, it gives an opportunity to maintainers to review changes. Maybe we need more components in the bug tracker to notify maintainers of pending changes?
You mentionned 3 different changes, let me reply.
(1) The random change: http://bugs.python.org/issue29085
I introduced a regression in random.Random.seed(): a typo in the C code has the consequence that the current time and process identifier is used, instead of os.urandom(16), to initialize the Mersenne Twister RNG.
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
Since we are talking about the development process, let me see how the change was made.
Context: The PEP 524 has a long and painful history... Something like more than 500 messages were sent on the bug tracker and python-dev, and nobody was listening to each others, two security experts "rage-quitted" Python because of this mess... I decided to try to fix this issue in a constructive way, so I wrote a PEP. Nick wrote a different PEP, since it was clear that it was possible to handle security in two different incompatible ways. A mailing list was even created just to discuss this bug! A mailing list just for a bug gives an idea of the size of the mess :-)
Well, about the change itself, it was done in http://bugs.python.org/issue27776
The patch was available for review during 19 days (2016-08-18-2016-09-06) and was reviewed by Nick Coghlan. Since Nick wrote a similar PEP, I trusted him to be able to review my change. (Well, anyway I already trust all core developers, but I mean that I was trusting him even more than usual :-))
Since the change has a big impact on security, I had prefer to get a review of more developers, especially our security experts... but as I wrote, two security experts "rage- quitted". Again, this PEP has a long and sad story :-/
Note: you say that you are the maintainer of the random module, but I don't recall having see you in any recent discussions and issues related to os.urandom(), whereas a lot of enhancements and changes were done last 2 years. I made many changes to support new OS functions like getentropy() an getrandom().
Oooookay, let's see the second change, "this change to sorted()", http://bugs.python.org/issue29327
(2) I introduced a bug in sorted(), last August: https://hg.python.org/cpython/rev/15eab21bf934/
Calling sorted(iterable=[]) does crash. To be honest, I didn't imagine that anyone would pass the iterable by keyword, but Serhiy is very good to spot bugs in corner cases :-)
IMHO the regression is subtle.
When I optimized the code to use FASTCALL, I replaced PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1). I checked that all tests passed, so it looks ok to me.
I didn't imagine that anyone would call sorted(iterable=[]), so I didn't notice that PyTuple_GetSlice() can create an empty tuple.
The previous code was wrong since sorted() accepted iterable as a keyword, whereas sort.list() doesn't.
So well, I let you guess if a review would have spot this bug in the large change.
(3) Recently, I ran sed to replace code patterns to use faster ways to call functions: https://hg.python.org/cpython/rev/54a89144ee1d
"Replace PyObject_CallObject(callable, NULL) with _PyObject_CallNoArg(callable)"
I recalled that I modified the _decimal module and that Stefan Krah complained, because he wants to have the same code base on Python 3.5, 3.6 and 3.7. He also mentionned an external test suite which was broken by recent _decimal changes (not sure if my specific change was in cause or not), but I wasn't aware of it.
To be honest, I didn't even notice that I modified _decimal when I ran sed on all .c files. Since the change was straightforward and (IMHO) made the code more readable, I didn't even wait for a review if I recall correctly.
Stefan and me handled this issue privately (he reverted my change), I'm not sure that it's worth it to say more about this "issue" (or even "non-issue").
To be clear, I don't consider that my change introduced a regression.
In general, Guido has been opposed to sweeping changes across the code base for only tiny benefits. Of late, that rule seems to have been lost.
The benefits of FASTCALL mainly apply to fine grained functions which only do a little work and tend to be called frequently in loops. For functions such as sorted(), the calling overhead is dominated by the cost of actually doing the sort. For sorted(), FASTCALL is truly irrelevant and likely wasn't worth the complexity, or the actual bug, or any of the time we've now put in it. There was no actual problem being solved, just a desire to broadly apply new optimizations.
Ok, first, you qualify my FASTCALL changes as code churn. So let me show an example with sorted(): https://hg.python.org/cpython/rev/b34d2ef5c412
Can you elaborate how such change increases the complexity?
Second, "no actual problem being solved"
Since the goal of FASTCALL is to optimize Python, I guess that you consider that the speedup doesn't justify the change. I gave numbers in the issue #29327:
Microbenchmark on sorted() on Python 3.7 compared to 3.5 (before FASTCALL):
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10))' 'sorted(seq)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 1.07 us +- 0.06 us -> [3.7] 958 ns +- 15 ns: 1.12x faster (-11%)
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10)); k=lambda x:x' 'sorted(seq, key=k)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 3.34 us +- 0.07 us -> [3.7] 2.66 us +- 0.05 us: 1.26x faster (-21%)
IMHO such speedup is significant even on a microbenchmark. Can you elaborate what are your criteria to decide if an optimization is worth it?
- Historically, we've relied on core developers showing restraint. Not every idea that pops into their head is immediately turned into a patch accompanied by pressure to apply it. Devs tended to restrict themselves to parts of the code they knew best through long and careful study rather sweeping through modules and altering other people's carefully crafted code.
Should I understand that I should restrict myself to some files? Or not touch some specific parts of Python, like... "your" code like random, itertools and collections modules?
I replied to the 3 issues you mentioned previously and explained how I contribute to Python.
FWIW, I applaud your efforts to reduce call overhead -- that has long been a sore spot for the language.
Guido has long opposed optimizations that increase risk of bugs, introduce complexity, or that affect long-term maintainability. In some places, it looks like FASTCALL is increasing the complexity (replacing something simple and well-understood with a wordier, more intricate API that I don't yet fully understand and will affect my ability to maintain the surrounding code).
I'm sorry, I didn't spent much time on explaing the FASTCALL design nor documenting my changes. It's partially deliberate to make everything related to FASTCALL private. Since it's a huge project modifying a lot of code, I wanted to wait until the APIs and the code stop moving too fast to take time to explain my work and document it.
If you have specific questions, please go ahead.
Shortest summary:
FASTCALL replaces (args: tuple, kwargs: optional dict) with (args: C array, nargs: int, kwnames: tuple of keyword keys). It's a new calling convention which allows to avoid a temporary tuple to pass positional arguments and avoids temporary dictionary to pass keyworkd arguments.
To use FASTCALL, C functions should be converted to the new METH_FASTCALL calling convention
PyObject_Call() can replaced with _PyObject_FastCallKeywords() or _PyObject_FastCallDict() (when we still get kwargs as a dict) in such conversion
Many existing C functions were optimized internally to use FASCALL, so even if you don't modify your code, you will benefit of it (speedup). Typical example: PyFunction_CallFunctionObjArgs().
The most massive change were purely internal and don't affect the most famous C APIs at all. In some cases, to fully benefit of FASTCALL, code should be modified. I'm trying to restrict such changes to Python internals, especially the most used functions.
I expected that the required changes were straightforward enough, it looks like I was wrong, but I don't recall anyone, before you recently, asking for an explanation.
- It was no long ago that you fought tooth-and-nail against a single line patch optimization I submitted. The code was clearly correct and had a simple disassembly to prove its benefit. Your opposition was based on "it increases the complexity of the code, introduces a maintenance cost, and increases the risk of bugs". In the end, your opposition killed the patch. But now, the AC and FASTCALL patches don't seem to mind any of these considerations.
Context: http://bugs.python.org/issue26201
It seems like we need more _explicit_ rules to decide if an optimization is worth it or not. For me, the de facto standard request for an optimization is to prove it with a benchmark. I requested a benchmark, but you refused to provide it.
So I ran my own benchmark and saw that your change made the modified code (PyList_Append()) 6% slower. I'm not sure that my bencmark was correct, but it was a first step to take a decision.
To come back to FASTCALL, your point is that it doesn't provide any speedup.
In most FASTCALL issues that I opened, I provide a script to reproduce my benchmark and the benchmark results. The speedup is usually betwen 10% and 20% faster.
Should I understand that 6% slower is ok, whereas 10-20% faster is not good? Can you please elaborate?
- AC is supposed to be a CPython-only concept. But along the way APIs are being changed without discussion. I don't mind that sorted() now exposes *iterable* as a keyword argument, but it was originally left out on purpose (Tim opined that code would look worse with iterable as a keyword argument). That decision was reversed unilaterally without consulting the author and without a test. Also as AC is being applied, the variable names are being changed. I never really liked the "mp" that used in dicts and prefer the use of "self" better, but it is a gratuitous change that unilaterally reverses the decisions of the authors and makes the code not match any of the surrounding code that uses the prior conventions.
Ah, at least I concur with you on one point :-) Changes to convert functions to AC must not change the API (type of arguments: positional only/keyword/..., default values, etc.) nor provide a worse docstring.
There is an active on-going work to enhance AC to fix issues that you reported, like the default value of positional-only parameters which should not be rendered in the function signature (I created the issue #29299 with a patch). Serhiy is also working on implementing the last major missing feature of AC: support *args and **kwargs parameters (issue #20291).
FYI I wasn't involved in AC changes, I only started to look at AC recently (1 or 2 months ago). Again, I agree that these changes should be carefully reviewed, which is an hard task since required changes are usually large and move a lot of code. We need more eyes to look at these changes!
For the specific case of sorted(), the name of first parameter is already documented in the docstring and documentation in Python 2.7: "iterable". So I guess that you mean that it is now possible to use it as a keyword argument. Well, see the issue #29327 for the long story. This issue is a regression, it was already fixed, and I didn't introduce the API change.
Oh by the way, when I read your comment, I understand that I'm responsible of all regressions. It's true that I introduced regressions, that's where I said "shit happens" (or more politically correct: "it's better to ask forgiveness than permission" ;-)). Since I'm one of the most active contributor in CPython, I'm not surprised of being the one who introduce many (most?) regressions :-) I'm trying to review my changes multiple times, test corner cases, etc. But I'm not perfect.
Sadly, to show its full power, FASTCALL requires changes at many levels of the code. It requires to change at lot of code, but I understood that core developers approved the whole project. Maybe I was wrong? At least, I asked for permissions multiple changes, especially at the start.
- FWIW, the claim that the help is much better is specious. AFAICT, there has never been the slightest problem with "sorted(iterable, key=None, reverse=False) --> new sorted list" which has been clear since the day it was released. It is some of the new strings the are causing problems with users (my students frequently are tripped-up by the / notation for example; no one seems to be able to intuit what it means without it being explained first).
Good news, it seems like you have a good experience in API design, documentation, etc. Join the "Argument Clinic" project to help us to enhance docstrings, function signatures and documentation ;-)
See the good part of the AC on-going work: it's a nice opportunity to also enhance documentation, not only provide a signature.
By the way, to be honest, the main advantage of converting functions to AC is to get a signature. The signature is visible in docstrings which is nice, but it is also very useful to a wide range of tools like (IDE, static checks, etc.).
Conversion to FASTCALL is more a nice effect. At least, it is a good motivation for me to convert mor and more code to AC :-)
AC moves docstring closer to the list of parameters. IHMO it makes the C code simpler to read and understand. It also removes the boring code responsible to "parse" arguments, so it makes the code shorter. But well, this is just my opinion.
- FWIW, I'm trying to be constructive and contribute where I can, but frankly I can't keep up with the volume of churn. Having seen bugs being introduced, it is not inappropriate to ask another dev to please be careful, especially when that dev has been prolific to an unprecedented degree and altering core parts of the language for function calls, to new opcodes, the memory allocators, etc. Very few people on the planet are competent to review these changes, make reasonable assessments about whether the complexity and churn are worth it. An fewer still have the time to keep up with the volume of changes.
Hum, I wasn't involved in bytecode changes.
Well, I reviewed the very good work of Demur Rumed. I recall that you worked on a similar area, trying to fetch bytecode by 16-bit instead of 8-bit. Demur proposed a good design and I recall that the design was approved.
I helped a little bit on the implementation and I pushed the final change, but all credits go to Demur and Serhiy Storshaka! By the way, Serhiy made further efficient enhancements in the bytecode of CALL_FUNCTION instructions.
About memory allocations, I guess that you are referring to my change on PyMem_Malloc() allocator. I discussed the issue on python-dev and waited for approval of my peers before pushing anything, since I know well that it's a critical part of Python: https://mail.python.org/pipermail/python-dev/2016-March/143467.html
I provide all data requested by Marc Andre Lemburg (test the change with common projects, Django, Pillow, numpy) and made further changes (PYTHONMALLOC=debug tool) to help to handle this backward incompatible change (GIL is now required to call PyMem_Malloc).
Hopefully, it seems like nobody noticed this subtle change (GIL now requied): I didn't see any bug report. By the way, I fixed a misused PyMem_Mem() in numpy.
- Please do continue your efforts to improve the language, but also please moderate the rate of change, mitigate the addition complexity, value stability over micro-optimizations, consult the authors and maintainers of code, take special care without code that hasn't been reviewed because that lacks a safety net, and remember that newer devs may be taking cues from you (do you want them making extensive changes to long existing stable code without consulting the authors and with weak LGTM reviews?)
Ok, I will do it.
Thank you for you feedback Raymond. I hope that my email helps you to understand how I work and how I take my decisions.
Victor
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Dormant core dev here. Not contributing at all due to severe lack of time in the past year and a half, not likely to have more time in the near future. Also no longer working with Python at all except as a hobby :(
I could pull off a review once a month if it would actually help!
On Sat, Jan 21, 2017 at 9:51 PM, Brett Cannon brett@python.org wrote:
What I'm picking up from this is (as a gross oversimplification):
- Victor _wants_ code reviews
- Raymond thinks we _need_ code reviews
So the common theme here regardless of whether you agree with Raymond or Victor's approach to development is that we are not getting enough code reviews to go around. To me that's what the systemic issue is that this email is bringing up.
Now I think most of us don't think the solution to the lack of reviews is to lower our standard of what it takes to become a core developer (this doesn't mean we shouldn't do a better job of identifying potential candidates, just that we shouldn't give people commit privileges after a single patch like some projects do). To me that means we need to address why out of 79 core developers only 39 have a single commit over the past year, 30/79 have more than 12 commits over that same time scale, 15/79 people have more than 52 commits, and 2/79 people have over 365 commits (https://github.com/python/cpython/graphs/contributors?from=2016-01-22&to=2017-01-21&type=c for the stats).
Some of you have said you're waiting for the GitHub migration before you start contributing again, which I can understand (I'm going to be sending an email with an update on that after this email to python-dev & core-workflow). But for those that have not told me that I don't know what it will take to get you involved again. For instance, not to pick on Andrew but he hasn't committed anything but he obviously still cares about the project. So what would it take to get Andrew to help review patches again so that the next time something involving random comes through he feels like taking a quick look?
As I have said before, the reason I took on the GitHub migration is for us core developers. I want our workflow to be as easy as possible so that we can be as productive as possible. But the unspoken goal I have long-term is to get to the point that even dormant core devs want to contribute again, and to the point that everyone reviews a patch/month and more people reviewing a patch/week (although I'll take a patch/year to start). I want to get to the point that every person with commit privileges takes 30 minutes a month to help with reviews and that the majority of folks take 30 minutes a week to review (and please don't think this as a hard rule and if you don't the privileges go away, view this as an aspirational goal). Even if people who don't have time to review the kind of patches Victor is producing which triggered this thread, reviewing documentation patches can be done without deep knowledge of things and without taking much time. That way people who have time to review the bigger, more difficult patches can actually spend their time on those reviews and not worrying about patches fixing a spelling mistake or adding a new test to raise test coverage.
All of this is so that I hope one day we get to the point where all patches require a review no matter who proposed the code change. Now I think we're quite a ways of from being there, but that's my moonshot goal for our workflow: that we have enough quality reviews coming in that we feel that even patches from fellow core developers is worth requiring the extra code check and disbursement of knowledge without feeling like a terrible drag on productivity.
Once the GitHub migration has occurred I'm planning to tackle our Misc/NEWS problem and then automate Misc/ACKS. After that, though, I hope we can take the time to have a hard look at what in our workflow prevents people from making even occasional code reviews so that everyone wants to help out again (and if any of this interests you then please subscribe to core-workflow).
On Fri, 20 Jan 2017 at 02:46 Victor Stinner victor.stinner@gmail.com wrote:
Hi,
Raymond Hettinger used a regression that I introduced in the builtin sorted() function (in Python 3.6.0) to give me his feedback on my FASTCALL work, but also on Argument Clinic.
Context: http://bugs.python.org/issue29327#msg285848
Since the reported issues is wider than just FASTCALL, including how I contribute to CPython, I decided to discuss the topic with a wider audience. I continue the discussion on python-committers to get the opinion of the other core developers.
Sorry for my very long answer! I tried to answer to each issues reported by Raymond.
Inaccurate summary: I'm a strong supporter of "it's better to ask forgiveness than permission", whereas Raymond considers that I introduced too many regressions with my workflow.
Raymond Hettinger added the comment:
A few random thoughts that may or may not be helpful:
- We now have two seasoned developers and one new core developer that
collectively are creating many non-trivial patches to core parts of Python at an unprecedented rate of change. The patches are coming in much faster than they can reasonably be reviewed and carefully considered, especially by devs such as myself who have very limited time available. IMO, taken as whole, these changes are destabilizing the language. Python is so successful and widely adopted that we can't afford a "shit happens" attitude. Perhaps that works in corners of the language, infrequently used modules, but it makes less sense when touching the critical paths that have had slow and careful evolution over 26 years.
- Besides the volume of patches, one other reason that reviews are hard to come by is that they apply new APIs that I don't fully understand yet. There are perhaps two people on the planet who could currently give thoughtful, correct, and critical evaluation of all those patches. Everyone else is just watching them all fly by and hoping that something good is happening.
Since one or maybe even two years, I noticed that many of my issues were blocked by the lack of reviews. As you wrote, only few developer have the knowledge and background to be able to provide a good review (not only "tests pass, so LGTM") on my changes modifying the Python core.
I also wanted to discuss this topic, but I didn't really know what to propose. Let's take this opportunity to explain how I contribute to CPython, especially how I decide to wait for a review or not.
For each patch that I write, I estimate the risk of regression. You may know that any regression is something unexpected, so such estimation is tricky. Here is my heuristic:
(*) if the patch is trivial (short, non controversal), I push it immediatly.
(*) If I'm less confident, I open an issue and attach the patch. I wait at least one day before pushing.
It's strange, but the process of opening an issue and attaching the patch usually helps to review the code myself (find bugs, or more generally enhance the patch). Maybe because it forces me to review the change one more time?
If the change is not part of a larger patch serie, so doesn't block me to move further, I try to keep the issue open around one week.
The truth is that too few of my patches get a review :-/ Maybe I should wait longer, but then it becomes harder for me to handle many patches.
Maybe it's a tooling issues. Recently, I started to use local branches in a Git repository. It helps a lot of work on parallel on large changes. Before, I only worked in a single directory (default/, the default Mercurial branch) and applied/reverted patches everytime. It's painful, especially when I have to use multiple computers, download again publshed patches, etc. Maybe it will run smoother once CPython will move to Git and GitHub.
By the way, it's painful to squash a long patch serie into a giant patch, much harder to review, where changes don't make sense at all at the first look. Again, a better reviewing tool supporting patch series (GitHub) will help here too.
Not supporting patch series in our reviewing tool also explains why I prefer to push than having to wait for a review. Rebasing manually long patch series stored as giant .patch files is complicated.
(*) If the change changes an API or changes a core component, I wait for at least one review from a core reviewer. Sometimes, I even send an email to python-dev. Again, sometimes I don't get any feedback on the patch nor the email after two weeks :-/ At least, I tried :-) Usually, I get feedback in less than one week, or no feedback at all. I understand that nobody understands my change or nobody cares :-)
I totally understand that most core developers have a little amount of time available to contribute to Python. I'm trying to find a compromise between the risk of introducing regressions and being stuck in my work. This email might help me to adjust my workflow.
By the way, I'm trying to always run the full test suite (./python -m test -rW -j0) before pushing any change. If I suspect that I may have introduced reference leaks, I also run "./python -m test -R 3:3 ..." on the tests related to the modified code to check for memory/reference leaks.
- One other reason for the lack of review comments in the enthusiasm and fervor surrounding the patches. I feel like there is a cost of questioning whether the patches should be done or how they are done, like I am burning little karma every time. Sometimes it feels safest and most cordial to just say nothing and let you make hundreds of semi-reviewed changes to just about every critical part of the language.
"semi-reviewed". Let me be more accurate: yeah, I do push a lot of changes which were not reviewed by anyone (see above).
- Historically, if there was creator or maintainer of the code who was still active, that person would always be consulted and have a final say on whether a change should be applied. Now, we have code constantly being changed without consulting the original author (for example, the recent and catastrophic random initialization bug was due to application of a patch without consulting the author of _randommodule.c and the maintainer of random.py, or this change to sorted(), or the changes to decimal, etc).
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
My problem is that I'm not aware of any explicit list of maintainers. I didn't know that you were the maintainer of the random module before you told me that at the Facebook sprint last september. I didn't expect that the random module had a maintainer, I thought that any core developer would be allowed to modify the code.
Moreover, since I open an issue for most of my changes, it gives an opportunity to maintainers to review changes. Maybe we need more components in the bug tracker to notify maintainers of pending changes?
You mentionned 3 different changes, let me reply.
(1) The random change: http://bugs.python.org/issue29085
I introduced a regression in random.Random.seed(): a typo in the C code has the consequence that the current time and process identifier is used, instead of os.urandom(16), to initialize the Mersenne Twister RNG.
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
Since we are talking about the development process, let me see how the change was made.
Context: The PEP 524 has a long and painful history... Something like more than 500 messages were sent on the bug tracker and python-dev, and nobody was listening to each others, two security experts "rage-quitted" Python because of this mess... I decided to try to fix this issue in a constructive way, so I wrote a PEP. Nick wrote a different PEP, since it was clear that it was possible to handle security in two different incompatible ways. A mailing list was even created just to discuss this bug! A mailing list just for a bug gives an idea of the size of the mess :-)
Well, about the change itself, it was done in http://bugs.python.org/issue27776
The patch was available for review during 19 days (2016-08-18-2016-09-06) and was reviewed by Nick Coghlan. Since Nick wrote a similar PEP, I trusted him to be able to review my change. (Well, anyway I already trust all core developers, but I mean that I was trusting him even more than usual :-))
Since the change has a big impact on security, I had prefer to get a review of more developers, especially our security experts... but as I wrote, two security experts "rage- quitted". Again, this PEP has a long and sad story :-/
Note: you say that you are the maintainer of the random module, but I don't recall having see you in any recent discussions and issues related to os.urandom(), whereas a lot of enhancements and changes were done last 2 years. I made many changes to support new OS functions like getentropy() an getrandom().
Oooookay, let's see the second change, "this change to sorted()", http://bugs.python.org/issue29327
(2) I introduced a bug in sorted(), last August: https://hg.python.org/cpython/rev/15eab21bf934/
Calling sorted(iterable=[]) does crash. To be honest, I didn't imagine that anyone would pass the iterable by keyword, but Serhiy is very good to spot bugs in corner cases :-)
IMHO the regression is subtle.
When I optimized the code to use FASTCALL, I replaced PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1). I checked that all tests passed, so it looks ok to me.
I didn't imagine that anyone would call sorted(iterable=[]), so I didn't notice that PyTuple_GetSlice() can create an empty tuple.
The previous code was wrong since sorted() accepted iterable as a keyword, whereas sort.list() doesn't.
So well, I let you guess if a review would have spot this bug in the large change.
(3) Recently, I ran sed to replace code patterns to use faster ways to call functions: https://hg.python.org/cpython/rev/54a89144ee1d
"Replace PyObject_CallObject(callable, NULL) with _PyObject_CallNoArg(callable)"
I recalled that I modified the _decimal module and that Stefan Krah complained, because he wants to have the same code base on Python 3.5, 3.6 and 3.7. He also mentionned an external test suite which was broken by recent _decimal changes (not sure if my specific change was in cause or not), but I wasn't aware of it.
To be honest, I didn't even notice that I modified _decimal when I ran sed on all .c files. Since the change was straightforward and (IMHO) made the code more readable, I didn't even wait for a review if I recall correctly.
Stefan and me handled this issue privately (he reverted my change), I'm not sure that it's worth it to say more about this "issue" (or even "non-issue").
To be clear, I don't consider that my change introduced a regression.
In general, Guido has been opposed to sweeping changes across the code base for only tiny benefits. Of late, that rule seems to have been lost.
The benefits of FASTCALL mainly apply to fine grained functions which only do a little work and tend to be called frequently in loops. For functions such as sorted(), the calling overhead is dominated by the cost of actually doing the sort. For sorted(), FASTCALL is truly irrelevant and likely wasn't worth the complexity, or the actual bug, or any of the time we've now put in it. There was no actual problem being solved, just a desire to broadly apply new optimizations.
Ok, first, you qualify my FASTCALL changes as code churn. So let me show an example with sorted(): https://hg.python.org/cpython/rev/b34d2ef5c412
Can you elaborate how such change increases the complexity?
Second, "no actual problem being solved"
Since the goal of FASTCALL is to optimize Python, I guess that you consider that the speedup doesn't justify the change. I gave numbers in the issue #29327:
Microbenchmark on sorted() on Python 3.7 compared to 3.5 (before FASTCALL):
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10))' 'sorted(seq)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 1.07 us +- 0.06 us -> [3.7] 958 ns +- 15 ns: 1.12x faster (-11%)
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10)); k=lambda x:x' 'sorted(seq, key=k)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 3.34 us +- 0.07 us -> [3.7] 2.66 us +- 0.05 us: 1.26x faster (-21%)
IMHO such speedup is significant even on a microbenchmark. Can you elaborate what are your criteria to decide if an optimization is worth it?
- Historically, we've relied on core developers showing restraint. Not every idea that pops into their head is immediately turned into a patch accompanied by pressure to apply it. Devs tended to restrict themselves to parts of the code they knew best through long and careful study rather sweeping through modules and altering other people's carefully crafted code.
Should I understand that I should restrict myself to some files? Or not touch some specific parts of Python, like... "your" code like random, itertools and collections modules?
I replied to the 3 issues you mentioned previously and explained how I contribute to Python.
FWIW, I applaud your efforts to reduce call overhead -- that has long been a sore spot for the language.
Guido has long opposed optimizations that increase risk of bugs, introduce complexity, or that affect long-term maintainability. In some places, it looks like FASTCALL is increasing the complexity (replacing something simple and well-understood with a wordier, more intricate API that I don't yet fully understand and will affect my ability to maintain the surrounding code).
I'm sorry, I didn't spent much time on explaing the FASTCALL design nor documenting my changes. It's partially deliberate to make everything related to FASTCALL private. Since it's a huge project modifying a lot of code, I wanted to wait until the APIs and the code stop moving too fast to take time to explain my work and document it.
If you have specific questions, please go ahead.
Shortest summary:
FASTCALL replaces (args: tuple, kwargs: optional dict) with (args: C array, nargs: int, kwnames: tuple of keyword keys). It's a new calling convention which allows to avoid a temporary tuple to pass positional arguments and avoids temporary dictionary to pass keyworkd arguments.
To use FASTCALL, C functions should be converted to the new METH_FASTCALL calling convention
PyObject_Call() can replaced with _PyObject_FastCallKeywords() or _PyObject_FastCallDict() (when we still get kwargs as a dict) in such conversion
Many existing C functions were optimized internally to use FASCALL, so even if you don't modify your code, you will benefit of it (speedup). Typical example: PyFunction_CallFunctionObjArgs().
The most massive change were purely internal and don't affect the most famous C APIs at all. In some cases, to fully benefit of FASTCALL, code should be modified. I'm trying to restrict such changes to Python internals, especially the most used functions.
I expected that the required changes were straightforward enough, it looks like I was wrong, but I don't recall anyone, before you recently, asking for an explanation.
- It was no long ago that you fought tooth-and-nail against a single line patch optimization I submitted. The code was clearly correct and had a simple disassembly to prove its benefit. Your opposition was based on "it increases the complexity of the code, introduces a maintenance cost, and increases the risk of bugs". In the end, your opposition killed the patch. But now, the AC and FASTCALL patches don't seem to mind any of these considerations.
Context: http://bugs.python.org/issue26201
It seems like we need more _explicit_ rules to decide if an optimization is worth it or not. For me, the de facto standard request for an optimization is to prove it with a benchmark. I requested a benchmark, but you refused to provide it.
So I ran my own benchmark and saw that your change made the modified code (PyList_Append()) 6% slower. I'm not sure that my bencmark was correct, but it was a first step to take a decision.
To come back to FASTCALL, your point is that it doesn't provide any speedup.
In most FASTCALL issues that I opened, I provide a script to reproduce my benchmark and the benchmark results. The speedup is usually betwen 10% and 20% faster.
Should I understand that 6% slower is ok, whereas 10-20% faster is not good? Can you please elaborate?
- AC is supposed to be a CPython-only concept. But along the way APIs are being changed without discussion. I don't mind that sorted() now exposes *iterable* as a keyword argument, but it was originally left out on purpose (Tim opined that code would look worse with iterable as a keyword argument). That decision was reversed unilaterally without consulting the author and without a test. Also as AC is being applied, the variable names are being changed. I never really liked the "mp" that used in dicts and prefer the use of "self" better, but it is a gratuitous change that unilaterally reverses the decisions of the authors and makes the code not match any of the surrounding code that uses the prior conventions.
Ah, at least I concur with you on one point :-) Changes to convert functions to AC must not change the API (type of arguments: positional only/keyword/..., default values, etc.) nor provide a worse docstring.
There is an active on-going work to enhance AC to fix issues that you reported, like the default value of positional-only parameters which should not be rendered in the function signature (I created the issue #29299 with a patch). Serhiy is also working on implementing the last major missing feature of AC: support *args and **kwargs parameters (issue #20291).
FYI I wasn't involved in AC changes, I only started to look at AC recently (1 or 2 months ago). Again, I agree that these changes should be carefully reviewed, which is an hard task since required changes are usually large and move a lot of code. We need more eyes to look at these changes!
For the specific case of sorted(), the name of first parameter is already documented in the docstring and documentation in Python 2.7: "iterable". So I guess that you mean that it is now possible to use it as a keyword argument. Well, see the issue #29327 for the long story. This issue is a regression, it was already fixed, and I didn't introduce the API change.
Oh by the way, when I read your comment, I understand that I'm responsible of all regressions. It's true that I introduced regressions, that's where I said "shit happens" (or more politically correct: "it's better to ask forgiveness than permission" ;-)). Since I'm one of the most active contributor in CPython, I'm not surprised of being the one who introduce many (most?) regressions :-) I'm trying to review my changes multiple times, test corner cases, etc. But I'm not perfect.
Sadly, to show its full power, FASTCALL requires changes at many levels of the code. It requires to change at lot of code, but I understood that core developers approved the whole project. Maybe I was wrong? At least, I asked for permissions multiple changes, especially at the start.
- FWIW, the claim that the help is much better is specious. AFAICT, there has never been the slightest problem with "sorted(iterable, key=None, reverse=False) --> new sorted list" which has been clear since the day it was released. It is some of the new strings the are causing problems with users (my students frequently are tripped-up by the / notation for example; no one seems to be able to intuit what it means without it being explained first).
Good news, it seems like you have a good experience in API design, documentation, etc. Join the "Argument Clinic" project to help us to enhance docstrings, function signatures and documentation ;-)
See the good part of the AC on-going work: it's a nice opportunity to also enhance documentation, not only provide a signature.
By the way, to be honest, the main advantage of converting functions to AC is to get a signature. The signature is visible in docstrings which is nice, but it is also very useful to a wide range of tools like (IDE, static checks, etc.).
Conversion to FASTCALL is more a nice effect. At least, it is a good motivation for me to convert mor and more code to AC :-)
AC moves docstring closer to the list of parameters. IHMO it makes the C code simpler to read and understand. It also removes the boring code responsible to "parse" arguments, so it makes the code shorter. But well, this is just my opinion.
- FWIW, I'm trying to be constructive and contribute where I can, but frankly I can't keep up with the volume of churn. Having seen bugs being introduced, it is not inappropriate to ask another dev to please be careful, especially when that dev has been prolific to an unprecedented degree and altering core parts of the language for function calls, to new opcodes, the memory allocators, etc. Very few people on the planet are competent to review these changes, make reasonable assessments about whether the complexity and churn are worth it. An fewer still have the time to keep up with the volume of changes.
Hum, I wasn't involved in bytecode changes.
Well, I reviewed the very good work of Demur Rumed. I recall that you worked on a similar area, trying to fetch bytecode by 16-bit instead of 8-bit. Demur proposed a good design and I recall that the design was approved.
I helped a little bit on the implementation and I pushed the final change, but all credits go to Demur and Serhiy Storshaka! By the way, Serhiy made further efficient enhancements in the bytecode of CALL_FUNCTION instructions.
About memory allocations, I guess that you are referring to my change on PyMem_Malloc() allocator. I discussed the issue on python-dev and waited for approval of my peers before pushing anything, since I know well that it's a critical part of Python: https://mail.python.org/pipermail/python-dev/2016-March/143467.html
I provide all data requested by Marc Andre Lemburg (test the change with common projects, Django, Pillow, numpy) and made further changes (PYTHONMALLOC=debug tool) to help to handle this backward incompatible change (GIL is now required to call PyMem_Malloc).
Hopefully, it seems like nobody noticed this subtle change (GIL now requied): I didn't see any bug report. By the way, I fixed a misused PyMem_Mem() in numpy.
- Please do continue your efforts to improve the language, but also please moderate the rate of change, mitigate the addition complexity, value stability over micro-optimizations, consult the authors and maintainers of code, take special care without code that hasn't been reviewed because that lacks a safety net, and remember that newer devs may be taking cues from you (do you want them making extensive changes to long existing stable code without consulting the authors and with weak LGTM reviews?)
Ok, I will do it.
Thank you for you feedback Raymond. I hope that my email helps you to understand how I work and how I take my decisions.
Victor
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
I've been on the sidelines for a while myself for a number of reasons, but the shift to GitHub will pull me back in for sure, at least in terms of code review. I look forward to actually contributing code again soon, but easier tooling on reviews—rather, a shiny new one, as I'm aware of Reitveld—is enough of a carrot to bring me back in.
On Sun, Jan 22, 2017 at 12:08 PM, Tal Einat taleinat@gmail.com wrote:
Dormant core dev here. Not contributing at all due to severe lack of time in the past year and a half, not likely to have more time in the near future. Also no longer working with Python at all except as a hobby :(
I could pull off a review once a month if it would actually help!
On Sat, Jan 21, 2017 at 9:51 PM, Brett Cannon brett@python.org wrote:
What I'm picking up from this is (as a gross oversimplification):
- Victor _wants_ code reviews
- Raymond thinks we _need_ code reviews
So the common theme here regardless of whether you agree with Raymond or Victor's approach to development is that we are not getting enough code reviews to go around. To me that's what the systemic issue is that this email is bringing up.
Now I think most of us don't think the solution to the lack of reviews is to lower our standard of what it takes to become a core developer (this doesn't mean we shouldn't do a better job of identifying potential candidates, just that we shouldn't give people commit privileges after a single patch like some projects do). To me that means we need to address why out of 79 core developers only 39 have a single commit over the past year, 30/79 have more than 12 commits over that same time scale, 15/79 people have more than 52 commits, and 2/79 people have over 365 commits (https://github.com/python/cpython/graphs/contributors?from=2016-01-22&to=2017-01-21&type=c for the stats).
Some of you have said you're waiting for the GitHub migration before you start contributing again, which I can understand (I'm going to be sending an email with an update on that after this email to python-dev & core-workflow). But for those that have not told me that I don't know what it will take to get you involved again. For instance, not to pick on Andrew but he hasn't committed anything but he obviously still cares about the project. So what would it take to get Andrew to help review patches again so that the next time something involving random comes through he feels like taking a quick look?
As I have said before, the reason I took on the GitHub migration is for us core developers. I want our workflow to be as easy as possible so that we can be as productive as possible. But the unspoken goal I have long-term is to get to the point that even dormant core devs want to contribute again, and to the point that everyone reviews a patch/month and more people reviewing a patch/week (although I'll take a patch/year to start). I want to get to the point that every person with commit privileges takes 30 minutes a month to help with reviews and that the majority of folks take 30 minutes a week to review (and please don't think this as a hard rule and if you don't the privileges go away, view this as an aspirational goal). Even if people who don't have time to review the kind of patches Victor is producing which triggered this thread, reviewing documentation patches can be done without deep knowledge of things and without taking much time. That way people who have time to review the bigger, more difficult patches can actually spend their time on those reviews and not worrying about patches fixing a spelling mistake or adding a new test to raise test coverage.
All of this is so that I hope one day we get to the point where all patches require a review no matter who proposed the code change. Now I think we're quite a ways of from being there, but that's my moonshot goal for our workflow: that we have enough quality reviews coming in that we feel that even patches from fellow core developers is worth requiring the extra code check and disbursement of knowledge without feeling like a terrible drag on productivity.
Once the GitHub migration has occurred I'm planning to tackle our Misc/NEWS problem and then automate Misc/ACKS. After that, though, I hope we can take the time to have a hard look at what in our workflow prevents people from making even occasional code reviews so that everyone wants to help out again (and if any of this interests you then please subscribe to core-workflow).
On Fri, 20 Jan 2017 at 02:46 Victor Stinner victor.stinner@gmail.com wrote:
Hi,
Raymond Hettinger used a regression that I introduced in the builtin sorted() function (in Python 3.6.0) to give me his feedback on my FASTCALL work, but also on Argument Clinic.
Context: http://bugs.python.org/issue29327#msg285848
Since the reported issues is wider than just FASTCALL, including how I contribute to CPython, I decided to discuss the topic with a wider audience. I continue the discussion on python-committers to get the opinion of the other core developers.
Sorry for my very long answer! I tried to answer to each issues reported by Raymond.
Inaccurate summary: I'm a strong supporter of "it's better to ask forgiveness than permission", whereas Raymond considers that I introduced too many regressions with my workflow.
Raymond Hettinger added the comment:
A few random thoughts that may or may not be helpful:
- We now have two seasoned developers and one new core developer that
collectively are creating many non-trivial patches to core parts of Python at an unprecedented rate of change. The patches are coming in much faster than they can reasonably be reviewed and carefully considered, especially by devs such as myself who have very limited time available. IMO, taken as whole, these changes are destabilizing the language. Python is so successful and widely adopted that we can't afford a "shit happens" attitude. Perhaps that works in corners of the language, infrequently used modules, but it makes less sense when touching the critical paths that have had slow and careful evolution over 26 years.
- Besides the volume of patches, one other reason that reviews are hard to come by is that they apply new APIs that I don't fully understand yet. There are perhaps two people on the planet who could currently give thoughtful, correct, and critical evaluation of all those patches. Everyone else is just watching them all fly by and hoping that something good is happening.
Since one or maybe even two years, I noticed that many of my issues were blocked by the lack of reviews. As you wrote, only few developer have the knowledge and background to be able to provide a good review (not only "tests pass, so LGTM") on my changes modifying the Python core.
I also wanted to discuss this topic, but I didn't really know what to propose. Let's take this opportunity to explain how I contribute to CPython, especially how I decide to wait for a review or not.
For each patch that I write, I estimate the risk of regression. You may know that any regression is something unexpected, so such estimation is tricky. Here is my heuristic:
(*) if the patch is trivial (short, non controversal), I push it immediatly.
(*) If I'm less confident, I open an issue and attach the patch. I wait at least one day before pushing.
It's strange, but the process of opening an issue and attaching the patch usually helps to review the code myself (find bugs, or more generally enhance the patch). Maybe because it forces me to review the change one more time?
If the change is not part of a larger patch serie, so doesn't block me to move further, I try to keep the issue open around one week.
The truth is that too few of my patches get a review :-/ Maybe I should wait longer, but then it becomes harder for me to handle many patches.
Maybe it's a tooling issues. Recently, I started to use local branches in a Git repository. It helps a lot of work on parallel on large changes. Before, I only worked in a single directory (default/, the default Mercurial branch) and applied/reverted patches everytime. It's painful, especially when I have to use multiple computers, download again publshed patches, etc. Maybe it will run smoother once CPython will move to Git and GitHub.
By the way, it's painful to squash a long patch serie into a giant patch, much harder to review, where changes don't make sense at all at the first look. Again, a better reviewing tool supporting patch series (GitHub) will help here too.
Not supporting patch series in our reviewing tool also explains why I prefer to push than having to wait for a review. Rebasing manually long patch series stored as giant .patch files is complicated.
(*) If the change changes an API or changes a core component, I wait for at least one review from a core reviewer. Sometimes, I even send an email to python-dev. Again, sometimes I don't get any feedback on the patch nor the email after two weeks :-/ At least, I tried :-) Usually, I get feedback in less than one week, or no feedback at all. I understand that nobody understands my change or nobody cares :-)
I totally understand that most core developers have a little amount of time available to contribute to Python. I'm trying to find a compromise between the risk of introducing regressions and being stuck in my work. This email might help me to adjust my workflow.
By the way, I'm trying to always run the full test suite (./python -m test -rW -j0) before pushing any change. If I suspect that I may have introduced reference leaks, I also run "./python -m test -R 3:3 ..." on the tests related to the modified code to check for memory/reference leaks.
- One other reason for the lack of review comments in the enthusiasm and fervor surrounding the patches. I feel like there is a cost of questioning whether the patches should be done or how they are done, like I am burning little karma every time. Sometimes it feels safest and most cordial to just say nothing and let you make hundreds of semi-reviewed changes to just about every critical part of the language.
"semi-reviewed". Let me be more accurate: yeah, I do push a lot of changes which were not reviewed by anyone (see above).
- Historically, if there was creator or maintainer of the code who was still active, that person would always be consulted and have a final say on whether a change should be applied. Now, we have code constantly being changed without consulting the original author (for example, the recent and catastrophic random initialization bug was due to application of a patch without consulting the author of _randommodule.c and the maintainer of random.py, or this change to sorted(), or the changes to decimal, etc).
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
My problem is that I'm not aware of any explicit list of maintainers. I didn't know that you were the maintainer of the random module before you told me that at the Facebook sprint last september. I didn't expect that the random module had a maintainer, I thought that any core developer would be allowed to modify the code.
Moreover, since I open an issue for most of my changes, it gives an opportunity to maintainers to review changes. Maybe we need more components in the bug tracker to notify maintainers of pending changes?
You mentionned 3 different changes, let me reply.
(1) The random change: http://bugs.python.org/issue29085
I introduced a regression in random.Random.seed(): a typo in the C code has the consequence that the current time and process identifier is used, instead of os.urandom(16), to initialize the Mersenne Twister RNG.
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
Since we are talking about the development process, let me see how the change was made.
Context: The PEP 524 has a long and painful history... Something like more than 500 messages were sent on the bug tracker and python-dev, and nobody was listening to each others, two security experts "rage-quitted" Python because of this mess... I decided to try to fix this issue in a constructive way, so I wrote a PEP. Nick wrote a different PEP, since it was clear that it was possible to handle security in two different incompatible ways. A mailing list was even created just to discuss this bug! A mailing list just for a bug gives an idea of the size of the mess :-)
Well, about the change itself, it was done in http://bugs.python.org/issue27776
The patch was available for review during 19 days (2016-08-18-2016-09-06) and was reviewed by Nick Coghlan. Since Nick wrote a similar PEP, I trusted him to be able to review my change. (Well, anyway I already trust all core developers, but I mean that I was trusting him even more than usual :-))
Since the change has a big impact on security, I had prefer to get a review of more developers, especially our security experts... but as I wrote, two security experts "rage- quitted". Again, this PEP has a long and sad story :-/
Note: you say that you are the maintainer of the random module, but I don't recall having see you in any recent discussions and issues related to os.urandom(), whereas a lot of enhancements and changes were done last 2 years. I made many changes to support new OS functions like getentropy() an getrandom().
Oooookay, let's see the second change, "this change to sorted()", http://bugs.python.org/issue29327
(2) I introduced a bug in sorted(), last August: https://hg.python.org/cpython/rev/15eab21bf934/
Calling sorted(iterable=[]) does crash. To be honest, I didn't imagine that anyone would pass the iterable by keyword, but Serhiy is very good to spot bugs in corner cases :-)
IMHO the regression is subtle.
When I optimized the code to use FASTCALL, I replaced PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1). I checked that all tests passed, so it looks ok to me.
I didn't imagine that anyone would call sorted(iterable=[]), so I didn't notice that PyTuple_GetSlice() can create an empty tuple.
The previous code was wrong since sorted() accepted iterable as a keyword, whereas sort.list() doesn't.
So well, I let you guess if a review would have spot this bug in the large change.
(3) Recently, I ran sed to replace code patterns to use faster ways to call functions: https://hg.python.org/cpython/rev/54a89144ee1d
"Replace PyObject_CallObject(callable, NULL) with _PyObject_CallNoArg(callable)"
I recalled that I modified the _decimal module and that Stefan Krah complained, because he wants to have the same code base on Python 3.5, 3.6 and 3.7. He also mentionned an external test suite which was broken by recent _decimal changes (not sure if my specific change was in cause or not), but I wasn't aware of it.
To be honest, I didn't even notice that I modified _decimal when I ran sed on all .c files. Since the change was straightforward and (IMHO) made the code more readable, I didn't even wait for a review if I recall correctly.
Stefan and me handled this issue privately (he reverted my change), I'm not sure that it's worth it to say more about this "issue" (or even "non-issue").
To be clear, I don't consider that my change introduced a regression.
In general, Guido has been opposed to sweeping changes across the code base for only tiny benefits. Of late, that rule seems to have been lost.
The benefits of FASTCALL mainly apply to fine grained functions which only do a little work and tend to be called frequently in loops. For functions such as sorted(), the calling overhead is dominated by the cost of actually doing the sort. For sorted(), FASTCALL is truly irrelevant and likely wasn't worth the complexity, or the actual bug, or any of the time we've now put in it. There was no actual problem being solved, just a desire to broadly apply new optimizations.
Ok, first, you qualify my FASTCALL changes as code churn. So let me show an example with sorted(): https://hg.python.org/cpython/rev/b34d2ef5c412
Can you elaborate how such change increases the complexity?
Second, "no actual problem being solved"
Since the goal of FASTCALL is to optimize Python, I guess that you consider that the speedup doesn't justify the change. I gave numbers in the issue #29327:
Microbenchmark on sorted() on Python 3.7 compared to 3.5 (before FASTCALL):
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10))' 'sorted(seq)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 1.07 us +- 0.06 us -> [3.7] 958 ns +- 15 ns: 1.12x faster (-11%)
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10)); k=lambda x:x' 'sorted(seq, key=k)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 3.34 us +- 0.07 us -> [3.7] 2.66 us +- 0.05 us: 1.26x faster (-21%)
IMHO such speedup is significant even on a microbenchmark. Can you elaborate what are your criteria to decide if an optimization is worth it?
- Historically, we've relied on core developers showing restraint. Not every idea that pops into their head is immediately turned into a patch accompanied by pressure to apply it. Devs tended to restrict themselves to parts of the code they knew best through long and careful study rather sweeping through modules and altering other people's carefully crafted code.
Should I understand that I should restrict myself to some files? Or not touch some specific parts of Python, like... "your" code like random, itertools and collections modules?
I replied to the 3 issues you mentioned previously and explained how I contribute to Python.
FWIW, I applaud your efforts to reduce call overhead -- that has long been a sore spot for the language.
Guido has long opposed optimizations that increase risk of bugs, introduce complexity, or that affect long-term maintainability. In some places, it looks like FASTCALL is increasing the complexity (replacing something simple and well-understood with a wordier, more intricate API that I don't yet fully understand and will affect my ability to maintain the surrounding code).
I'm sorry, I didn't spent much time on explaing the FASTCALL design nor documenting my changes. It's partially deliberate to make everything related to FASTCALL private. Since it's a huge project modifying a lot of code, I wanted to wait until the APIs and the code stop moving too fast to take time to explain my work and document it.
If you have specific questions, please go ahead.
Shortest summary:
FASTCALL replaces (args: tuple, kwargs: optional dict) with (args: C array, nargs: int, kwnames: tuple of keyword keys). It's a new calling convention which allows to avoid a temporary tuple to pass positional arguments and avoids temporary dictionary to pass keyworkd arguments.
To use FASTCALL, C functions should be converted to the new METH_FASTCALL calling convention
PyObject_Call() can replaced with _PyObject_FastCallKeywords() or _PyObject_FastCallDict() (when we still get kwargs as a dict) in such conversion
Many existing C functions were optimized internally to use FASCALL, so even if you don't modify your code, you will benefit of it (speedup). Typical example: PyFunction_CallFunctionObjArgs().
The most massive change were purely internal and don't affect the most famous C APIs at all. In some cases, to fully benefit of FASTCALL, code should be modified. I'm trying to restrict such changes to Python internals, especially the most used functions.
I expected that the required changes were straightforward enough, it looks like I was wrong, but I don't recall anyone, before you recently, asking for an explanation.
- It was no long ago that you fought tooth-and-nail against a single line patch optimization I submitted. The code was clearly correct and had a simple disassembly to prove its benefit. Your opposition was based on "it increases the complexity of the code, introduces a maintenance cost, and increases the risk of bugs". In the end, your opposition killed the patch. But now, the AC and FASTCALL patches don't seem to mind any of these considerations.
Context: http://bugs.python.org/issue26201
It seems like we need more _explicit_ rules to decide if an optimization is worth it or not. For me, the de facto standard request for an optimization is to prove it with a benchmark. I requested a benchmark, but you refused to provide it.
So I ran my own benchmark and saw that your change made the modified code (PyList_Append()) 6% slower. I'm not sure that my bencmark was correct, but it was a first step to take a decision.
To come back to FASTCALL, your point is that it doesn't provide any speedup.
In most FASTCALL issues that I opened, I provide a script to reproduce my benchmark and the benchmark results. The speedup is usually betwen 10% and 20% faster.
Should I understand that 6% slower is ok, whereas 10-20% faster is not good? Can you please elaborate?
- AC is supposed to be a CPython-only concept. But along the way APIs are being changed without discussion. I don't mind that sorted() now exposes *iterable* as a keyword argument, but it was originally left out on purpose (Tim opined that code would look worse with iterable as a keyword argument). That decision was reversed unilaterally without consulting the author and without a test. Also as AC is being applied, the variable names are being changed. I never really liked the "mp" that used in dicts and prefer the use of "self" better, but it is a gratuitous change that unilaterally reverses the decisions of the authors and makes the code not match any of the surrounding code that uses the prior conventions.
Ah, at least I concur with you on one point :-) Changes to convert functions to AC must not change the API (type of arguments: positional only/keyword/..., default values, etc.) nor provide a worse docstring.
There is an active on-going work to enhance AC to fix issues that you reported, like the default value of positional-only parameters which should not be rendered in the function signature (I created the issue #29299 with a patch). Serhiy is also working on implementing the last major missing feature of AC: support *args and **kwargs parameters (issue #20291).
FYI I wasn't involved in AC changes, I only started to look at AC recently (1 or 2 months ago). Again, I agree that these changes should be carefully reviewed, which is an hard task since required changes are usually large and move a lot of code. We need more eyes to look at these changes!
For the specific case of sorted(), the name of first parameter is already documented in the docstring and documentation in Python 2.7: "iterable". So I guess that you mean that it is now possible to use it as a keyword argument. Well, see the issue #29327 for the long story. This issue is a regression, it was already fixed, and I didn't introduce the API change.
Oh by the way, when I read your comment, I understand that I'm responsible of all regressions. It's true that I introduced regressions, that's where I said "shit happens" (or more politically correct: "it's better to ask forgiveness than permission" ;-)). Since I'm one of the most active contributor in CPython, I'm not surprised of being the one who introduce many (most?) regressions :-) I'm trying to review my changes multiple times, test corner cases, etc. But I'm not perfect.
Sadly, to show its full power, FASTCALL requires changes at many levels of the code. It requires to change at lot of code, but I understood that core developers approved the whole project. Maybe I was wrong? At least, I asked for permissions multiple changes, especially at the start.
- FWIW, the claim that the help is much better is specious. AFAICT, there has never been the slightest problem with "sorted(iterable, key=None, reverse=False) --> new sorted list" which has been clear since the day it was released. It is some of the new strings the are causing problems with users (my students frequently are tripped-up by the / notation for example; no one seems to be able to intuit what it means without it being explained first).
Good news, it seems like you have a good experience in API design, documentation, etc. Join the "Argument Clinic" project to help us to enhance docstrings, function signatures and documentation ;-)
See the good part of the AC on-going work: it's a nice opportunity to also enhance documentation, not only provide a signature.
By the way, to be honest, the main advantage of converting functions to AC is to get a signature. The signature is visible in docstrings which is nice, but it is also very useful to a wide range of tools like (IDE, static checks, etc.).
Conversion to FASTCALL is more a nice effect. At least, it is a good motivation for me to convert mor and more code to AC :-)
AC moves docstring closer to the list of parameters. IHMO it makes the C code simpler to read and understand. It also removes the boring code responsible to "parse" arguments, so it makes the code shorter. But well, this is just my opinion.
- FWIW, I'm trying to be constructive and contribute where I can, but frankly I can't keep up with the volume of churn. Having seen bugs being introduced, it is not inappropriate to ask another dev to please be careful, especially when that dev has been prolific to an unprecedented degree and altering core parts of the language for function calls, to new opcodes, the memory allocators, etc. Very few people on the planet are competent to review these changes, make reasonable assessments about whether the complexity and churn are worth it. An fewer still have the time to keep up with the volume of changes.
Hum, I wasn't involved in bytecode changes.
Well, I reviewed the very good work of Demur Rumed. I recall that you worked on a similar area, trying to fetch bytecode by 16-bit instead of 8-bit. Demur proposed a good design and I recall that the design was approved.
I helped a little bit on the implementation and I pushed the final change, but all credits go to Demur and Serhiy Storshaka! By the way, Serhiy made further efficient enhancements in the bytecode of CALL_FUNCTION instructions.
About memory allocations, I guess that you are referring to my change on PyMem_Malloc() allocator. I discussed the issue on python-dev and waited for approval of my peers before pushing anything, since I know well that it's a critical part of Python: https://mail.python.org/pipermail/python-dev/2016-March/143467.html
I provide all data requested by Marc Andre Lemburg (test the change with common projects, Django, Pillow, numpy) and made further changes (PYTHONMALLOC=debug tool) to help to handle this backward incompatible change (GIL is now required to call PyMem_Malloc).
Hopefully, it seems like nobody noticed this subtle change (GIL now requied): I didn't see any bug report. By the way, I fixed a misused PyMem_Mem() in numpy.
- Please do continue your efforts to improve the language, but also please moderate the rate of change, mitigate the addition complexity, value stability over micro-optimizations, consult the authors and maintainers of code, take special care without code that hasn't been reviewed because that lacks a safety net, and remember that newer devs may be taking cues from you (do you want them making extensive changes to long existing stable code without consulting the authors and with weak LGTM reviews?)
Ok, I will do it.
Thank you for you feedback Raymond. I hope that my email helps you to understand how I work and how I take my decisions.
Victor
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On Jan 22, 2017, at 12:22 PM, Brian Curtin wrote:
I've been on the sidelines for a while myself for a number of reasons, but the shift to GitHub will pull me back in for sure, at least in terms of code review. I look forward to actually contributing code again soon, but easier tooling on reviews—rather, a shiny new one, as I'm aware of Reitveld—is enough of a carrot to bring me back in.
I feel exactly the same way. I'm very excited about the move to git and GitHub and look forward to ramping my contributions back up. Thank you Brett and everyone else working so hard to make this as smooth and timely a transition as possible.
Cheers, -Barry
On 22 January 2017 at 21:59, Barry Warsaw barry@python.org wrote:
On Jan 22, 2017, at 12:22 PM, Brian Curtin wrote:
I've been on the sidelines for a while myself for a number of reasons, but the shift to GitHub will pull me back in for sure, at least in terms of code review. I look forward to actually contributing code again soon, but easier tooling on reviews—rather, a shiny new one, as I'm aware of Reitveld—is enough of a carrot to bring me back in.
I feel exactly the same way. I'm very excited about the move to git and GitHub and look forward to ramping my contributions back up. Thank you Brett and everyone else working so hard to make this as smooth and timely a transition as possible.
One question (and apologies if this has been discussed on another list somewhere) - my biggest bottleneck is the sheer number of python-bugs emails, and the difficulty of identifying ones I can contribute. Will we be moving to the github issue tracker, and/or are there any likely changes to more closely classify issues (ideally in a way that can be handled via mail filters)? Specifically, I'm interested in being able to restrict issue traffic by:
- Pure Python, C, or "not code" (docs, etc).
- Windows/Unix
- Relevant stdlib module
(Or at least have some means of scanning issue emails to quickly spot which ones fall into which classification).
That's a long way beyond simply "switching to github which is a workflow I'm more familiar with" and while I hope github will help me to contribute more, I do think that ultimately the issue is simply that Python is a large and complex system, and people like me have limited time - and too much of it gets wasted playing "spot something I can work on", but that's inherent in the nature of a system this size.
Paul
PS I know there's searches and labels. But the "push" nature of email has its own benefits for me, so there's still a trade off there.
On Jan 22, 2017, at 10:26 PM, Paul Moore wrote:
Will we be moving to the github issue tracker
No. I think a lot of us still prefer Roundup and a significant amount of work is being done to integrate our Roundup instance with GH.
(Or at least have some means of scanning issue emails to quickly spot which ones fall into which classification).
I love Roundup's nosy feature, so I subscribe to the new-bugs-announce list, and yes, you see the first message of any new bug, but I only nosy myself to the issues I know I care about right then (it's always easy to nosy in on a message later).
That's a long way beyond simply "switching to github which is a workflow I'm more familiar with" and while I hope github will help me to contribute more, I do think that ultimately the issue is simply that Python is a large and complex system, and people like me have limited time - and too much of it gets wasted playing "spot something I can work on", but that's inherent in the nature of a system this size.
I think that's true. I plan to start with issues I've already filed or nosied on.
Cheers, -Barry
On Sun, 22 Jan 2017 at 14:26 Paul Moore p.f.moore@gmail.com wrote:
On 22 January 2017 at 21:59, Barry Warsaw barry@python.org wrote:
On Jan 22, 2017, at 12:22 PM, Brian Curtin wrote:
I've been on the sidelines for a while myself for a number of reasons, but the shift to GitHub will pull me back in for sure, at least in terms of code review. I look forward to actually contributing code again soon, but easier tooling on reviews—rather, a shiny new one, as I'm aware of Reitveld—is enough of a carrot to bring me back in.
I feel exactly the same way. I'm very excited about the move to git and GitHub and look forward to ramping my contributions back up. Thank you Brett and everyone else working so hard to make this as smooth and timely a transition as possible.
One question (and apologies if this has been discussed on another list somewhere) - my biggest bottleneck is the sheer number of python-bugs emails, and the difficulty of identifying ones I can contribute. Will we be moving to the github issue tracker, and/or are there any likely changes to more closely classify issues (ideally in a way that can be handled via mail filters)?
As Barry pointed out we are not moving the issue tracker. I thought about it but I instantly got pushback on the idea and I only have so much time and patience. :)
Specifically, I'm interested in being able to restrict issue traffic by:
- Pure Python, C, or "not code" (docs, etc).
- Windows/Unix
- Relevant stdlib module
Do you want this to search issues or PRs by? Since the migration has not happened yet there isn't any emerged practice yet of what will be labeled on PRs and what will be kept on bpo (e.g. we will more than likely label what versions of Python a PR should be applied to, but should we also add the type of labels you mention above?).
Someone could also write a bot to help with this, e.g. automatically add people to a PR for a review if a module mentioned in https://cpython-devguide.readthedocs.io/en/latest/experts.html#stdlib is a part of the PR.
(Or at least have some means of scanning issue emails to quickly spot which ones fall into which classification).
As Barry said, you can always follow new-bugs-announce or have a saved search on bpo that sorts open issues by creation date and you check that regularly (I do the latter and look at issues created the day previously and just glance at their titles to decide if I should have a look).
That's a long way beyond simply "switching to github which is a workflow I'm more familiar with" and while I hope github will help me to contribute more, I do think that ultimately the issue is simply that Python is a large and complex system, and people like me have limited time - and too much of it gets wasted playing "spot something I can work on", but that's inherent in the nature of a system this size.
Sure, but there are also things we can try to do to minimize the burden (and anything that can be automated is best :) .
-Brett
Paul
PS I know there's searches and labels. But the "push" nature of email has its own benefits for me, so there's still a trade off there.
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On 23 January 2017 at 19:31, Brett Cannon brett@python.org wrote:
Do you want this to search issues or PRs by? Since the migration has not happened yet there isn't any emerged practice yet of what will be labeled on PRs and what will be kept on bpo (e.g. we will more than likely label what versions of Python a PR should be applied to, but should we also add the type of labels you mention above?).
Hmm, issues and PRs on separate trackers? That might be interesting... (Although I'm sure you've thought it through and it'll be fine).
I think the real issue here is that I really don't work well with systems where I have to go and look for stuff (I get distracted, or don't bother). Email is a huge win for me because I'm always looking at it, so things arriving by email get my attention. But of course, the converse of that is that too *much* traffic swamps me and I just start ignoring that folder (which is basically what happens with "Python bugs" and "Python checkins"). So I need to manage that, and bpo traffic is far away the highest-traffic thing I receive, so I haven't really evolved strategies for dealing well with that level of traffic.
Someone could also write a bot to help with this, e.g. automatically add people to a PR for a review if a module mentioned in https://cpython-devguide.readthedocs.io/en/latest/experts.html#stdlib is a part of the PR.
If PRs will come on github, I guess that as a member of the python-dev group I'll get the emails by default (like with pip, or other projects I contribute to). I'd probably just start by seeing how well skimming those emails would work (I imagine traffic on actual PRs would be notably less than on bugs as a whole).
The trouble is, it's not really the experts list that matters to me. I get Windows issues from bpo at the moment, and while I'm interested in most of them, I don't often have much to contribute in terms of actual code reviews (because they tend to be C issues). I've no idea what facilities github has for anything in between "get everything" and "get nothing except what I subscribe to explicitly" as I've never yet needed to use anything but the former. And while a custom bot might be interesting, it's not going to pick up the sorts of things I get by skimming stuff.
As Barry said, you can always follow new-bugs-announce or have a saved search on bpo that sorts open issues by creation date and you check that regularly (I do the latter and look at issues created the day previously and just glance at their titles to decide if I should have a look).
new-bugs-announce might be a better list for me than the full bpo stream. I might try that. IIRC, I joined the bpo and python checkins lists because the "guidelines for new core devs" said I should. Maybe there should be a qualification in there that the traffic is high, and if you have limited time, lower-traffic options might be better (or maybe there is and I ignored it :-))?
Paul
On Mon, 23 Jan 2017 at 11:50 Paul Moore p.f.moore@gmail.com wrote:
On 23 January 2017 at 19:31, Brett Cannon brett@python.org wrote:
Do you want this to search issues or PRs by? Since the migration has not happened yet there isn't any emerged practice yet of what will be labeled on PRs and what will be kept on bpo (e.g. we will more than likely label what versions of Python a PR should be applied to, but should we also add the type of labels you mention above?).
Hmm, issues and PRs on separate trackers? That might be interesting... (Although I'm sure you've thought it through and it'll be fine).
You can look at the test issue at https://bugs.python.org/issue2771 to see what's there so far (specifically the Pull Requests section and the latest PR listed there).
I think the real issue here is that I really don't work well with systems where I have to go and look for stuff (I get distracted, or don't bother). Email is a huge win for me because I'm always looking at it, so things arriving by email get my attention. But of course, the converse of that is that too *much* traffic swamps me and I just start ignoring that folder (which is basically what happens with "Python bugs" and "Python checkins"). So I need to manage that, and bpo traffic is far away the highest-traffic thing I receive, so I haven't really evolved strategies for dealing well with that level of traffic.
Someone could also write a bot to help with this, e.g. automatically add people to a PR for a review if a module mentioned in https://cpython-devguide.readthedocs.io/en/latest/experts.html#stdlib is a part of the PR.
If PRs will come on github, I guess that as a member of the python-dev group I'll get the emails by default (like with pip, or other projects I contribute to).
You will need to have email notifications turned on with GitHub and watch the cpython repository once we migrate. I think that will be enough if you want it through GitHub.
I'd probably just start by seeing how well skimming those emails would work (I imagine traffic on actual PRs would be notably less than on bugs as a whole).
The trouble is, it's not really the experts list that matters to me. I get Windows issues from bpo at the moment, and while I'm interested in most of them, I don't often have much to contribute in terms of actual code reviews (because they tend to be C issues). I've no idea what facilities github has for anything in between "get everything" and "get nothing except what I subscribe to explicitly" as I've never yet needed to use anything but the former. And while a custom bot might be interesting, it's not going to pick up the sorts of things I get by skimming stuff.
You could try training a deep neural network to pick up on the things you care about <half-wink>.
As Barry said, you can always follow new-bugs-announce or have a saved search on bpo that sorts open issues by creation date and you check that regularly (I do the latter and look at issues created the day previously and just glance at their titles to decide if I should have a look).
new-bugs-announce might be a better list for me than the full bpo stream. I might try that. IIRC, I joined the bpo and python checkins lists because the "guidelines for new core devs" said I should. Maybe there should be a qualification in there that the traffic is high, and if you have limited time, lower-traffic options might be better (or maybe there is and I ignored it :-))?
It actually says you can choose: https://cpython-devguide.readthedocs.io/en/latest/coredev.html?highlight=new...
-Brett
Paul
On 22 Jan 2017 11:26 pm, "Paul Moore" p.f.moore@gmail.com wrote:
One question (and apologies if this has been discussed on another list somewhere) - my biggest bottleneck is the sheer number of python-bugs emails, and the difficulty of identifying ones I can contribute. Will we be moving to the github issue tracker, and/or are there any likely changes to more closely classify issues (ideally in a way that can be handled via mail filters)?
The experts index in the developer guide is excellent for this - I *don't* follow every new issue that gets filed, but I pay close attention to the ones where I get added to the nosy list, and that's usually via the experts index module and topic entries. It also gives me an incentive to keep my entries in that file up to date (e.g. I was following tempfile when doing a lot of filesystem manipulation tasks at work, but stopped doing so quite some time ago).
We have some docs in the triager section of the guide about that, but perhaps it would help to emphasise it more in the core dev section?
The move to GitHub will also make it easier to follow new PRs, independently of the issue tracker.
Cheers, Nick.
On Mon, 23 Jan 2017 at 22:42 Nick Coghlan ncoghlan@gmail.com wrote:
On 22 Jan 2017 11:26 pm, "Paul Moore" p.f.moore@gmail.com wrote:
One question (and apologies if this has been discussed on another list somewhere) - my biggest bottleneck is the sheer number of python-bugs emails, and the difficulty of identifying ones I can contribute. Will we be moving to the github issue tracker, and/or are there any likely changes to more closely classify issues (ideally in a way that can be handled via mail filters)?
The experts index in the developer guide is excellent for this - I *don't* follow every new issue that gets filed, but I pay close attention to the ones where I get added to the nosy list, and that's usually via the experts index module and topic entries. It also gives me an incentive to keep my entries in that file up to date (e.g. I was following tempfile when doing a lot of filesystem manipulation tasks at work, but stopped doing so quite some time ago).
We have some docs in the triager section of the guide about that, but perhaps it would help to emphasise it more in the core dev section?
The move to GitHub will also make it easier to follow new PRs, independently of the issue tracker.
And with the appropriate mapping we could set up a bot to automatically add people as reviewers or subscribe to the PR discussion if that's what people want.
Excerpts from Brett Cannon's message of 2017-01-21 19:51:48 +0000:
What I'm picking up from this is (as a gross oversimplification):
- Victor _wants_ code reviews
- Raymond thinks we _need_ code reviews
So the common theme here regardless of whether you agree with Raymond or Victor's approach to development is that we are not getting enough code reviews to go around. To me that's what the systemic issue is that this email is bringing up.
Now I think most of us don't think the solution to the lack of reviews is to lower our standard of what it takes to become a core developer (this doesn't mean we shouldn't do a better job of identifying potential candidates, just that we shouldn't give people commit privileges after a single patch like some projects do). To me that means we need to address why out of 79 core developers only 39 have a single commit over the past year, 30/79 have more than 12 commits over that same time scale, 15/79 people have more than 52 commits, and 2/79 people have over 365 commits ( https://github.com/python/cpython/graphs/contributors?from=2016-01-22&to=2017-01-21&type=c for the stats).
Some of you have said you're waiting for the GitHub migration before you start contributing again, which I can understand (I'm going to be sending an email with an update on that after this email to python-dev & core-workflow). But for those that have not told me that I don't know what it will take to get you involved again. For instance, not to pick on Andrew but he hasn't committed anything but he obviously still cares about the project. So what would it take to get Andrew to help review patches again so that the next time something involving random comes through he feels like taking a quick look?
As I have said before, the reason I took on the GitHub migration is for us core developers. I want our workflow to be as easy as possible so that we can be as productive as possible. But the unspoken goal I have long-term is to get to the point that even dormant core devs want to contribute again, and to the point that everyone reviews a patch/month and more people reviewing a patch/week (although I'll take a patch/year to start). I want to get to the point that every person with commit privileges takes 30 minutes a month to help with reviews and that the majority of folks take 30 minutes a week to review (and please don't think this as a hard rule and if you don't the privileges go away, view this as an aspirational goal). Even if people who don't have time to review the kind of patches Victor is producing which triggered this thread, reviewing documentation patches can be done without deep knowledge of things and without taking much time. That way people who have time to review the bigger, more difficult patches can actually spend their time on those reviews and not worrying about patches fixing a spelling mistake or adding a new test to raise test coverage.
All of this is so that I hope one day we get to the point where all patches require a review no matter who proposed the code change. Now I think we're quite a ways of from being there, but that's my moonshot goal for our workflow: that we have enough quality reviews coming in that we feel that even patches from fellow core developers is worth requiring the extra code check and disbursement of knowledge without feeling like a terrible drag on productivity.
Once the GitHub migration has occurred I'm planning to tackle our Misc/NEWS problem and then automate Misc/ACKS. After that, though, I hope we can take
I would be happy to help with both of those tasks. I have experience with both within the OpenStack project.
And put me on the list of "waiting for github" contributors. I should have more time freeing up in a couple of months as I change some of my responsibilities at work. I would like to help with the migration and eventually regular reviews.
Doug
the time to have a hard look at what in our workflow prevents people from making even occasional code reviews so that everyone wants to help out again (and if any of this interests you then please subscribe to core-workflow).
On Fri, 20 Jan 2017 at 02:46 Victor Stinner victor.stinner@gmail.com wrote:
Hi,
Raymond Hettinger used a regression that I introduced in the builtin sorted() function (in Python 3.6.0) to give me his feedback on my FASTCALL work, but also on Argument Clinic.
Context: http://bugs.python.org/issue29327#msg285848
Since the reported issues is wider than just FASTCALL, including how I contribute to CPython, I decided to discuss the topic with a wider audience. I continue the discussion on python-committers to get the opinion of the other core developers.
Sorry for my very long answer! I tried to answer to each issues reported by Raymond.
Inaccurate summary: I'm a strong supporter of "it's better to ask forgiveness than permission", whereas Raymond considers that I introduced too many regressions with my workflow.
Raymond Hettinger added the comment:
A few random thoughts that may or may not be helpful:
- We now have two seasoned developers and one new core developer that
collectively are creating many non-trivial patches to core parts of Python at an unprecedented rate of change. The patches are coming in much faster than they can reasonably be reviewed and carefully considered, especially by devs such as myself who have very limited time available. IMO, taken as whole, these changes are destabilizing the language. Python is so successful and widely adopted that we can't afford a "shit happens" attitude. Perhaps that works in corners of the language, infrequently used modules, but it makes less sense when touching the critical paths that have had slow and careful evolution over 26 years. > > * Besides the volume of patches, one other reason that reviews are hard to come by is that they apply new APIs that I don't fully understand yet. There are perhaps two people on the planet who could currently give thoughtful, correct, and critical evaluation of all those patches. Everyone else is just watching them all fly by and hoping that something good is happening.
Since one or maybe even two years, I noticed that many of my issues were blocked by the lack of reviews. As you wrote, only few developer have the knowledge and background to be able to provide a good review (not only "tests pass, so LGTM") on my changes modifying the Python core.
I also wanted to discuss this topic, but I didn't really know what to propose. Let's take this opportunity to explain how I contribute to CPython, especially how I decide to wait for a review or not.
For each patch that I write, I estimate the risk of regression. You may know that any regression is something unexpected, so such estimation is tricky. Here is my heuristic:
(*) if the patch is trivial (short, non controversal), I push it immediatly.
(*) If I'm less confident, I open an issue and attach the patch. I wait at least one day before pushing.
It's strange, but the process of opening an issue and attaching the patch usually helps to review the code myself (find bugs, or more generally enhance the patch). Maybe because it forces me to review the change one more time?
If the change is not part of a larger patch serie, so doesn't block me to move further, I try to keep the issue open around one week.
The truth is that too few of my patches get a review :-/ Maybe I should wait longer, but then it becomes harder for me to handle many patches.
Maybe it's a tooling issues. Recently, I started to use local branches in a Git repository. It helps a lot of work on parallel on large changes. Before, I only worked in a single directory (default/, the default Mercurial branch) and applied/reverted patches everytime. It's painful, especially when I have to use multiple computers, download again publshed patches, etc. Maybe it will run smoother once CPython will move to Git and GitHub.
By the way, it's painful to squash a long patch serie into a giant patch, much harder to review, where changes don't make sense at all at the first look. Again, a better reviewing tool supporting patch series (GitHub) will help here too.
Not supporting patch series in our reviewing tool also explains why I prefer to push than having to wait for a review. Rebasing manually long patch series stored as giant .patch files is complicated.
(*) If the change changes an API or changes a core component, I wait for at least one review from a core reviewer. Sometimes, I even send an email to python-dev. Again, sometimes I don't get any feedback on the patch nor the email after two weeks :-/ At least, I tried :-) Usually, I get feedback in less than one week, or no feedback at all. I understand that nobody understands my change or nobody cares :-)
I totally understand that most core developers have a little amount of time available to contribute to Python. I'm trying to find a compromise between the risk of introducing regressions and being stuck in my work. This email might help me to adjust my workflow.
By the way, I'm trying to always run the full test suite (./python -m test -rW -j0) before pushing any change. If I suspect that I may have introduced reference leaks, I also run "./python -m test -R 3:3 ..." on the tests related to the modified code to check for memory/reference leaks.
- One other reason for the lack of review comments in the enthusiasm and fervor surrounding the patches. I feel like there is a cost of questioning whether the patches should be done or how they are done, like I am burning little karma every time. Sometimes it feels safest and most cordial to just say nothing and let you make hundreds of semi-reviewed changes to just about every critical part of the language.
"semi-reviewed". Let me be more accurate: yeah, I do push a lot of changes which were not reviewed by anyone (see above).
- Historically, if there was creator or maintainer of the code who was still active, that person would always be consulted and have a final say on whether a change should be applied. Now, we have code constantly being changed without consulting the original author (for example, the recent and catastrophic random initialization bug was due to application of a patch without consulting the author of _randommodule.c and the maintainer of random.py, or this change to sorted(), or the changes to decimal, etc).
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
My problem is that I'm not aware of any explicit list of maintainers. I didn't know that you were the maintainer of the random module before you told me that at the Facebook sprint last september. I didn't expect that the random module had a maintainer, I thought that any core developer would be allowed to modify the code.
Moreover, since I open an issue for most of my changes, it gives an opportunity to maintainers to review changes. Maybe we need more components in the bug tracker to notify maintainers of pending changes?
You mentionned 3 different changes, let me reply.
(1) The random change: http://bugs.python.org/issue29085
I introduced a regression in random.Random.seed(): a typo in the C code has the consequence that the current time and process identifier is used, instead of os.urandom(16), to initialize the Mersenne Twister RNG.
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
Since we are talking about the development process, let me see how the change was made.
Context: The PEP 524 has a long and painful history... Something like more than 500 messages were sent on the bug tracker and python-dev, and nobody was listening to each others, two security experts "rage-quitted" Python because of this mess... I decided to try to fix this issue in a constructive way, so I wrote a PEP. Nick wrote a different PEP, since it was clear that it was possible to handle security in two different incompatible ways. A mailing list was even created just to discuss this bug! A mailing list just for a bug gives an idea of the size of the mess :-)
Well, about the change itself, it was done in http://bugs.python.org/issue27776
The patch was available for review during 19 days (2016-08-18-2016-09-06) and was reviewed by Nick Coghlan. Since Nick wrote a similar PEP, I trusted him to be able to review my change. (Well, anyway I already trust all core developers, but I mean that I was trusting him even more than usual :-))
Since the change has a big impact on security, I had prefer to get a review of more developers, especially our security experts... but as I wrote, two security experts "rage- quitted". Again, this PEP has a long and sad story :-/
Note: you say that you are the maintainer of the random module, but I don't recall having see you in any recent discussions and issues related to os.urandom(), whereas a lot of enhancements and changes were done last 2 years. I made many changes to support new OS functions like getentropy() an getrandom().
Oooookay, let's see the second change, "this change to sorted()", http://bugs.python.org/issue29327
(2) I introduced a bug in sorted(), last August: https://hg.python.org/cpython/rev/15eab21bf934/
Calling sorted(iterable=[]) does crash. To be honest, I didn't imagine that anyone would pass the iterable by keyword, but Serhiy is very good to spot bugs in corner cases :-)
IMHO the regression is subtle.
When I optimized the code to use FASTCALL, I replaced PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1). I checked that all tests passed, so it looks ok to me.
I didn't imagine that anyone would call sorted(iterable=[]), so I didn't notice that PyTuple_GetSlice() can create an empty tuple.
The previous code was wrong since sorted() accepted iterable as a keyword, whereas sort.list() doesn't.
So well, I let you guess if a review would have spot this bug in the large change.
(3) Recently, I ran sed to replace code patterns to use faster ways to call functions: https://hg.python.org/cpython/rev/54a89144ee1d
"Replace PyObject_CallObject(callable, NULL) with _PyObject_CallNoArg(callable)"
I recalled that I modified the _decimal module and that Stefan Krah complained, because he wants to have the same code base on Python 3.5, 3.6 and 3.7. He also mentionned an external test suite which was broken by recent _decimal changes (not sure if my specific change was in cause or not), but I wasn't aware of it.
To be honest, I didn't even notice that I modified _decimal when I ran sed on all .c files. Since the change was straightforward and (IMHO) made the code more readable, I didn't even wait for a review if I recall correctly.
Stefan and me handled this issue privately (he reverted my change), I'm not sure that it's worth it to say more about this "issue" (or even "non-issue").
To be clear, I don't consider that my change introduced a regression.
In general, Guido has been opposed to sweeping changes across the code base for only tiny benefits. Of late, that rule seems to have been lost.
The benefits of FASTCALL mainly apply to fine grained functions which only do a little work and tend to be called frequently in loops. For functions such as sorted(), the calling overhead is dominated by the cost of actually doing the sort. For sorted(), FASTCALL is truly irrelevant and likely wasn't worth the complexity, or the actual bug, or any of the time we've now put in it. There was no actual problem being solved, just a desire to broadly apply new optimizations.
Ok, first, you qualify my FASTCALL changes as code churn. So let me show an example with sorted(): https://hg.python.org/cpython/rev/b34d2ef5c412
Can you elaborate how such change increases the complexity?
Second, "no actual problem being solved"
Since the goal of FASTCALL is to optimize Python, I guess that you consider that the speedup doesn't justify the change. I gave numbers in the issue #29327:
Microbenchmark on sorted() on Python 3.7 compared to 3.5 (before FASTCALL):
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10))' 'sorted(seq)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 1.07 us +- 0.06 us -> [3.7] 958 ns +- 15 ns: 1.12x faster (-11%)
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10)); k=lambda x:x' 'sorted(seq, key=k)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 3.34 us +- 0.07 us -> [3.7] 2.66 us +- 0.05 us: 1.26x faster (-21%)
IMHO such speedup is significant even on a microbenchmark. Can you elaborate what are your criteria to decide if an optimization is worth it?
- Historically, we've relied on core developers showing restraint. Not every idea that pops into their head is immediately turned into a patch accompanied by pressure to apply it. Devs tended to restrict themselves to parts of the code they knew best through long and careful study rather sweeping through modules and altering other people's carefully crafted code.
Should I understand that I should restrict myself to some files? Or not touch some specific parts of Python, like... "your" code like random, itertools and collections modules?
I replied to the 3 issues you mentioned previously and explained how I contribute to Python.
FWIW, I applaud your efforts to reduce call overhead -- that has long been a sore spot for the language.
Guido has long opposed optimizations that increase risk of bugs, introduce complexity, or that affect long-term maintainability. In some places, it looks like FASTCALL is increasing the complexity (replacing something simple and well-understood with a wordier, more intricate API that I don't yet fully understand and will affect my ability to maintain the surrounding code).
I'm sorry, I didn't spent much time on explaing the FASTCALL design nor documenting my changes. It's partially deliberate to make everything related to FASTCALL private. Since it's a huge project modifying a lot of code, I wanted to wait until the APIs and the code stop moving too fast to take time to explain my work and document it.
If you have specific questions, please go ahead.
Shortest summary:
FASTCALL replaces (args: tuple, kwargs: optional dict) with (args: C array, nargs: int, kwnames: tuple of keyword keys). It's a new calling convention which allows to avoid a temporary tuple to pass positional arguments and avoids temporary dictionary to pass keyworkd arguments.
To use FASTCALL, C functions should be converted to the new METH_FASTCALL calling convention
PyObject_Call() can replaced with _PyObject_FastCallKeywords() or _PyObject_FastCallDict() (when we still get kwargs as a dict) in such conversion
Many existing C functions were optimized internally to use FASCALL, so even if you don't modify your code, you will benefit of it (speedup). Typical example: PyFunction_CallFunctionObjArgs().
The most massive change were purely internal and don't affect the most famous C APIs at all. In some cases, to fully benefit of FASTCALL, code should be modified. I'm trying to restrict such changes to Python internals, especially the most used functions.
I expected that the required changes were straightforward enough, it looks like I was wrong, but I don't recall anyone, before you recently, asking for an explanation.
- It was no long ago that you fought tooth-and-nail against a single line patch optimization I submitted. The code was clearly correct and had a simple disassembly to prove its benefit. Your opposition was based on "it increases the complexity of the code, introduces a maintenance cost, and increases the risk of bugs". In the end, your opposition killed the patch. But now, the AC and FASTCALL patches don't seem to mind any of these considerations.
Context: http://bugs.python.org/issue26201
It seems like we need more _explicit_ rules to decide if an optimization is worth it or not. For me, the de facto standard request for an optimization is to prove it with a benchmark. I requested a benchmark, but you refused to provide it.
So I ran my own benchmark and saw that your change made the modified code (PyList_Append()) 6% slower. I'm not sure that my bencmark was correct, but it was a first step to take a decision.
To come back to FASTCALL, your point is that it doesn't provide any speedup.
In most FASTCALL issues that I opened, I provide a script to reproduce my benchmark and the benchmark results. The speedup is usually betwen 10% and 20% faster.
Should I understand that 6% slower is ok, whereas 10-20% faster is not good? Can you please elaborate?
- AC is supposed to be a CPython-only concept. But along the way APIs are being changed without discussion. I don't mind that sorted() now exposes *iterable* as a keyword argument, but it was originally left out on purpose (Tim opined that code would look worse with iterable as a keyword argument). That decision was reversed unilaterally without consulting the author and without a test. Also as AC is being applied, the variable names are being changed. I never really liked the "mp" that used in dicts and prefer the use of "self" better, but it is a gratuitous change that unilaterally reverses the decisions of the authors and makes the code not match any of the surrounding code that uses the prior conventions.
Ah, at least I concur with you on one point :-) Changes to convert functions to AC must not change the API (type of arguments: positional only/keyword/..., default values, etc.) nor provide a worse docstring.
There is an active on-going work to enhance AC to fix issues that you reported, like the default value of positional-only parameters which should not be rendered in the function signature (I created the issue #29299 with a patch). Serhiy is also working on implementing the last major missing feature of AC: support *args and **kwargs parameters (issue #20291).
FYI I wasn't involved in AC changes, I only started to look at AC recently (1 or 2 months ago). Again, I agree that these changes should be carefully reviewed, which is an hard task since required changes are usually large and move a lot of code. We need more eyes to look at these changes!
For the specific case of sorted(), the name of first parameter is already documented in the docstring and documentation in Python 2.7: "iterable". So I guess that you mean that it is now possible to use it as a keyword argument. Well, see the issue #29327 for the long story. This issue is a regression, it was already fixed, and I didn't introduce the API change.
Oh by the way, when I read your comment, I understand that I'm responsible of all regressions. It's true that I introduced regressions, that's where I said "shit happens" (or more politically correct: "it's better to ask forgiveness than permission" ;-)). Since I'm one of the most active contributor in CPython, I'm not surprised of being the one who introduce many (most?) regressions :-) I'm trying to review my changes multiple times, test corner cases, etc. But I'm not perfect.
Sadly, to show its full power, FASTCALL requires changes at many levels of the code. It requires to change at lot of code, but I understood that core developers approved the whole project. Maybe I was wrong? At least, I asked for permissions multiple changes, especially at the start.
- FWIW, the claim that the help is much better is specious. AFAICT, there has never been the slightest problem with "sorted(iterable, key=None, reverse=False) --> new sorted list" which has been clear since the day it was released. It is some of the new strings the are causing problems with users (my students frequently are tripped-up by the / notation for example; no one seems to be able to intuit what it means without it being explained first).
Good news, it seems like you have a good experience in API design, documentation, etc. Join the "Argument Clinic" project to help us to enhance docstrings, function signatures and documentation ;-)
See the good part of the AC on-going work: it's a nice opportunity to also enhance documentation, not only provide a signature.
By the way, to be honest, the main advantage of converting functions to AC is to get a signature. The signature is visible in docstrings which is nice, but it is also very useful to a wide range of tools like (IDE, static checks, etc.).
Conversion to FASTCALL is more a nice effect. At least, it is a good motivation for me to convert mor and more code to AC :-)
AC moves docstring closer to the list of parameters. IHMO it makes the C code simpler to read and understand. It also removes the boring code responsible to "parse" arguments, so it makes the code shorter. But well, this is just my opinion.
- FWIW, I'm trying to be constructive and contribute where I can, but frankly I can't keep up with the volume of churn. Having seen bugs being introduced, it is not inappropriate to ask another dev to please be careful, especially when that dev has been prolific to an unprecedented degree and altering core parts of the language for function calls, to new opcodes, the memory allocators, etc. Very few people on the planet are competent to review these changes, make reasonable assessments about whether the complexity and churn are worth it. An fewer still have the time to keep up with the volume of changes.
Hum, I wasn't involved in bytecode changes.
Well, I reviewed the very good work of Demur Rumed. I recall that you worked on a similar area, trying to fetch bytecode by 16-bit instead of 8-bit. Demur proposed a good design and I recall that the design was approved.
I helped a little bit on the implementation and I pushed the final change, but all credits go to Demur and Serhiy Storshaka! By the way, Serhiy made further efficient enhancements in the bytecode of CALL_FUNCTION instructions.
About memory allocations, I guess that you are referring to my change on PyMem_Malloc() allocator. I discussed the issue on python-dev and waited for approval of my peers before pushing anything, since I know well that it's a critical part of Python: https://mail.python.org/pipermail/python-dev/2016-March/143467.html
I provide all data requested by Marc Andre Lemburg (test the change with common projects, Django, Pillow, numpy) and made further changes (PYTHONMALLOC=debug tool) to help to handle this backward incompatible change (GIL is now required to call PyMem_Malloc).
Hopefully, it seems like nobody noticed this subtle change (GIL now requied): I didn't see any bug report. By the way, I fixed a misused PyMem_Mem() in numpy.
- Please do continue your efforts to improve the language, but also please moderate the rate of change, mitigate the addition complexity, value stability over micro-optimizations, consult the authors and maintainers of code, take special care without code that hasn't been reviewed because that lacks a safety net, and remember that newer devs may be taking cues from you (do you want them making extensive changes to long existing stable code without consulting the authors and with weak LGTM reviews?)
Ok, I will do it.
Thank you for you feedback Raymond. I hope that my email helps you to understand how I work and how I take my decisions.
Victor
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On Mon, 23 Jan 2017 at 11:15 Doug Hellmann doug@doughellmann.com wrote:
Excerpts from Brett Cannon's message of 2017-01-21 19:51:48 +0000:
What I'm picking up from this is (as a gross oversimplification):
- Victor _wants_ code reviews
- Raymond thinks we _need_ code reviews
So the common theme here regardless of whether you agree with Raymond or Victor's approach to development is that we are not getting enough code reviews to go around. To me that's what the systemic issue is that this email is bringing up.
Now I think most of us don't think the solution to the lack of reviews is to lower our standard of what it takes to become a core developer (this doesn't mean we shouldn't do a better job of identifying potential candidates, just that we shouldn't give people commit privileges after a single patch like some projects do). To me that means we need to address why out of 79 core developers only 39 have a single commit over the past year, 30/79 have more than 12 commits over that same time scale, 15/79 people have more than 52 commits, and 2/79 people have over 365 commits (
for the stats).
Some of you have said you're waiting for the GitHub migration before you start contributing again, which I can understand (I'm going to be sending an email with an update on that after this email to python-dev & core-workflow). But for those that have not told me that I don't know what it will take to get you involved again. For instance, not to pick on Andrew but he hasn't committed anything but he obviously still cares about the project. So what would it take to get Andrew to help review patches again so that the next time something involving random comes through he feels like taking a quick look?
As I have said before, the reason I took on the GitHub migration is for us core developers. I want our workflow to be as easy as possible so that we can be as productive as possible. But the unspoken goal I have long-term is to get to the point that even dormant core devs want to contribute again, and to the point that everyone reviews a patch/month and more people reviewing a patch/week (although I'll take a patch/year to start). I want to get to the point that every person with commit privileges takes 30 minutes a month to help with reviews and that the majority of folks take 30 minutes a week to review (and please don't think this as a hard rule and if you don't the privileges go away, view this as an aspirational goal). Even if people who don't have time to review the kind of patches Victor is producing which triggered this thread, reviewing documentation patches can be done without deep knowledge of things and without taking much time. That way people who have time to review the bigger, more difficult patches can actually spend their time on those reviews and not worrying about patches fixing a spelling mistake or adding a new test to raise test coverage.
All of this is so that I hope one day we get to the point where all
https://github.com/python/cpython/graphs/contributors?from=2016-01-22&to=2017-01-21&type=c patches
require a review no matter who proposed the code change. Now I think we're quite a ways of from being there, but that's my moonshot goal for our workflow: that we have enough quality reviews coming in that we feel that even patches from fellow core developers is worth requiring the extra code check and disbursement of knowledge without feeling like a terrible drag on productivity.
Once the GitHub migration has occurred I'm planning to tackle our Misc/NEWS problem and then automate Misc/ACKS. After that, though, I hope we can take
I would be happy to help with both of those tasks. I have experience with both within the OpenStack project.
Great! All of that will be discussed on the core-workflow mailing list and we track ideas at https://github.com/python/core-workflow/issues
And put me on the list of "waiting for github" contributors. I should have more time freeing up in a couple of months as I change some of my responsibilities at work. I would like to help with the migration and eventually regular reviews.
All great to hear!
-Brett
Doug
the time to have a hard look at what in our workflow prevents people from making even occasional code reviews so that everyone wants to help out again (and if any of this interests you then please subscribe to core-workflow).
On Fri, 20 Jan 2017 at 02:46 Victor Stinner victor.stinner@gmail.com wrote:
Hi,
Raymond Hettinger used a regression that I introduced in the builtin sorted() function (in Python 3.6.0) to give me his feedback on my FASTCALL work, but also on Argument Clinic.
Context: http://bugs.python.org/issue29327#msg285848
Since the reported issues is wider than just FASTCALL, including how I contribute to CPython, I decided to discuss the topic with a wider audience. I continue the discussion on python-committers to get the opinion of the other core developers.
Sorry for my very long answer! I tried to answer to each issues reported by Raymond.
Inaccurate summary: I'm a strong supporter of "it's better to ask forgiveness than permission", whereas Raymond considers that I introduced too many regressions with my workflow.
Raymond Hettinger added the comment:
A few random thoughts that may or may not be helpful:
- We now have two seasoned developers and one new core developer that
collectively are creating many non-trivial patches to core parts of Python at an unprecedented rate of change. The patches are coming in much faster than they can reasonably be reviewed and carefully considered, especially by devs such as myself who have very limited time available. IMO, taken as whole, these changes are destabilizing the language. Python is so successful and widely adopted that we can't afford a "shit happens" attitude. Perhaps that works in corners of the language, infrequently used modules, but it makes less sense when touching the critical paths that have had slow and careful evolution over 26 years. > > * Besides the volume of patches, one other reason that reviews are hard to come by is that they apply new APIs that I don't fully understand yet. There are perhaps two people on the planet who could currently give thoughtful, correct, and critical evaluation of all those patches. Everyone else is just watching them all fly by and hoping that something good is happening.
Since one or maybe even two years, I noticed that many of my issues were blocked by the lack of reviews. As you wrote, only few developer have the knowledge and background to be able to provide a good review (not only "tests pass, so LGTM") on my changes modifying the Python core.
I also wanted to discuss this topic, but I didn't really know what to propose. Let's take this opportunity to explain how I contribute to CPython, especially how I decide to wait for a review or not.
For each patch that I write, I estimate the risk of regression. You may know that any regression is something unexpected, so such estimation is tricky. Here is my heuristic:
(*) if the patch is trivial (short, non controversal), I push it immediatly.
(*) If I'm less confident, I open an issue and attach the patch. I wait at least one day before pushing.
It's strange, but the process of opening an issue and attaching the patch usually helps to review the code myself (find bugs, or more generally enhance the patch). Maybe because it forces me to review the change one more time?
If the change is not part of a larger patch serie, so doesn't block me to move further, I try to keep the issue open around one week.
The truth is that too few of my patches get a review :-/ Maybe I should wait longer, but then it becomes harder for me to handle many patches.
Maybe it's a tooling issues. Recently, I started to use local branches in a Git repository. It helps a lot of work on parallel on large changes. Before, I only worked in a single directory (default/, the default Mercurial branch) and applied/reverted patches everytime. It's painful, especially when I have to use multiple computers, download again publshed patches, etc. Maybe it will run smoother once CPython will move to Git and GitHub.
By the way, it's painful to squash a long patch serie into a giant patch, much harder to review, where changes don't make sense at all at the first look. Again, a better reviewing tool supporting patch series (GitHub) will help here too.
Not supporting patch series in our reviewing tool also explains why I prefer to push than having to wait for a review. Rebasing manually long patch series stored as giant .patch files is complicated.
(*) If the change changes an API or changes a core component, I wait for at least one review from a core reviewer. Sometimes, I even send an email to python-dev. Again, sometimes I don't get any feedback on the patch nor the email after two weeks :-/ At least, I tried :-) Usually, I get feedback in less than one week, or no feedback at all. I understand that nobody understands my change or nobody cares :-)
I totally understand that most core developers have a little amount of time available to contribute to Python. I'm trying to find a compromise between the risk of introducing regressions and being stuck in my work. This email might help me to adjust my workflow.
By the way, I'm trying to always run the full test suite (./python -m test -rW -j0) before pushing any change. If I suspect that I may have introduced reference leaks, I also run "./python -m test -R 3:3 ..." on the tests related to the modified code to check for memory/reference leaks.
- One other reason for the lack of review comments in the enthusiasm and fervor surrounding the patches. I feel like there is a cost of questioning whether the patches should be done or how they are done, like I am burning little karma every time. Sometimes it feels safest and most cordial to just say nothing and let you make hundreds of semi-reviewed changes to just about every critical part of the language.
"semi-reviewed". Let me be more accurate: yeah, I do push a lot of changes which were not reviewed by anyone (see above).
- Historically, if there was creator or maintainer of the code who was still active, that person would always be consulted and have a final say on whether a change should be applied. Now, we have code constantly being changed without consulting the original author (for example, the recent and catastrophic random initialization bug was due to application of a patch without consulting the author of _randommodule.c and the maintainer of random.py, or this change to sorted(), or the changes to decimal, etc).
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
My problem is that I'm not aware of any explicit list of maintainers. I didn't know that you were the maintainer of the random module before you told me that at the Facebook sprint last september. I didn't expect that the random module had a maintainer, I thought that any core developer would be allowed to modify the code.
Moreover, since I open an issue for most of my changes, it gives an opportunity to maintainers to review changes. Maybe we need more components in the bug tracker to notify maintainers of pending changes?
You mentionned 3 different changes, let me reply.
(1) The random change: http://bugs.python.org/issue29085
I introduced a regression in random.Random.seed(): a typo in the C code has the consequence that the current time and process identifier is used, instead of os.urandom(16), to initialize the Mersenne Twister RNG.
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
Since we are talking about the development process, let me see how the change was made.
Context: The PEP 524 has a long and painful history... Something like more than 500 messages were sent on the bug tracker and python-dev, and nobody was listening to each others, two security experts "rage-quitted" Python because of this mess... I decided to try to fix this issue in a constructive way, so I wrote a PEP. Nick wrote a different PEP, since it was clear that it was possible to handle security in two different incompatible ways. A mailing list was even created just to discuss this bug! A mailing list just for a bug gives an idea of the size of the mess :-)
Well, about the change itself, it was done in http://bugs.python.org/issue27776
The patch was available for review during 19 days (2016-08-18-2016-09-06) and was reviewed by Nick Coghlan. Since Nick wrote a similar PEP, I trusted him to be able to review my change. (Well, anyway I already trust all core developers, but I mean that I was trusting him even more than usual :-))
Since the change has a big impact on security, I had prefer to get a review of more developers, especially our security experts... but as I wrote, two security experts "rage- quitted". Again, this PEP has a long and sad story :-/
Note: you say that you are the maintainer of the random module, but I don't recall having see you in any recent discussions and issues related to os.urandom(), whereas a lot of enhancements and changes were done last 2 years. I made many changes to support new OS functions like getentropy() an getrandom().
Oooookay, let's see the second change, "this change to sorted()", http://bugs.python.org/issue29327
(2) I introduced a bug in sorted(), last August: https://hg.python.org/cpython/rev/15eab21bf934/
Calling sorted(iterable=[]) does crash. To be honest, I didn't imagine that anyone would pass the iterable by keyword, but Serhiy is very good to spot bugs in corner cases :-)
IMHO the regression is subtle.
When I optimized the code to use FASTCALL, I replaced PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1). I checked that all tests passed, so it looks ok to me.
I didn't imagine that anyone would call sorted(iterable=[]), so I didn't notice that PyTuple_GetSlice() can create an empty tuple.
The previous code was wrong since sorted() accepted iterable as a keyword, whereas sort.list() doesn't.
So well, I let you guess if a review would have spot this bug in the large change.
(3) Recently, I ran sed to replace code patterns to use faster ways to call functions: https://hg.python.org/cpython/rev/54a89144ee1d
"Replace PyObject_CallObject(callable, NULL) with _PyObject_CallNoArg(callable)"
I recalled that I modified the _decimal module and that Stefan Krah complained, because he wants to have the same code base on Python 3.5, 3.6 and 3.7. He also mentionned an external test suite which was broken by recent _decimal changes (not sure if my specific change was in cause or not), but I wasn't aware of it.
To be honest, I didn't even notice that I modified _decimal when I ran sed on all .c files. Since the change was straightforward and (IMHO) made the code more readable, I didn't even wait for a review if I recall correctly.
Stefan and me handled this issue privately (he reverted my change), I'm not sure that it's worth it to say more about this "issue" (or even "non-issue").
To be clear, I don't consider that my change introduced a regression.
In general, Guido has been opposed to sweeping changes across the code base for only tiny benefits. Of late, that rule seems to have been lost.
The benefits of FASTCALL mainly apply to fine grained functions which only do a little work and tend to be called frequently in loops. For functions such as sorted(), the calling overhead is dominated by the cost of actually doing the sort. For sorted(), FASTCALL is truly irrelevant and likely wasn't worth the complexity, or the actual bug, or any of the time we've now put in it. There was no actual problem being solved, just a desire to broadly apply new optimizations.
Ok, first, you qualify my FASTCALL changes as code churn. So let me show an example with sorted(): https://hg.python.org/cpython/rev/b34d2ef5c412
Can you elaborate how such change increases the complexity?
Second, "no actual problem being solved"
Since the goal of FASTCALL is to optimize Python, I guess that you consider that the speedup doesn't justify the change. I gave numbers in the issue #29327:
Microbenchmark on sorted() on Python 3.7 compared to 3.5 (before FASTCALL):
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10))' 'sorted(seq)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 1.07 us +- 0.06 us -> [3.7] 958 ns +- 15 ns: 1.12x faster (-11%)
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10)); k=lambda x:x' 'sorted(seq, key=k)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 3.34 us +- 0.07 us -> [3.7] 2.66 us +- 0.05 us: 1.26x faster (-21%)
IMHO such speedup is significant even on a microbenchmark. Can you elaborate what are your criteria to decide if an optimization is worth it?
- Historically, we've relied on core developers showing restraint. Not every idea that pops into their head is immediately turned into a patch accompanied by pressure to apply it. Devs tended to restrict themselves to parts of the code they knew best through long and careful study rather sweeping through modules and altering other people's carefully crafted code.
Should I understand that I should restrict myself to some files? Or not touch some specific parts of Python, like... "your" code like random, itertools and collections modules?
I replied to the 3 issues you mentioned previously and explained how I contribute to Python.
FWIW, I applaud your efforts to reduce call overhead -- that has long been a sore spot for the language.
Guido has long opposed optimizations that increase risk of bugs, introduce complexity, or that affect long-term maintainability. In some places, it looks like FASTCALL is increasing the complexity (replacing something simple and well-understood with a wordier, more intricate API that I don't yet fully understand and will affect my ability to maintain the surrounding code).
I'm sorry, I didn't spent much time on explaing the FASTCALL design nor documenting my changes. It's partially deliberate to make everything related to FASTCALL private. Since it's a huge project modifying a lot of code, I wanted to wait until the APIs and the code stop moving too fast to take time to explain my work and document it.
If you have specific questions, please go ahead.
Shortest summary:
FASTCALL replaces (args: tuple, kwargs: optional dict) with (args: C array, nargs: int, kwnames: tuple of keyword keys). It's a new calling convention which allows to avoid a temporary tuple to pass positional arguments and avoids temporary dictionary to pass keyworkd arguments.
To use FASTCALL, C functions should be converted to the new METH_FASTCALL calling convention
PyObject_Call() can replaced with _PyObject_FastCallKeywords() or _PyObject_FastCallDict() (when we still get kwargs as a dict) in such conversion
Many existing C functions were optimized internally to use FASCALL, so even if you don't modify your code, you will benefit of it (speedup). Typical example: PyFunction_CallFunctionObjArgs().
The most massive change were purely internal and don't affect the most famous C APIs at all. In some cases, to fully benefit of FASTCALL, code should be modified. I'm trying to restrict such changes to Python internals, especially the most used functions.
I expected that the required changes were straightforward enough, it looks like I was wrong, but I don't recall anyone, before you recently, asking for an explanation.
- It was no long ago that you fought tooth-and-nail against a single line patch optimization I submitted. The code was clearly correct and had a simple disassembly to prove its benefit. Your opposition was based on "it increases the complexity of the code, introduces a maintenance cost, and increases the risk of bugs". In the end, your opposition killed the patch. But now, the AC and FASTCALL patches don't seem to mind any of these considerations.
Context: http://bugs.python.org/issue26201
It seems like we need more _explicit_ rules to decide if an optimization is worth it or not. For me, the de facto standard request for an optimization is to prove it with a benchmark. I requested a benchmark, but you refused to provide it.
So I ran my own benchmark and saw that your change made the modified code (PyList_Append()) 6% slower. I'm not sure that my bencmark was correct, but it was a first step to take a decision.
To come back to FASTCALL, your point is that it doesn't provide any speedup.
In most FASTCALL issues that I opened, I provide a script to reproduce my benchmark and the benchmark results. The speedup is usually betwen 10% and 20% faster.
Should I understand that 6% slower is ok, whereas 10-20% faster is not good? Can you please elaborate?
- AC is supposed to be a CPython-only concept. But along the way APIs are being changed without discussion. I don't mind that sorted() now exposes *iterable* as a keyword argument, but it was originally left out on purpose (Tim opined that code would look worse with iterable as a keyword argument). That decision was reversed unilaterally without consulting the author and without a test. Also as AC is being applied, the variable names are being changed. I never really liked the "mp" that used in dicts and prefer the use of "self" better, but it is a gratuitous change that unilaterally reverses the decisions of the authors and makes the code not match any of the surrounding code that uses the prior conventions.
Ah, at least I concur with you on one point :-) Changes to convert functions to AC must not change the API (type of arguments: positional only/keyword/..., default values, etc.) nor provide a worse docstring.
There is an active on-going work to enhance AC to fix issues that you reported, like the default value of positional-only parameters which should not be rendered in the function signature (I created the issue #29299 with a patch). Serhiy is also working on implementing the last major missing feature of AC: support *args and **kwargs parameters (issue #20291).
FYI I wasn't involved in AC changes, I only started to look at AC recently (1 or 2 months ago). Again, I agree that these changes should be carefully reviewed, which is an hard task since required changes are usually large and move a lot of code. We need more eyes to look at these changes!
For the specific case of sorted(), the name of first parameter is already documented in the docstring and documentation in Python 2.7: "iterable". So I guess that you mean that it is now possible to use it as a keyword argument. Well, see the issue #29327 for the long story. This issue is a regression, it was already fixed, and I didn't introduce the API change.
Oh by the way, when I read your comment, I understand that I'm responsible of all regressions. It's true that I introduced regressions, that's where I said "shit happens" (or more politically correct: "it's better to ask forgiveness than permission" ;-)). Since I'm one of the most active contributor in CPython, I'm not surprised of being the one who introduce many (most?) regressions :-) I'm trying to review my changes multiple times, test corner cases, etc. But I'm not perfect.
Sadly, to show its full power, FASTCALL requires changes at many levels of the code. It requires to change at lot of code, but I understood that core developers approved the whole project. Maybe I was wrong? At least, I asked for permissions multiple changes, especially at the start.
- FWIW, the claim that the help is much better is specious. AFAICT, there has never been the slightest problem with "sorted(iterable, key=None, reverse=False) --> new sorted list" which has been clear since the day it was released. It is some of the new strings the are causing problems with users (my students frequently are tripped-up by the / notation for example; no one seems to be able to intuit what it means without it being explained first).
Good news, it seems like you have a good experience in API design, documentation, etc. Join the "Argument Clinic" project to help us to enhance docstrings, function signatures and documentation ;-)
See the good part of the AC on-going work: it's a nice opportunity to also enhance documentation, not only provide a signature.
By the way, to be honest, the main advantage of converting functions to AC is to get a signature. The signature is visible in docstrings which is nice, but it is also very useful to a wide range of tools like (IDE, static checks, etc.).
Conversion to FASTCALL is more a nice effect. At least, it is a good motivation for me to convert mor and more code to AC :-)
AC moves docstring closer to the list of parameters. IHMO it makes the C code simpler to read and understand. It also removes the boring code responsible to "parse" arguments, so it makes the code shorter. But well, this is just my opinion.
- FWIW, I'm trying to be constructive and contribute where I can, but frankly I can't keep up with the volume of churn. Having seen bugs being introduced, it is not inappropriate to ask another dev to please be careful, especially when that dev has been prolific to an unprecedented degree and altering core parts of the language for function calls, to new opcodes, the memory allocators, etc. Very few people on the planet are competent to review these changes, make reasonable assessments about whether the complexity and churn are worth it. An fewer still have the time to keep up with the volume of changes.
Hum, I wasn't involved in bytecode changes.
Well, I reviewed the very good work of Demur Rumed. I recall that you worked on a similar area, trying to fetch bytecode by 16-bit instead of 8-bit. Demur proposed a good design and I recall that the design was approved.
I helped a little bit on the implementation and I pushed the final change, but all credits go to Demur and Serhiy Storshaka! By the way, Serhiy made further efficient enhancements in the bytecode of CALL_FUNCTION instructions.
About memory allocations, I guess that you are referring to my change on PyMem_Malloc() allocator. I discussed the issue on python-dev and waited for approval of my peers before pushing anything, since I know well that it's a critical part of Python: https://mail.python.org/pipermail/python-dev/2016-March/143467.html
I provide all data requested by Marc Andre Lemburg (test the change with common projects, Django, Pillow, numpy) and made further changes (PYTHONMALLOC=debug tool) to help to handle this backward incompatible change (GIL is now required to call PyMem_Malloc).
Hopefully, it seems like nobody noticed this subtle change (GIL now requied): I didn't see any bug report. By the way, I fixed a misused PyMem_Mem() in numpy.
- Please do continue your efforts to improve the language, but also please moderate the rate of change, mitigate the addition complexity, value stability over micro-optimizations, consult the authors and maintainers of code, take special care without code that hasn't been reviewed because that lacks a safety net, and remember that newer devs may be taking cues from you (do you want them making extensive changes to long existing stable code without consulting the authors and with weak LGTM reviews?)
Ok, I will do it.
Thank you for you feedback Raymond. I hope that my email helps you to understand how I work and how I take my decisions.
Victor
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On Sat, Jan 21, 2017 at 9:51 PM, Brett Cannon brett@python.org wrote:
What I'm picking up from this is (as a gross oversimplification):
- Victor _wants_ code reviews
- Raymond thinks we _need_ code reviews
So the common theme here regardless of whether you agree with Raymond or Victor's approach to development is that we are not getting enough code reviews to go around. To me that's what the systemic issue is that this email is bringing up.
Now I think most of us don't think the solution to the lack of reviews is to lower our standard of what it takes to become a core developer (this doesn't mean we shouldn't do a better job of identifying potential candidates, just that we shouldn't give people commit privileges after a single patch like some projects do). To me that means we need to address why out of 79 core developers only 39 have a single commit over the past year, 30/79 have more than 12 commits over that same time scale, 15/79 people have more than 52 commits, and 2/79 people have over 365 commits (https://github.com/python/cpython/graphs/contributors?from=2016-01-22&to=2017-01-21&type=c for the stats).
Some of you have said you're waiting for the GitHub migration before you start contributing again, which I can understand (I'm going to be sending an email with an update on that after this email to python-dev & core-workflow). But for those that have not told me that I don't know what it will take to get you involved again.
I can tell you what drove me away: python-issues, 320 from python-dev). This also means that:
- real life got in the way, time is limited;
- I spend the available time working on the bug tracker rather than on core Python, since my time is likely more valuable if spent on bpo;
- MLs are quite high traffic, and I gave up keeping up /for now/ [0] (I have 6.8k unread mails from python-checkins, 460 from
a) if a bug related to one of the modules I maintained is opened, I won't see it[1]; b) if a message explicitly asks for my opinion/review, I won't see it[1]; 4) the GitHub migration (first I was against, now I'm waiting for everything to settle and then give it a try, in future I hope it will become a reason to get me involved again); 5) the more time I spend away, the more difficult it becomes to follow all the new features that get introduced.
As for what would get me involved again, for my case there's not much else you can do -- I just need to find more free time, catch up with development (mails, tools, features, etc) and then try to keep up.
Best Regards, Ezio Melotti
[0]: FWIW one thing that helped me keeping the mail traffic low(er), was following the weekly issues reports on python-dev, and adding myself to nosy on the issues I cared about, rather than subscribing and following the other mailing list (python-bugs-list and new-bugs-announce). I also used to keep an eye on #python-dev for new issues and replies, and I wish more core-devs would hang around there, since it makes communication (including asking for opinions/reviews) much faster.
[1]: Feel free to reach to me directly, either on IRC or via direct mail.
For instance, not to pick on Andrew but he hasn't committed anything but he obviously still cares about the project. So what would it take to get Andrew to help review patches again so that the next time something involving random comes through he feels like taking a quick look?
As I have said before, the reason I took on the GitHub migration is for us core developers. I want our workflow to be as easy as possible so that we can be as productive as possible. But the unspoken goal I have long-term is to get to the point that even dormant core devs want to contribute again, and to the point that everyone reviews a patch/month and more people reviewing a patch/week (although I'll take a patch/year to start). I want to get to the point that every person with commit privileges takes 30 minutes a month to help with reviews and that the majority of folks take 30 minutes a week to review (and please don't think this as a hard rule and if you don't the privileges go away, view this as an aspirational goal). Even if people who don't have time to review the kind of patches Victor is producing which triggered this thread, reviewing documentation patches can be done without deep knowledge of things and without taking much time. That way people who have time to review the bigger, more difficult patches can actually spend their time on those reviews and not worrying about patches fixing a spelling mistake or adding a new test to raise test coverage.
All of this is so that I hope one day we get to the point where all patches require a review no matter who proposed the code change. Now I think we're quite a ways of from being there, but that's my moonshot goal for our workflow: that we have enough quality reviews coming in that we feel that even patches from fellow core developers is worth requiring the extra code check and disbursement of knowledge without feeling like a terrible drag on productivity.
Once the GitHub migration has occurred I'm planning to tackle our Misc/NEWS problem and then automate Misc/ACKS. After that, though, I hope we can take the time to have a hard look at what in our workflow prevents people from making even occasional code reviews so that everyone wants to help out again (and if any of this interests you then please subscribe to core-workflow).
On Fri, 20 Jan 2017 at 02:46 Victor Stinner victor.stinner@gmail.com wrote:
Hi,
Raymond Hettinger used a regression that I introduced in the builtin sorted() function (in Python 3.6.0) to give me his feedback on my FASTCALL work, but also on Argument Clinic.
Context: http://bugs.python.org/issue29327#msg285848
Since the reported issues is wider than just FASTCALL, including how I contribute to CPython, I decided to discuss the topic with a wider audience. I continue the discussion on python-committers to get the opinion of the other core developers.
Sorry for my very long answer! I tried to answer to each issues reported by Raymond.
Inaccurate summary: I'm a strong supporter of "it's better to ask forgiveness than permission", whereas Raymond considers that I introduced too many regressions with my workflow.
Raymond Hettinger added the comment:
A few random thoughts that may or may not be helpful:
- We now have two seasoned developers and one new core developer that
collectively are creating many non-trivial patches to core parts of Python at an unprecedented rate of change. The patches are coming in much faster than they can reasonably be reviewed and carefully considered, especially by devs such as myself who have very limited time available. IMO, taken as whole, these changes are destabilizing the language. Python is so successful and widely adopted that we can't afford a "shit happens" attitude. Perhaps that works in corners of the language, infrequently used modules, but it makes less sense when touching the critical paths that have had slow and careful evolution over 26 years.
- Besides the volume of patches, one other reason that reviews are hard to come by is that they apply new APIs that I don't fully understand yet. There are perhaps two people on the planet who could currently give thoughtful, correct, and critical evaluation of all those patches. Everyone else is just watching them all fly by and hoping that something good is happening.
Since one or maybe even two years, I noticed that many of my issues were blocked by the lack of reviews. As you wrote, only few developer have the knowledge and background to be able to provide a good review (not only "tests pass, so LGTM") on my changes modifying the Python core.
I also wanted to discuss this topic, but I didn't really know what to propose. Let's take this opportunity to explain how I contribute to CPython, especially how I decide to wait for a review or not.
For each patch that I write, I estimate the risk of regression. You may know that any regression is something unexpected, so such estimation is tricky. Here is my heuristic:
(*) if the patch is trivial (short, non controversal), I push it immediatly.
(*) If I'm less confident, I open an issue and attach the patch. I wait at least one day before pushing.
It's strange, but the process of opening an issue and attaching the patch usually helps to review the code myself (find bugs, or more generally enhance the patch). Maybe because it forces me to review the change one more time?
If the change is not part of a larger patch serie, so doesn't block me to move further, I try to keep the issue open around one week.
The truth is that too few of my patches get a review :-/ Maybe I should wait longer, but then it becomes harder for me to handle many patches.
Maybe it's a tooling issues. Recently, I started to use local branches in a Git repository. It helps a lot of work on parallel on large changes. Before, I only worked in a single directory (default/, the default Mercurial branch) and applied/reverted patches everytime. It's painful, especially when I have to use multiple computers, download again publshed patches, etc. Maybe it will run smoother once CPython will move to Git and GitHub.
By the way, it's painful to squash a long patch serie into a giant patch, much harder to review, where changes don't make sense at all at the first look. Again, a better reviewing tool supporting patch series (GitHub) will help here too.
Not supporting patch series in our reviewing tool also explains why I prefer to push than having to wait for a review. Rebasing manually long patch series stored as giant .patch files is complicated.
(*) If the change changes an API or changes a core component, I wait for at least one review from a core reviewer. Sometimes, I even send an email to python-dev. Again, sometimes I don't get any feedback on the patch nor the email after two weeks :-/ At least, I tried :-) Usually, I get feedback in less than one week, or no feedback at all. I understand that nobody understands my change or nobody cares :-)
I totally understand that most core developers have a little amount of time available to contribute to Python. I'm trying to find a compromise between the risk of introducing regressions and being stuck in my work. This email might help me to adjust my workflow.
By the way, I'm trying to always run the full test suite (./python -m test -rW -j0) before pushing any change. If I suspect that I may have introduced reference leaks, I also run "./python -m test -R 3:3 ..." on the tests related to the modified code to check for memory/reference leaks.
- One other reason for the lack of review comments in the enthusiasm and fervor surrounding the patches. I feel like there is a cost of questioning whether the patches should be done or how they are done, like I am burning little karma every time. Sometimes it feels safest and most cordial to just say nothing and let you make hundreds of semi-reviewed changes to just about every critical part of the language.
"semi-reviewed". Let me be more accurate: yeah, I do push a lot of changes which were not reviewed by anyone (see above).
- Historically, if there was creator or maintainer of the code who was still active, that person would always be consulted and have a final say on whether a change should be applied. Now, we have code constantly being changed without consulting the original author (for example, the recent and catastrophic random initialization bug was due to application of a patch without consulting the author of _randommodule.c and the maintainer of random.py, or this change to sorted(), or the changes to decimal, etc).
What do you mean by "author"? As you wrote, Python is now 26 years old, so it had a very long history, and each file has a very long list of "authors". I guess that you mean more a "maintainer".
My problem is that I'm not aware of any explicit list of maintainers. I didn't know that you were the maintainer of the random module before you told me that at the Facebook sprint last september. I didn't expect that the random module had a maintainer, I thought that any core developer would be allowed to modify the code.
Moreover, since I open an issue for most of my changes, it gives an opportunity to maintainers to review changes. Maybe we need more components in the bug tracker to notify maintainers of pending changes?
You mentionned 3 different changes, let me reply.
(1) The random change: http://bugs.python.org/issue29085
I introduced a regression in random.Random.seed(): a typo in the C code has the consequence that the current time and process identifier is used, instead of os.urandom(16), to initialize the Mersenne Twister RNG.
IMHO the regression is not "catastrophic". Only few developers instanciate random.Random themself, random.Random must not be used for security, etc. I let others decide if this bug was catastrophic or not.
Since we are talking about the development process, let me see how the change was made.
Context: The PEP 524 has a long and painful history... Something like more than 500 messages were sent on the bug tracker and python-dev, and nobody was listening to each others, two security experts "rage-quitted" Python because of this mess... I decided to try to fix this issue in a constructive way, so I wrote a PEP. Nick wrote a different PEP, since it was clear that it was possible to handle security in two different incompatible ways. A mailing list was even created just to discuss this bug! A mailing list just for a bug gives an idea of the size of the mess :-)
Well, about the change itself, it was done in http://bugs.python.org/issue27776
The patch was available for review during 19 days (2016-08-18-2016-09-06) and was reviewed by Nick Coghlan. Since Nick wrote a similar PEP, I trusted him to be able to review my change. (Well, anyway I already trust all core developers, but I mean that I was trusting him even more than usual :-))
Since the change has a big impact on security, I had prefer to get a review of more developers, especially our security experts... but as I wrote, two security experts "rage- quitted". Again, this PEP has a long and sad story :-/
Note: you say that you are the maintainer of the random module, but I don't recall having see you in any recent discussions and issues related to os.urandom(), whereas a lot of enhancements and changes were done last 2 years. I made many changes to support new OS functions like getentropy() an getrandom().
Oooookay, let's see the second change, "this change to sorted()", http://bugs.python.org/issue29327
(2) I introduced a bug in sorted(), last August: https://hg.python.org/cpython/rev/15eab21bf934/
Calling sorted(iterable=[]) does crash. To be honest, I didn't imagine that anyone would pass the iterable by keyword, but Serhiy is very good to spot bugs in corner cases :-)
IMHO the regression is subtle.
When I optimized the code to use FASTCALL, I replaced PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1). I checked that all tests passed, so it looks ok to me.
I didn't imagine that anyone would call sorted(iterable=[]), so I didn't notice that PyTuple_GetSlice() can create an empty tuple.
The previous code was wrong since sorted() accepted iterable as a keyword, whereas sort.list() doesn't.
So well, I let you guess if a review would have spot this bug in the large change.
(3) Recently, I ran sed to replace code patterns to use faster ways to call functions: https://hg.python.org/cpython/rev/54a89144ee1d
"Replace PyObject_CallObject(callable, NULL) with _PyObject_CallNoArg(callable)"
I recalled that I modified the _decimal module and that Stefan Krah complained, because he wants to have the same code base on Python 3.5, 3.6 and 3.7. He also mentionned an external test suite which was broken by recent _decimal changes (not sure if my specific change was in cause or not), but I wasn't aware of it.
To be honest, I didn't even notice that I modified _decimal when I ran sed on all .c files. Since the change was straightforward and (IMHO) made the code more readable, I didn't even wait for a review if I recall correctly.
Stefan and me handled this issue privately (he reverted my change), I'm not sure that it's worth it to say more about this "issue" (or even "non-issue").
To be clear, I don't consider that my change introduced a regression.
In general, Guido has been opposed to sweeping changes across the code base for only tiny benefits. Of late, that rule seems to have been lost.
The benefits of FASTCALL mainly apply to fine grained functions which only do a little work and tend to be called frequently in loops. For functions such as sorted(), the calling overhead is dominated by the cost of actually doing the sort. For sorted(), FASTCALL is truly irrelevant and likely wasn't worth the complexity, or the actual bug, or any of the time we've now put in it. There was no actual problem being solved, just a desire to broadly apply new optimizations.
Ok, first, you qualify my FASTCALL changes as code churn. So let me show an example with sorted(): https://hg.python.org/cpython/rev/b34d2ef5c412
Can you elaborate how such change increases the complexity?
Second, "no actual problem being solved"
Since the goal of FASTCALL is to optimize Python, I guess that you consider that the speedup doesn't justify the change. I gave numbers in the issue #29327:
Microbenchmark on sorted() on Python 3.7 compared to 3.5 (before FASTCALL):
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10))' 'sorted(seq)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 1.07 us +- 0.06 us -> [3.7] 958 ns +- 15 ns: 1.12x faster (-11%)
haypo@smithers$ ./python -m perf timeit 'seq=list(range(10)); k=lambda x:x' 'sorted(seq, key=k)' --compare-to=../3.5/python -v Median +- std dev: [3.5] 3.34 us +- 0.07 us -> [3.7] 2.66 us +- 0.05 us: 1.26x faster (-21%)
IMHO such speedup is significant even on a microbenchmark. Can you elaborate what are your criteria to decide if an optimization is worth it?
- Historically, we've relied on core developers showing restraint. Not every idea that pops into their head is immediately turned into a patch accompanied by pressure to apply it. Devs tended to restrict themselves to parts of the code they knew best through long and careful study rather sweeping through modules and altering other people's carefully crafted code.
Should I understand that I should restrict myself to some files? Or not touch some specific parts of Python, like... "your" code like random, itertools and collections modules?
I replied to the 3 issues you mentioned previously and explained how I contribute to Python.
FWIW, I applaud your efforts to reduce call overhead -- that has long been a sore spot for the language.
Guido has long opposed optimizations that increase risk of bugs, introduce complexity, or that affect long-term maintainability. In some places, it looks like FASTCALL is increasing the complexity (replacing something simple and well-understood with a wordier, more intricate API that I don't yet fully understand and will affect my ability to maintain the surrounding code).
I'm sorry, I didn't spent much time on explaing the FASTCALL design nor documenting my changes. It's partially deliberate to make everything related to FASTCALL private. Since it's a huge project modifying a lot of code, I wanted to wait until the APIs and the code stop moving too fast to take time to explain my work and document it.
If you have specific questions, please go ahead.
Shortest summary:
FASTCALL replaces (args: tuple, kwargs: optional dict) with (args: C array, nargs: int, kwnames: tuple of keyword keys). It's a new calling convention which allows to avoid a temporary tuple to pass positional arguments and avoids temporary dictionary to pass keyworkd arguments.
To use FASTCALL, C functions should be converted to the new METH_FASTCALL calling convention
PyObject_Call() can replaced with _PyObject_FastCallKeywords() or _PyObject_FastCallDict() (when we still get kwargs as a dict) in such conversion
Many existing C functions were optimized internally to use FASCALL, so even if you don't modify your code, you will benefit of it (speedup). Typical example: PyFunction_CallFunctionObjArgs().
The most massive change were purely internal and don't affect the most famous C APIs at all. In some cases, to fully benefit of FASTCALL, code should be modified. I'm trying to restrict such changes to Python internals, especially the most used functions.
I expected that the required changes were straightforward enough, it looks like I was wrong, but I don't recall anyone, before you recently, asking for an explanation.
- It was no long ago that you fought tooth-and-nail against a single line patch optimization I submitted. The code was clearly correct and had a simple disassembly to prove its benefit. Your opposition was based on "it increases the complexity of the code, introduces a maintenance cost, and increases the risk of bugs". In the end, your opposition killed the patch. But now, the AC and FASTCALL patches don't seem to mind any of these considerations.
Context: http://bugs.python.org/issue26201
It seems like we need more _explicit_ rules to decide if an optimization is worth it or not. For me, the de facto standard request for an optimization is to prove it with a benchmark. I requested a benchmark, but you refused to provide it.
So I ran my own benchmark and saw that your change made the modified code (PyList_Append()) 6% slower. I'm not sure that my bencmark was correct, but it was a first step to take a decision.
To come back to FASTCALL, your point is that it doesn't provide any speedup.
In most FASTCALL issues that I opened, I provide a script to reproduce my benchmark and the benchmark results. The speedup is usually betwen 10% and 20% faster.
Should I understand that 6% slower is ok, whereas 10-20% faster is not good? Can you please elaborate?
- AC is supposed to be a CPython-only concept. But along the way APIs are being changed without discussion. I don't mind that sorted() now exposes *iterable* as a keyword argument, but it was originally left out on purpose (Tim opined that code would look worse with iterable as a keyword argument). That decision was reversed unilaterally without consulting the author and without a test. Also as AC is being applied, the variable names are being changed. I never really liked the "mp" that used in dicts and prefer the use of "self" better, but it is a gratuitous change that unilaterally reverses the decisions of the authors and makes the code not match any of the surrounding code that uses the prior conventions.
Ah, at least I concur with you on one point :-) Changes to convert functions to AC must not change the API (type of arguments: positional only/keyword/..., default values, etc.) nor provide a worse docstring.
There is an active on-going work to enhance AC to fix issues that you reported, like the default value of positional-only parameters which should not be rendered in the function signature (I created the issue #29299 with a patch). Serhiy is also working on implementing the last major missing feature of AC: support *args and **kwargs parameters (issue #20291).
FYI I wasn't involved in AC changes, I only started to look at AC recently (1 or 2 months ago). Again, I agree that these changes should be carefully reviewed, which is an hard task since required changes are usually large and move a lot of code. We need more eyes to look at these changes!
For the specific case of sorted(), the name of first parameter is already documented in the docstring and documentation in Python 2.7: "iterable". So I guess that you mean that it is now possible to use it as a keyword argument. Well, see the issue #29327 for the long story. This issue is a regression, it was already fixed, and I didn't introduce the API change.
Oh by the way, when I read your comment, I understand that I'm responsible of all regressions. It's true that I introduced regressions, that's where I said "shit happens" (or more politically correct: "it's better to ask forgiveness than permission" ;-)). Since I'm one of the most active contributor in CPython, I'm not surprised of being the one who introduce many (most?) regressions :-) I'm trying to review my changes multiple times, test corner cases, etc. But I'm not perfect.
Sadly, to show its full power, FASTCALL requires changes at many levels of the code. It requires to change at lot of code, but I understood that core developers approved the whole project. Maybe I was wrong? At least, I asked for permissions multiple changes, especially at the start.
- FWIW, the claim that the help is much better is specious. AFAICT, there has never been the slightest problem with "sorted(iterable, key=None, reverse=False) --> new sorted list" which has been clear since the day it was released. It is some of the new strings the are causing problems with users (my students frequently are tripped-up by the / notation for example; no one seems to be able to intuit what it means without it being explained first).
Good news, it seems like you have a good experience in API design, documentation, etc. Join the "Argument Clinic" project to help us to enhance docstrings, function signatures and documentation ;-)
See the good part of the AC on-going work: it's a nice opportunity to also enhance documentation, not only provide a signature.
By the way, to be honest, the main advantage of converting functions to AC is to get a signature. The signature is visible in docstrings which is nice, but it is also very useful to a wide range of tools like (IDE, static checks, etc.).
Conversion to FASTCALL is more a nice effect. At least, it is a good motivation for me to convert mor and more code to AC :-)
AC moves docstring closer to the list of parameters. IHMO it makes the C code simpler to read and understand. It also removes the boring code responsible to "parse" arguments, so it makes the code shorter. But well, this is just my opinion.
- FWIW, I'm trying to be constructive and contribute where I can, but frankly I can't keep up with the volume of churn. Having seen bugs being introduced, it is not inappropriate to ask another dev to please be careful, especially when that dev has been prolific to an unprecedented degree and altering core parts of the language for function calls, to new opcodes, the memory allocators, etc. Very few people on the planet are competent to review these changes, make reasonable assessments about whether the complexity and churn are worth it. An fewer still have the time to keep up with the volume of changes.
Hum, I wasn't involved in bytecode changes.
Well, I reviewed the very good work of Demur Rumed. I recall that you worked on a similar area, trying to fetch bytecode by 16-bit instead of 8-bit. Demur proposed a good design and I recall that the design was approved.
I helped a little bit on the implementation and I pushed the final change, but all credits go to Demur and Serhiy Storshaka! By the way, Serhiy made further efficient enhancements in the bytecode of CALL_FUNCTION instructions.
About memory allocations, I guess that you are referring to my change on PyMem_Malloc() allocator. I discussed the issue on python-dev and waited for approval of my peers before pushing anything, since I know well that it's a critical part of Python: https://mail.python.org/pipermail/python-dev/2016-March/143467.html
I provide all data requested by Marc Andre Lemburg (test the change with common projects, Django, Pillow, numpy) and made further changes (PYTHONMALLOC=debug tool) to help to handle this backward incompatible change (GIL is now required to call PyMem_Malloc).
Hopefully, it seems like nobody noticed this subtle change (GIL now requied): I didn't see any bug report. By the way, I fixed a misused PyMem_Mem() in numpy.
- Please do continue your efforts to improve the language, but also please moderate the rate of change, mitigate the addition complexity, value stability over micro-optimizations, consult the authors and maintainers of code, take special care without code that hasn't been reviewed because that lacks a safety net, and remember that newer devs may be taking cues from you (do you want them making extensive changes to long existing stable code without consulting the authors and with weak LGTM reviews?)
Ok, I will do it.
Thank you for you feedback Raymond. I hope that my email helps you to understand how I work and how I take my decisions.
Victor
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On 2017-01-21, Brett Cannon wrote:
So the common theme here regardless of whether you agree with Raymond or Victor's approach to development is that we are not getting enough code reviews to go around. To me that's what the systemic issue is that this email is bringing up.
I think there is another issue. What pace of evolution is appropriate for Python 3.x? Python 2.7.x represents an extreme position: bug fixes only. Evolution for 3.x is too fast for some users. It is natural that as Python matures the average user is more interested in a stable langage than in bleeding-edge features. Python 3.x should be compelling to those users if we want to smoothly end-of-life 2.x. If 3.x looks like a "change treadmill" then 2.7 will have a long life.
I don't know how to solve that. Having experimental and stable forks of 3.x seems bad. Incompatible language versions are a drain on the ecosystem. However, a total block of language evolution is also no good.
Maybe we could emulate the Linux kernel releases. I.e. have relatively fast moving development but also choose releases to give long maintenance cycles. Ideally the long term releases would be synchronized with OS distribitions (e.g. Red Hat, Debian, Ubuntu).
The Linux kernel has a strict policy of not breaking user space programs. For Python, a similar rule would be not breaking Python programs. Until 2.x has finally died out, I think we should have an extra conservative policy on incompatible language changes.
Lack of patch reviewers is a major problem. Linux seems to have an advantage of more contributors and that many are paid to work on Linux. As far as I'm aware, there is no core Python developer who works mostly on Python and gets paid for it. So, we have to work with what we have or figure out some way to get people funded.
BTW, thanks for working on the GitHub migration Brett. I think it will help.
2017-01-24 21:46 GMT+01:00 Neil Schemenauer nas-python@arctrix.com:
Maybe we could emulate the Linux kernel releases. I.e. have relatively fast moving development but also choose releases to give long maintenance cycles. Ideally the long term releases would be synchronized with OS distribitions (e.g. Red Hat, Debian, Ubuntu).
You should take a look at this old deferred PEP: https://www.python.org/dev/peps/pep-0407/
Victor
On 2017-01-24, Victor Stinner wrote:
You should take a look at this old deferred PEP: https://www.python.org/dev/peps/pep-0407/
Thanks, that's very close to what I was thinking. I would still add that we should be extra careful about incompatible language features until 2.7.x usage has mostly died off. That isn't logically part of the PEP but a general development philosophy I think we should adopt.
On Tue, Jan 24, 2017 at 1:26 PM, Neil Schemenauer nas-python@arctrix.com wrote:
On 2017-01-24, Victor Stinner wrote:
You should take a look at this old deferred PEP: https://www.python.org/dev/peps/pep-0407/
Thanks, that's very close to what I was thinking. I would still add that we should be extra careful about incompatible language features until 2.7.x usage has mostly died off. That isn't logically part of the PEP but a general development philosophy I think we should adopt.
I love that PEP. However, I don't think the best way to attract large masses of people away from 2.7 and onto 3.* is to avoid adding features to 3.* releases: such an approach would not offer compelling reasons to migrate, as needed to overcome natural inertia.
Rather, thinking of what arguments I could bring to add further support to a case for migration (wearing the "consultant's hat" which I haven't donned in 12 years, so, I'm a bit rusty:-) I think: performance/scalability; stability; cool new features; whatever extra gizmos may help existing 2.7 code run fine under new shiny 3.whatever with only automated code transformation steps in the way (reverse migration, i.e 3.foobar -> 2.7, nowhere near as important). A lot of this describes stuff that HAS been happening -- the "stability" point would be particularly well addressed by PEP 407 or some variant thereof...
Alex
On Tue, 24 Jan 2017 at 13:26 Neil Schemenauer nas-python@arctrix.com wrote:
On 2017-01-24, Victor Stinner wrote:
You should take a look at this old deferred PEP: https://www.python.org/dev/peps/pep-0407/
Thanks, that's very close to what I was thinking. I would still add that we should be extra careful about incompatible language features until 2.7.x usage has mostly died off. That isn't logically part of the PEP but a general development philosophy I think we should adopt.
In terms of the stdlib we have already committed to not remove any code until after 2.7 hits EOL : https://www.python.org/dev/peps/pep-0004/#for-modules-existing-in-both-pytho... (but I suspect will be stretched until we decide to release Py4k).
We also have had a language change moratorium for Python 3.2: https://www.python.org/dev/peps/pep-3003/ . That was done in hopes of PyPy and other interpreters would use the time to catch up, but that never materialized.
But as Alex pointed out, part of the problem is people don't want to switch unless they have a compelling reason to that they can materially point to. For instance I have heard plenty of people say that async/await and f-strings are reasons enough for them to switch, not the fact that Python 3 has a more consistent design while now matching Python 2.7's overall performance.
I should also say that I think Python 3.6 is a rare release. With async opening up so many doors for consistency in the language it led to a lot of changes there. But then again Python 3.6 also had a whole PEP dedicated to adding a 'fold' argument to datetime. And in Victor's case a lot of stuff was performance stuff that's under the covers and not really exposed directly to the user anyway so no language moratorium would have changed things.
While I like the idea of PEP 407 if we stop doing bugfix releases for non-LTS releases and everything is considered provisional until it lands in an LTS release, I think we will have to see if GitHub helps or hurts our release process. If it makes it easier to cut releases then maybe it will be okay (that's something current and former RMs and their cohorts should be the ones to answer). But I'm also happy with waiting until 2020 to think about this -- which is 2 feature releases away -- or to make PEP 407 a Py4k thing.
On 24.01.2017 22:08, Victor Stinner wrote:
2017-01-24 21:46 GMT+01:00 Neil Schemenauer nas-python@arctrix.com:
Maybe we could emulate the Linux kernel releases. I.e. have relatively fast moving development but also choose releases to give long maintenance cycles. Ideally the long term releases would be synchronized with OS distribitions (e.g. Red Hat, Debian, Ubuntu).
You should take a look at this old deferred PEP: https://www.python.org/dev/peps/pep-0407/
I don't understand the reasoning behind that PEP. With the proposed timing, those "LTS" releases would be no different than what we have today.
The only effect of the PEP would be to do releases more often, which then results in using up minor release version numbers much too fast to give the impression of a stable programming system.
FWIW: I don't consider a release which is supported for just two years a long term support release. If we start using such a term it should match people's expectation following Ubunutu's LTS cycles, i.e. you get (at least) 5 years support for the release.
All that said, I believe a having a Python 2.7 style long support version for Python 3 would be nice and have a stabilizing effect which our user base would appreciate.
I'd not make this based on any timing, though. We'd just say: this is our new LTS release (e.g. Python 3.7) and then move on until we're confident again that the feature set has stabilized enough to create a new LTS release.
Perhaps making the last minor version to a major release version as we did with 2.7 is a good approach, i.e. we'd switch the major number when cutting an LTS release and follow similar guidelines as we have for 2.7 for this 3.x LTS release.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jan 25 2017)
Python Projects, Coaching and Consulting ... http://www.egenix.com/ Python Database Interfaces ... http://products.egenix.com/ Plone/Zope Database Interfaces ... http://zope.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 http://www.egenix.com/company/contact/ http://www.malemburg.com/
Le 25/01/2017 à 10:19, M.-A. Lemburg a écrit :
You should take a look at this old deferred PEP: https://www.python.org/dev/peps/pep-0407/
I don't understand the reasoning behind that PEP. With the proposed timing, those "LTS" releases would be no different than what we have today.
The only effect of the PEP would be to do releases more often,
That's indeed the main motivation: allow shipping changes faster to the subset of users who are ok with a shorter support cycle (which has the side-effect of making those changes tested in the wild earlier).
which then results in using up minor release version numbers much too fast to give the impression of a stable programming system.
People with such psychological bias can simply use said "LTS" releases, which would provide the same level of stability as today's feature releases.
FWIW: I don't consider a release which is supported for just two years a long term support release.
The vocabulary can be changed if that's a concern :-)
All that said, I believe a having a Python 2.7 style long support version for Python 3 would be nice and have a stabilizing effect which our user base would appreciate.
Trying to enforce such a level of commitment (if we're talking 5+ years of bugfix maintenance on an increasingly divergent codebase) in the already controversial PEP 407 is probably not a good idea. A separate PEP is in order.
We'd just say: this is our new LTS release (e.g. Python 3.7) and then move on until we're confident again that the feature set has stabilized enough to create a new LTS release.
In practice you wouldn't just "move on" but have to maintain that LTS release (which is the whole point). If we're talking something past the 2 years timerange, you can't just impose that on all core developers, so you need a subgroup of maintainers dedicated to that LTS release.
Regards
Antoine.
On 1/25/2017 7:30 AM, Antoine Pitrou wrote:
We'd just say: this is our new LTS release (e.g. Python 3.7) and then move on until we're confident again that the feature set has stabilized enough to create a new LTS release.
In practice you wouldn't just "move on" but have to maintain that LTS release (which is the whole point). If we're talking something past the 2 years timerange, you can't just impose that on all core developers, so you need a subgroup of maintainers dedicated to that LTS release.
Channeling Nick, I'd say that LTS support should come from commercial third parties, such as OS vendors. I agree with Antoine: it's not something we want to impose on the core devs. How much fun is 2.7 maintenance?
Eric.
On 25 January 2017 at 12:45, Eric V. Smith eric@trueblade.com wrote:
Channeling Nick, I'd say that LTS support should come from commercial third parties, such as OS vendors. I agree with Antoine: it's not something we want to impose on the core devs. How much fun is 2.7 maintenance?
Agreed. When lack of reviews is an issue, imposing additional "not fun" work on core devs isn't sustainable. As someone who does (non-Python) software support for my day job, dealing with legacy issues in Python doesn't feel like a hobby, it feels like work (and unlike the rest of my day, I'm not getting paid for it).
Paul
On Wed, Jan 25, 2017 at 07:45:05AM -0500, Eric V. Smith wrote:
Channeling Nick, I'd say that LTS support should come from commercial third parties, such as OS vendors. I agree with Antoine: it's not something we want to impose on the core devs. How much fun is 2.7 maintenance?
Are there any core developers who have Patreon accounts for monthly donations? Examples from more Linux-y projects:
https://www.reddit.com/r/linux/duplicates/5omtvg/patreons_to_support_open_so...
I think this is the next frontier for Python maintenance; we need full-time core maintainers, no third parties are funding any such developers, and the PSF doesn't seem interested in pursuing that.
--amk
On Wed, 25 Jan 2017 at 06:11 A.M. Kuchling amk@amk.ca wrote:
Channeling Nick, I'd say that LTS support should come from commercial
On Wed, Jan 25, 2017 at 07:45:05AM -0500, Eric V. Smith wrote: third
parties, such as OS vendors. I agree with Antoine: it's not something we want to impose on the core devs. How much fun is 2.7 maintenance?
Are there any core developers who have Patreon accounts for monthly donations? Examples from more Linux-y projects:
https://www.reddit.com/r/linux/duplicates/5omtvg/patreons_to_support_open_so...
I have yet to see anyone reach a level high enough to support themselves. The people that I have seen use these approaches and make a living are bloggers that also sell advertising and merchandise.
I think this is the next frontier for Python maintenance; we need full-time core maintainers, no third parties are funding any such developers, and the PSF doesn't seem interested in pursuing that.
I've talked to the PSF about this and it isn't a lack of desire, it's a lack of funds. The PSF wants to have at least a couple of years worth of salary in the bank before taking on any employee and I suspect the salary that would be required isn't a small sum.
On 2017-01-25, A.M. Kuchling wrote:
I think this is the next frontier for Python maintenance; we need full-time core maintainers, no third parties are funding any such developers, and the PSF doesn't seem interested in pursuing that.
IMHO, the PSF should be doing it. I don't know exactly how the Linux Foundation works but my superficial understanding is that the LF gets funding mostly from big companies and then directly pays some Linux developers. Most notable, Linus is paid by the LF.
Probably it is hard to attract and retrain top talent with a Patreon-like donation system. Software hackers are an eclectic group so some of them would prefer that system. However, I would guess the majority would prefer more stable employment.
Regarding PEP 407, I agree the LTS periods need to be longer. Certainly we can't force core developers into doing maintenance. However, there should be a process for an officially blessed LTS release. As a project, we should decide that it is worthwhile and figure out how to help make it happen.
I guess we could decide that we don't want to do LTS releases. I would accept that. Personally I think it would be a huge mistake, especially at this point. Python 2.7 is poised to become the ultimate LTS release of Python. I fear if we can't get more people to upgrade in the next few years, Python will be forever fractured into two communities.
On 25 January 2017 at 19:28, Neil Schemenauer nas-python@arctrix.com wrote:
On 2017-01-25, A.M. Kuchling wrote:
I think this is the next frontier for Python maintenance; we need full-time core maintainers, no third parties are funding any such developers, and the PSF doesn't seem interested in pursuing that.
IMHO, the PSF should be doing it. I don't know exactly how the Linux Foundation works but my superficial understanding is that the LF gets funding mostly from big companies and then directly pays some Linux developers. Most notable, Linus is paid by the LF.
Right now, the PSF's more concerned by the state of PyPI and the packaging ecosystem than they are CPython - keep in mind that one of the main concerns being raised about CPython development is that the pace of change is already *too high* for the rest of the ecosystem to keep up with (just based on those of us that have obtained individual agreements with our employers to spend part of our time on upstream contributions), whereas improvements in the packaging tools space (which provide a more immediate benefit to many more community members) are severely constrained by volunteer availability.
On that front, a funding proposal is being submitted to the Mozilla Grants program to finalize the sunsetting of the legacy web service at pypi.python.org, and migrating all operations over to pypi.org (those are currently running as parallel front ends to the same backing data store, but the new one is missing maintainer facing features that mean it isn't yet possible to shut down the old one).
Eric also correctly channeled me in that I think the right way for people to advocate for LTS CPython releases is:
- Pick a commercial Python redistributor
- Start paying them for support
- Advocate for *them* (through whatever channels they provide) to pursue a fully funded recurring LTS model in CPython upstream
That entirely avoids the "Is this an appropriate activity for a public interest charity to be funding?" question, and also gives commercial redistributors a clear practical benefit that they can pitch to their subscribers (i.e. getting fixes backported from the main line of development to LTS versions).
Cheers, Nick.
P.S. Since it's relevant to the conversation at hand, and we all collectively benefit from the upstream community maintaining strong ties with our commercial redistributors, I'll also point out that ActiveState, one of CPython's longest term commercial redistributors and one of the founding sponsors of the PSF, is currently hiring for a couple of key roles in their open source languages support and development team: http://www.activestate.com/company#careers
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Jan 25, 2017, at 07:45 AM, Eric V. Smith wrote:
Channeling Nick, I'd say that LTS support should come from commercial third parties, such as OS vendors. I agree with Antoine: it's not something we want to impose on the core devs. How much fun is 2.7 maintenance?
Which effectively happens anyway, although to varying degrees of activity. If an OS vendor releases a version of Python in their LTS, that's also a commitment to support that for the length of LTS (Python usually being within in the sphere of LTS supported packages, e.g. not everything in Ubuntu is guaranteed support for 5 years of an LTS, just 'main' packages).
Cheers, -Barry
On 25.01.2017 13:30, Antoine Pitrou wrote:
Le 25/01/2017 à 10:19, M.-A. Lemburg a écrit :
All that said, I believe a having a Python 2.7 style long support version for Python 3 would be nice and have a stabilizing effect which our user base would appreciate.
Trying to enforce such a level of commitment (if we're talking 5+ years of bugfix maintenance on an increasingly divergent codebase) in the already controversial PEP 407 is probably not a good idea. A separate PEP is in order.
This is not about enforcing commitment, it's about opening up our release process to be able to apply fixes on such an LTS beyond what we'd normally do.
I'm pretty sure we can find core developers or attract new ones who'd be happy to be able to help fix issues in Python releases they use in their day job rather than work on releases which they won't be able to use in their day for at least another few years due to company policies.
I also believe that the PSF should start stepping up and invest some money into helping with Python support. A lot of money is being spent on community work, but hardly any on development work at the moment.
By doing so, the PSF could also attract more sponsors, since sponsoring would then have a real tangible benefit to the sponsors.
We'd just say: this is our new LTS release (e.g. Python 3.7) and then move on until we're confident again that the feature set has stabilized enough to create a new LTS release.
In practice you wouldn't just "move on" but have to maintain that LTS release (which is the whole point). If we're talking something past the 2 years timerange, you can't just impose that on all core developers, so you need a subgroup of maintainers dedicated to that LTS release.
See above; this is about opening doors and lifting restrictions.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jan 25 2017)
Python Projects, Coaching and Consulting ... http://www.egenix.com/ Python Database Interfaces ... http://products.egenix.com/ Plone/Zope Database Interfaces ... http://zope.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 http://www.egenix.com/company/contact/ http://www.malemburg.com/
On Wed, 25 Jan 2017 at 06:29 M.-A. Lemburg mal@egenix.com wrote:
On 25.01.2017 13:30, Antoine Pitrou wrote:
Le 25/01/2017 à 10:19, M.-A. Lemburg a écrit :
All that said, I believe a having a Python 2.7 style long support version for Python 3 would be nice and have a stabilizing effect which our user base would appreciate.
Trying to enforce such a level of commitment (if we're talking 5+ years of bugfix maintenance on an increasingly divergent codebase) in the already controversial PEP 407 is probably not a good idea. A separate PEP is in order.
This is not about enforcing commitment, it's about opening up our release process to be able to apply fixes on such an LTS beyond what we'd normally do.
I'm pretty sure we can find core developers or attract new ones who'd be happy to be able to help fix issues in Python releases they use in their day job rather than work on releases which they won't be able to use in their day for at least another few years due to company policies.
I don't think that's necessarily true. I'm sure core developers fix bugs in Python 2.7 if they run into them and it affects their work, but beyond that I don't know how many of us are actually helping to maintain Python 2.7 beyond that (I for one immediately ignore all issues relating to 2.7 only at this point). And attracting people to help is typically not an issue, it's getting enough core developers to do code reviews to keep up with the workload.
-Brett
I also believe that the PSF should start stepping up and invest some money into helping with Python support. A lot of money is being spent on community work, but hardly any on development work at the moment.
By doing so, the PSF could also attract more sponsors, since sponsoring would then have a real tangible benefit to the sponsors.
We'd just say: this is our new LTS release (e.g. Python 3.7) and then move on until we're confident again that the feature set has stabilized enough to create a new LTS release.
In practice you wouldn't just "move on" but have to maintain that LTS release (which is the whole point). If we're talking something past the 2 years timerange, you can't just impose that on all core developers, so you need a subgroup of maintainers dedicated to that LTS release.
See above; this is about opening doors and lifting restrictions.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jan 25 2017)
Python Projects, Coaching and Consulting ... http://www.egenix.com/ Python Database Interfaces ... http://products.egenix.com/ Plone/Zope Database Interfaces ... http://zope.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 http://www.egenix.com/company/contact/ http://www.malemburg.com/
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On 25 January 2017 at 15:29, M.-A. Lemburg mal@egenix.com wrote:
I also believe that the PSF should start stepping up and invest some money into helping with Python support. A lot of money is being spent on community work, but hardly any on development work at the moment.
Software maintenance is a commercial support activity that is normally done for profit, so the PSF needs to be careful in how it approaches it to avoid getting in trouble with the IRS (public interest charities like the PSF operate under different taxation rules from trade associations like the Linux and OpenStack Foundations, and hence have a different set of constraints on their activities).
By doing so, the PSF could also attract more sponsors, since sponsoring would then have a real tangible benefit to the sponsors.
This is the aspect the PSF needs to be careful about, as it's treading very close to the line of activities that are better suited to a trade association or for-profit corporation.
Something along the lines of the Twisted or Django fellowship would likely be feasible - that would involve a grant request from the core development community to fund someone to focus full-time on issue triage and patch review for a designated period of time.
We'd need volunteers to help out with the review and selection process for applicants for the role, as well as a volunteer to actually write a grant proposal (including coming up with measurable objectives to help the PSF judge whether or not the grant was a success).
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Thu, Jan 26, 2017 at 01:58:42PM +0100, Nick Coghlan wrote:
Software maintenance is a commercial support activity that is normally done for profit, so the PSF needs to be careful in how it approaches it to avoid getting in trouble with the IRS (public interest charities like the PSF operate under different taxation rules from trade associations like the Linux and OpenStack Foundations, and hence have a different set of constraints on their activities).
This only seems true if "software maintenance" in the above means something like "writing contracts and taking money to apply bugfixes within certain guaranteed SLAs".
If "software maintenance" = "paying someone a salary to apply bugfixes to an old branch", that activity seems to clearly fall into a 501c3's purview under the categorizations of "advancement of education or science" or "erecting or maintaining public buildings, monuments, or works". [*]
I mean, if funding software maintenance is illegal for a 501c3, then the FSF, Software Conservancy, Software in the Public Interest, Django Foundation, NumFOCUS, etc. are probably all illegal.
(At some point this thread probably needs to move to psf-discuss-public or whatever it is.)
--amk
[*] https://www.irs.gov/charities-non-profits/charitable-organizations/exempt-pu...
On 26.01.2017 17:34, A.M. Kuchling wrote:
On Thu, Jan 26, 2017 at 01:58:42PM +0100, Nick Coghlan wrote:
Software maintenance is a commercial support activity that is normally done for profit, so the PSF needs to be careful in how it approaches it to avoid getting in trouble with the IRS (public interest charities like the PSF operate under different taxation rules from trade associations like the Linux and OpenStack Foundations, and hence have a different set of constraints on their activities).
This only seems true if "software maintenance" in the above means something like "writing contracts and taking money to apply bugfixes within certain guaranteed SLAs".
If "software maintenance" = "paying someone a salary to apply bugfixes to an old branch", that activity seems to clearly fall into a 501c3's purview under the categorizations of "advancement of education or science" or "erecting or maintaining public buildings, monuments, or works". [*]
I mean, if funding software maintenance is illegal for a 501c3, then the FSF, Software Conservancy, Software in the Public Interest, Django Foundation, NumFOCUS, etc. are probably all illegal.
I agree with Andrew: while software maintenance is a great business model for some companies, it's certainly not an activity that may only exclusively done by for-profit entities (even though some of those companies might find that useful ;-)).
As long as we're not using software maintenance for some hidden political lobbying agenda, we're fine as PSF.
(At some point this thread probably needs to move to psf-discuss-public or whatever it is.)
Agreed as well, but for the PSF to get active in this area, we'd have to draft up a proposal.
Note that the PSF does have a fair amount of money in the bank, which would certainly be enough to contract developers for doing maintenance or development work, either for dedicated work (as was done in the past a couple of times), or continuously, provided the rates are reasonable.
The main problem the PSF has is not having enough human resources to provide project management or even oversight, so a lot of such funding activities depend on whether there are trusted people available who could take up these tasks on behalf of the PSF.
[*] https://www.irs.gov/charities-non-profits/charitable-organizations/exempt-pu...
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jan 26 2017)
Python Projects, Coaching and Consulting ... http://www.egenix.com/ Python Database Interfaces ... http://products.egenix.com/ Plone/Zope Database Interfaces ... http://zope.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 http://www.egenix.com/company/contact/ http://www.malemburg.com/
On 26 January 2017 at 17:34, A.M. Kuchling amk@amk.ca wrote:
I mean, if funding software maintenance is illegal for a 501c3, then the FSF, Software Conservancy, Software in the Public Interest, Django Foundation, NumFOCUS, etc. are probably all illegal.
Note that I said the PSF needs to be careful in how it approaches the question, not that it can't do it at all.
Problematic: "Commercial and other institutional end users are worried about a lack of developer time being invested in maintaining long term support branches, so the PSF should commit to funding that directly" (this is the motivation where I believe the answer should be "Pay a vendor for commercial support and tell them to work more actively on the sustainability problem")
Entirely fine: "The core development community have submitted a grant request to fund a dedicated part-time contract role for X months to facilitate issue triage, patch reviews, and mentoring of potential new core developers"
The latter motivation is about supporting the community and facilitating its growth, which is entirely in line with the Foundation's public interest mission.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On 25 January 2017 at 10:19, M.-A. Lemburg mal@egenix.com wrote:
Perhaps making the last minor version to a major release version as we did with 2.7 is a good approach, i.e. we'd switch the major number when cutting an LTS release and follow similar guidelines as we have for 2.7 for this 3.x LTS release.
It also takes about that long for *idiomatic* expectations to change - Python 2.7 code written by someone that learned Python 3 first is likely to look quite different from code written by someone that started with 1.5.2 or one of the earlier 2.x releases.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
2017-01-21 20:51 GMT+01:00 Brett Cannon brett@python.org:
What I'm picking up from this is (as a gross oversimplification):
- Victor _wants_ code reviews
- Raymond thinks we _need_ code reviews
For a concrete example, I wrote a patch for a major regression in the datetime module at 2017-01-04: http://bugs.python.org/issue29100
I'm waiting for a review. To be honest, I almost forgot this issue since I didn't get any feedback. I didn't get any review. In the meanwhile, a duplicate was opened: http://bugs.python.org/issue29346
Usually, when this case occurs, I push my patch without review. But since some developers wrote that the want to review, please take a look at timestamp_limits.patch of http://bugs.python.org/issue29100 !
"timestamp_limits.patch is waiting for your review! This issue is a major regression of Python 3.6. I'm going to push the patch in one week if I don't get any review."
Victor
I read this whole thread. Here is my opinion.
- Lack of reviewers
It's really problem. If well written patch is not reviewed and committed for long time, patch writer (core developer, or potential core developer) may lost their motivation. Motivated contributor will be lower and lower.
I wrote compact dict patch in June, before Python 3.6a3. Luckily, the patch was reviewed and committed in September, right before 3.6b1. But I realized Python lacks active reviewers heavily. That's main reason why I wanted to be core developer.
I was very lucky. But there might be some contributors lost motivation because of lack of review.
- Stability
Stability is important. Review is valuable for stability. But even though careful review, regression may be happens.
Python 3.6 has many improvements. And many of them are committed right before 3.6b1, at sprint. http://blog.python.org/2016/09/python-core-development-sprint-2016-36.html
I feel adding fundamental changes (like FASTCALL) at early stage of development process (alpha1~3) is also valuable for stability of last beta, RC and final.
And I think advocating running test with "nightly" Python on Travis-CI may help stability, even though there are few beta users.
One regression caused by compact dict (*), and it was one reason of 3.6rc2 was released. The regression is found by py.test.
*) An excuse: the issue was not from compact dict. It was issue of key sharing dict. But compact dict increased method which can touch the issue, and py.test touched it.
- Domain expert
I agree that consult to expert is good. But sometimes the expert may not be active.
And mechanical changes by sed or Coccinelle may affects wide part. It's not realistic to consult all experts.
We should think carefully it's worth enough. But after some devs agree it's worth enough, "ask forgiveness than permission" seems realistic approach, like Stefan reverted _decimal change.
- Github
Github may ease some problems we have, especially reviewing large patch. https://github.com/blog/2123-more-code-review-tools
Python 3.4.6 and 3.5.3 are released already, migration happens near future? Anyway, I'm thanks to people working on the migration.
Regards,
participants (21)
-
A.M. Kuchling -
Alex Martelli -
Andrew Dalke -
Antoine Pitrou -
Barry Warsaw -
Brett Cannon -
Brian Curtin -
Doug Hellmann -
Eric V. Smith -
Ethan Furman -
Ezio Melotti -
INADA Naoki -
Lukasz Langa -
M.-A. Lemburg -
Neil Schemenauer -
Nick Coghlan -
Paul Moore -
Stefan Krah -
Tal Einat -
Victor Stinner -
Yury Selivanov