Make manager required in AstroidBuilder/InspectBuilder

[jacobtylerwalls]

Type of Changes

	Type
✓	🔨 Refactoring

Description

Closes #251

Remove delayed imports in AstroidManager by requiring AstroidManager to be provided to AstroidBuilder/InspectBuilder.

A couple new delayed imports became necessary, but they're in helpers like parse() and _astroid_bootstrapping(). We could address those by:

• Requiring AstroidManager for parse() calls also.
• Moving _astroid_bootstrapping() to manager.py.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.19%. Comparing base (6805fa3) to head (c452b0f).
Report is 47 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2313      +/-   ##
==========================================
- Coverage   93.19%   93.19%   -0.01%     
==========================================
  Files          93       93              
  Lines       11088    11066      -22     
==========================================
- Hits        10334    10313      -21     
+ Misses        754      753       -1     

Flag	Coverage Δ
linux	93.08% <100.00%> (-0.01%)	⬇️
pypy	93.19% <100.00%> (-0.01%)	⬇️
windows	93.19% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
astroid/brain/brain_nose.py	95.00% <100.00%> (ø)
astroid/builder.py	95.13% <100.00%> (+0.02%)	⬆️
astroid/interpreter/_import/spec.py	97.26% <100.00%> (+0.01%)	⬆️
astroid/manager.py	89.75% <100.00%> (-0.21%)	⬇️
astroid/raw_building.py	94.53% <100.00%> (-0.02%)	⬇️
astroid/rebuilder.py	98.18% <100.00%> (-0.03%)	⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Pierre-Sassoulas]

This one bring a lot of joy. Were you able to use nodes.X instead of X thanks to the fixed circular imports ?

[jacobtylerwalls]

I didn't have a choice, because the second import statement [from nodes import ...] was failing because nodes was now partially installed at that point. I think before, it was an accident that nodes was fully initialized already.

[DanielNoord]

Although really in favour, this do introduce more global state for AstroidManager(), see also #2048.

As long as AstroidManager is a singleton we'll keep accessing shared memory in multiprocessing scenarios. Ideally, to me, we would change it in 4.x so that the manager needs to be instantiated.

[jacobtylerwalls]

Although really in favour, this do introduce more global state for AstroidManager(), see also #2048.

Getting back up to speed on this to see if I can get it in for 4.0. @DanielNoord can you remind me what you meant by "more global state"? I'm not sure I follow.

[DanielNoord]

From my testing with multiprocessing on of the issues is that the cache of all nodes is stored on the manager, which means that we have a lot of dependency on this one shared object. Since it is global, and not passed or instantiated in pylint, it is quite hard to extract that dependency and (for example) give every process its own cache.

I'm fine with this PR, but I do think at some point we need to look into making the manager something that is instantiated by the process that wants to do inference.

[jacobtylerwalls]

Thanks.

As long as AstroidManager is a singleton we'll keep accessing shared memory in multiprocessing scenarios. Ideally, to me, we would change it in 4.x so that the manager needs to be instantiated.

Makes sense. For another day :-)

[Pierre-Sassoulas]

LGTM, just a nit.

[Pierre-Sassoulas]

This file would deserve to be a package, github refuse to display the diff, like it's a package-lock.json. (Just a passing observation, I'm not suggesting we immediately do something about it)