Skip executing apps.ready for installed apps

[flaeppe]

I have made things!

I got an idea from reading #1337 (comment), that it'd be possible and also preferable to avoid triggering AppConfig.ready for INSTALLED_APPS. For multiple reasons:

• It'll avoid reporting of issues of django-stubs crashing for reasons unrelated to django-stubs (https://github.com/typeddjango/django-stubs/issues?q=is%3Aissue+%22in+ready%22).
• I don't think there should ever be a mypy plugin requirement existing in an AppConfig.ready. It should work without it.
• It should (in the general case) make startup time of django-stubs shorter

The approach taken here is quite invasive(i.e. monkey patching with unittest.mock), it could be done in multiple ways. Consider it a PoC.

Related issues

Closes #1312

[flaeppe]

@intgr @sobolevn do you have any opinion on this?

Not really an argument, just additional info; I ran this on one of our projects with a 6 second speedup. Although we do a lot of stuff during startup there.

[intgr]

Ah sorry. Seems like a good idea, although not a fan of the monkeypatching.

What gave me pause is that I imagine some people might be using the ready() callbacks to impement some hacks or monkeypatching, e.g. changing model or field options, which mypy then would fail to pick up. So I was wondering whether it should be configurable. There could be some other use cases that I can't think of. But there's no precedent currently for making plugin features toggleable.

[intgr]

I ran this on one of our projects with a 6 second speedup

If this is purely from the ready() callbacks, it sounds like a rather poor developer experience, since every ./manage.py invocation incurs this delay as well.

If the ready callbacks are only necessary for e.g. runserver to function, this would be better solved by Django passing some information to ready() about what it's supposed to be preparing for. I wonder if antying like this has been discussed upstream for Django.

It'll avoid reporting of issues of django-stubs crashing for reasons unrelated to django-stubs

IMO this is a major design issue with the current plugin design -- importing checked code into the mypy runtime is problematic for many reasons (also mentioned in #1337 (comment)). This PR only fixes a symptom of that, but the list of issues reported with this symptom is impressive for sure.

Ideally we should also think about what a better design would look like. Although such a solution would probably take a lot longer.

[flaeppe]

What gave me pause is that I imagine some people might be using the ready() callbacks to impement some hacks or monkeypatching, e.g. changing model or field options, which mypy then would fail to pick up. ...

Just as the real goal of the plugin should be to not have to boot django's runtime, I don't think there should be any reason the plugin should support some arbitrary runtime code to be executed. I wouldn't claim that as a reason not to monkey patch ourselves, if we'd benefit from it.

If this is purely from the ready() callbacks, it sounds like a rather poor developer experience, since every ./manage.py invocation incurs this delay as well.

The timing isn't important and it has nothing to do with django or django-stubs. It was specifics for that project is all.

If the ready callbacks are only necessary for e.g. runserver to function, this would be better solved by Django passing some information to ready() about what it's supposed to be preparing for. I wonder if antying like this has been discussed upstream for Django.

I think ready() is what you like or need it to be. Django just gives you a hook to do stuff when it's ready, but hasn't started yet.

IMO this is a major design issue with the current plugin design -- importing checked code into the mypy runtime is problematic for many reasons (also mentioned in #1337 (comment)). This PR only fixes a symptom of that, but the list of issues reported with this symptom is impressive for sure.

Ideally we should also think about what a better design would look like. Although such a solution would probably take a lot longer.

While all that might be true, I think it'd help out skipping ready() as I think that'd be the main entrypoint for the plugin triggering runtime code of projects it type checks. Leading to arbitrary runtime errors during type checking.

What I'm trying to say is; that I'm thinking patching ready() will reduce the surface of runtime code touched by the plugin, thus minimising errors unrelated to our plugin.

[intgr]

Are you opposed to making this configurable?

You've demonstrated the issues solved by this change well. But it could open some other can of worms that causes issues for a different set of users.

I think ready() is what you like or need it to be. Django just gives you a hook to do stuff when it's ready, but hasn't started yet.

Yep, that's the thing. Django itself always unconditionally calls these hooks and we have no idea what users may be doing in those. We do initialize Django within the mypy process; this change would make a Django project in mypy context behave differently from a usual Django runtime context. Then we expect to be able to introspect models and settings in this different runtime context.

We could hypothesize that no user does anything in these ready() hooks that affects the introspection behavior, but I'm afraid Hyrum's Law will prove us wrong.

If some poor soul does hit this issue, it would be better if we can reply "OK just disable this option" than "welp, just keep using the old django-stubs version then". The latter has been our de facto stance in #1276 for example.

[flaeppe]

Are you opposed to making this configurable?

Not really, no.

But if so I'd prefer enabling the monkey patch of ready() as default, thus opting out of skipping ready(). Not too strong opinions about that either though.

[intgr]

Yes agreed about defaulting to enabled.

[intgr]

@flaeppe Are you planning to revive this? I hope I haven't discouraged you. :)

[flaeppe]

@flaeppe Are you planning to revive this? I hope I haven't discouraged you. :)

Sure, I just haven't gotten around to it.