On Mar 2, 2021, at 6:10 PM, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:

Policy aside, this change doesn't seem like much of an improvement to me. If I were to guess, I would guess the change was made to satisfy some check Mypy is now being asked to make about Twisted. If that's the case, it seems unfortunate that real-world software is suffering so that a synthetic goal can be achieved. I do recognize there is a perception that practical value can come from attending to the errors Mypy reports. It would probably benefit everyone if more care were taken to consider the real-world consequences of changes that are made to satisfy the non-real-world goalposts set by tools like Mypy.

I do think that Mypy was likely highlighting a real issue here, and it should have been fixed. We had documented interfaces already, and we were failing to adhere to them properly, and Mypy was complaining about that. For easy reference, the change that made these fixes is here https://github.com/twisted/twisted/pull/1298 <https://github.com/twisted/twisted/pull/1298> This genre of fix is definitely something we should unwind over time, although it does make it easier to start mypy-clean rather than have hundreds of exclusion lines that need to be manually adjusted, so on balance I agree with this tradeoff at the point it was made. Specifically what I mean by "this genre of fix" is that you can always make mypy happy by transforming code like this: class Thing: pass Thing().stuff() into code like this: class Thing: @property def stuff(self): raise AttributeError("no such thing as `stuff`") Thing().stuff() The runtime behavior is the same, but now Mypy can't tell you about this class of bug any more. So, at some point, we should slowly unwind all `NotImplementedError` methods and find ways to either implement them for real or make their required use raise at import time. Finally: let's not develop an aversion to new tooling and change because it might create problems; experience over the last few years has shown me that Mypy can catch tons of real bugs and it's well worth getting the codebase to type check. If we want to prevent breakages like this in the future, the answer is not to stop trying to get linters and typecheckers to run cleanly with arbitrary changes, but to figure out some kind of continuous integration solution that's actually continuous with our downstream dependencies If dependencies could start testing against Twisted trunk in some capacity, we could get notified close to when unintentionally breaking changes occur, and dependencies can let us know well before the release happens, and we can either revert or they can fix things if the error is on their end. If initially, say, crossbar and matrix would like to work with us to set up some kind of repeatable pattern we can suggest to others, that would be great. -g

On Mar 3, 2021, at 4:58 AM, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:

Broadly, I agree. But not with this part. It seems like there is clearly a trade-off that is better for everyone. The trade-off represented by #1298: Breaks application code without providing any new functionality or fixing any bugs Captures a long list of TODOs as an arbitrarily complex collection of sources spread across the entire codebase All the work of addressing the problems still remains to be done Contrast this with any basic ratchet-style approach (for example, capture the list of mypy errors, then allow any PR that either removes some or doesn't add any new ones):

In principle, I agree. And some coarse-grained ratchets are absolutely worth doing, when they're supported by the tools - for example, we have one of these with the way we're handling `disallow_untyped_defs` right now. I wish tools like Mypy would build in ratcheting infrastructure that was easy to use and convenient to deploy. But in practice this type of ratchet involves a "# type: ignore" at every single call site, as well as delaying shipping stubs or maintaining separate stubs. As well as building the maladaptive habit that #type:ignore or cast()ing your way out of type errors is the right way to address them in individual features and getting every code reviewer used to that.

At the outset, no application code breaks because nothing has actually changed As work towards fixing the TODOs progresses, if it does break application code then at least applications have a chance at some benefit As the work towards fixing the TODOs progresses, maintainers can easily look up what issues remain by consulting the list of errors that feeds the ratchet-style job. The exact mechanism for the ratchet isn't the point here. Maybe Mypy plays more nicely with inline comments telling it to ignore something, I don't know. The point is:

The "exact mechanism for the ratchet" introduces enough hard problems that it inevitably delays tooling adoption for months or years, reduces the benefits of the tooling adoption (what Twisted users really want are comprehensive stubs, less than Twisted to type-check internally, I think, and this lets us ship them sooner in at least a mostly-usable state).

A centralized list of stuff left to do is better than a mess smeared across the whole codebase If we're going to risk breaking applications we should at least try to offer some value to them We shouldn't make applications deal with every change twice when we could just disturb them once

Overall I very much appreciate your criticism and I wish that I could provide clear and concrete steps towards a functioning ratchet for all of these types of tools, but in practice the minor issues caused by the boil-the-ocean approach are an order of magnitude easier to address (and cheaper to leave un-addressed) than the effort required to build ratchets for our contributors that aren't a total nightmare to maintain and interact with. Case in point: running `black` was a giant earthquake in the codebase but the pain associated with that was a drop in the bucket compared to the lingering, years-long nightmare of our attempt to build a style ratchet with twistedchecker. In principle what we were trying to do with twistedchecker was very simple, but the practicalities ate us alive. You could of course quite rightly argue that there were multiple conflated issues at play there! But that's also the point: there's always a lot of complexity that comes along with trying to do codemod migrations "the right way" when that's going against the grain of the rest of the ecosystem. I don't love that we've foisted this off onto our users, but I suspect Tobias spent a lot less time fixing the bug here than we've spent writing emails about it at this point ;-). I look forward to the day where I'm wrong about all of this and every new code quality tool comes with a super easy-to-deploy ratchet, though. -g

On Mar 3, 2021, at 8:25 AM, Richard van der Hoff <richard@matrix.org> wrote:

On 03/03/2021 08:07, Glyph wrote:

...

If dependencies could start testing against Twisted trunk in some capacity, we could get notified close to when unintentionally breaking changes occur, and dependencies can let us know well before the release happens, and we can either revert or they can fix things if the error is on their end. If initially, say, crossbar and matrix would like to work with us to set up some kind of repeatable pattern we can suggest to others, that would be great.

I'll take this to the Synapse team to discuss further, but we could probably easily arrange for one of our CI runs to install Twisted trunk from git instead of pypi, which might be a good start.

This is specifically the approach I'd really rather not take :) and here's why: You want to provide stability for you contributors so that if a problem is introduced, you don't halt development on that unrelated feature to diagnose the upstream issue. You want to ensure that when users install your software, it works with everything that's currently released. Ideally your CI configuration would be a timed job which builds against trunk and PR builds that build against a release, and then some process in place for reacting to regressions and filing a ticket upstream with us so we can work out what to do. Easier said than done though, I know! I wish we had something like this on Twisted for all of our dependencies, but we have had enough difficulty keeping our CI configuration current based on what cloud provider is falling over this month ;-). -g

...

but we have had enough difficulty keeping our CI configuration current based on what cloud provider is falling over this month ;-).

Yes, CI seems to be universally one of those things that is conceptually simple but somehow takes hours and hours to maintain.

Absolutely agreed, glad that it's not only me feeling like that=) If I'd sum up the time I just recently spent on pip, tox, docker, github actions, terraform, .. the list goes on, and compare to time actually spent on coding, it's depressing. for our cloud stuff with Crossbar.io, from code-to-live looks like: GitHub branch (code change) -> GitHub actions (CI main) -> GitHub master (code merge) -> Python wheel (CD "deploy") -> Docker image (CD "docker") -> downstream repo: AMI (via Packer) -> Terraform (live) for devices, it looks like: GitHub branch (code change) -> GitHub actions (CI main) -> GitHub master (code merge) -> Python wheel (CD "deploy") -> snap (CD "snap") -> downstream repo: Yocto -> S3 -> CDN -> RAUC (live) ---- obviously, a lot of things can go wrong. one decision we made: all CI/CD pipelines run through a common set of Python wheels we build.

Hi folks, On 03/03/2021 08:07, Glyph wrote:

Finally: let's not develop an aversion to new tooling and change because it might create problems; experience over the last few years has shown me that Mypy can catch /tons/ of real bugs and it's well worth getting the codebase to type check.  If we want to prevent breakages like this in the future, the answer is not to stop trying to get linters and typecheckers to run cleanly with arbitrary changes, but to figure out some kind of /continuous integration /solution that's actually continuous with our downstream dependencies

If dependencies could start testing against Twisted trunk in some capacity, we could get notified close to when unintentionally breaking changes occur, and dependencies can let us know well before the release happens, and we can either revert or they can fix things if the error is on their end.  If initially, say, crossbar and matrix would like to work with us to set up some kind of repeatable pattern we can suggest to others, that would be great.

Just to let you know, that I have (not before time) set up a Github Actions run which runs a subset of Synapse's CI against Twisted trunk. If it fails, it opens an issue against our repo, where it'll be triaged via our normal process. Hopefully this will bring benefits to everyone by giving early warning of any potential regressions! If you're interested, the GHA config is at https://github.com/matrix-org/synapse/blob/develop/.github/workflows/twisted.... Cheers Richard

On Aug 23, 2021, at 3:40 AM, Richard van der Hoff <richard@matrix.org> wrote:

Hi folks,

On 03/03/2021 08:07, Glyph wrote:

...

Finally: let's not develop an aversion to new tooling and change because it might create problems; experience over the last few years has shown me that Mypy can catch tons of real bugs and it's well worth getting the codebase to type check. If we want to prevent breakages like this in the future, the answer is not to stop trying to get linters and typecheckers to run cleanly with arbitrary changes, but to figure out some kind of continuous integration solution that's actually continuous with our downstream dependencies

If dependencies could start testing against Twisted trunk in some capacity, we could get notified close to when unintentionally breaking changes occur, and dependencies can let us know well before the release happens, and we can either revert or they can fix things if the error is on their end. If initially, say, crossbar and matrix would like to work with us to set up some kind of repeatable pattern we can suggest to others, that would be great. Just to let you know, that I have (not before time) set up a Github Actions run which runs a subset of Synapse's CI against Twisted trunk. If it fails, it opens an issue against our repo, where it'll be triaged via our normal process. Hopefully this will bring benefits to everyone by giving early warning of any potential regressions!

If you're interested, the GHA config is at https://github.com/matrix-org/synapse/blob/develop/.github/workflows/twisted... <https://github.com/matrix-org/synapse/blob/develop/.github/workflows/twisted...>.

Cheers

Richard

Awesome, thank you for doing this Richard! I see the ticket-opening action there too, so I look forward to hearing about bugs immediately when we screw things up in the future :) -g