[python-committers] 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 susp
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
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, ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
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. ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
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 ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
On 2017-01-20 8:38 AM, Stefan Krah wrote: 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. AFAIK, even though FASTCALL isn't "official", Cython already uses it. Yury ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
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 ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
> On Jan 20, 2017, at 9:26 AM, Yury Selivanov 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. - Ł ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
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 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_in_the_Debian_version_of_OpenSSL 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 [email protected] ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
2017-01-21 0:14 GMT+01:00 Andrew Dalke : > 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 ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
Re: [python-committers] My cavalier and aggressive manner, API change and bugs introduced for basically zero benefit
On 2017-01-20 5:02 PM, Lukasz Langa wrote: On Jan 20, 2017, at 9:26 AM, Yury Selivanov 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 ___ python-committers mailing list [email protected] https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
