gh-133485: Use _interpreters.call() in InterpreterPoolExecutor

[ericsnowcurrently]

Most importantly, this resolves the issues with functions and types defined in __main__.
It also expands the number of supported objects.

(This is based on gh-133484, thus only the last commit.)

• Issue: Minor InterpreterPoolExecutor Issues #133485

[ericsnowcurrently]

@markshannon, any ideas on why this is happening? It smells like a ceval bug, but it certainly could be something I've done wrong.

[ericsnowcurrently]

ping @markshannon

[neonene]

There seem to be related changes in inspect.getclosurevars() since 83ba8c2:

before:
ClosureVars(nonlocals={},
            globals={'INITIALIZER_STATUS': 'uninitialized'},
            builtins={}, unbound=set())
after:
ClosureVars(nonlocals={},
            globals={},
            builtins={}, unbound=set())

• init() on main (without L26):

  3           RESUME                   0

  5           LOAD_FAST_BORROW         0 (x)
              STORE_GLOBAL             0 (INITIALIZER_STATUS)
              LOAD_CONST               0 (None)
              RETURN_VALUE

• 3.3.5 (2014):

  5           0 LOAD_FAST                0 (x)
              3 STORE_GLOBAL             0 (INITIALIZER_STATUS)
              6 LOAD_CONST               0 (None)
              9 RETURN_VALUE

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit ccc135c 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133957%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[neonene]

The wasm32-wasi Non-Debug buildbot seems to be unused on merged commits and out of order on PRs. For example: https://buildbot.python.org/#/builders/1373/builds/507

[neonene]

Is the following usage invalid?

INITIALIZER_STATUS = 'uninitialized'

def init(x):
    global INITIALIZER_STATUS
    INITIALIZER_STATUS = x
    INITIALIZER_STATUS  # for now

def get_init_status():
    return INITIALIZER_STATUS

if __name__ == "__main__":
    from concurrent.futures import InterpreterPoolExecutor
    exe = InterpreterPoolExecutor(initializer=init, initargs=('initialized',))
    fut = exe.submit(get_init_status)
    assert fut.result() == 'initialized'  # fails
    exe.shutdown(wait=True)
    assert INITIALIZER_STATUS == 'uninitialized'

[neonene]

cpython/Python/crossinterp.c

Line 601 in ec12559

res.loaded = runpy_run_path(filename, run_modname);

I guess the failure case in my previous comment can be resolved if the runpy_run_path is called only once for __main__ by using/keeping ctx->main.cached->loaded or something across xi-sessions without being replaced. (My experiment was just putting a static local PyObject* var here ignoring the leak.)

[ericsnowcurrently]

That example should definitely work. You're probably right about the fix.

[ericsnowcurrently]

I've committed a solution that fixes the failure. The quirky thing, I realized, is that the loaded functions use the cached NS for __globals__, rather than the actual module's __dict__. Thus the functions copied into __main__ for unpickling won't actually operate against __main__. In the earlier failing example, that means the global variable INITIALIZER_STATUS doesn't show up in __main__. I need to ponder if that's a problem. (It might be surprising to users.)

[ericsnowcurrently]

I need to ponder if that's a problem. (It might be surprising to users.)

I think we're okay on this. The unpickled function isn't actually bound anywhere in the subinterpreter. Instead, it is pulled from the cached namespace, called, and then discarded (decref'ed). At worst, the function could treat its own __globals__ as though it were the subinterpreter's __main__ module, and cache something there, which would be pretty mysterious. We can deal with that later if it comes up.

[neonene]

A concern is that the shared items can conflict with the __main__ module's attributes:

InterpreterPoolExecutor(..., shared={'init': None})

If putting that aside, could _handle_unpickle_missing_attr() use something like PyDict_Update(_PyModule_GetDict(mod.module), mod.loaded) as an alternative to using the interpreter dictionary?

[neonene]

It seems like pickle and contextlib are now unused in this file. Importing pickle might be unnecessary when running only stateless functions?

[ericsnowcurrently]

A concern is that the shared items can conflict with the __main__ module's attributes:

InterpreterPoolExecutor(..., shared={'init': None})

Good point. The shared arg made more sense when InterpreterPoolExecutor supported scripts (text), in addition to callables. Since scripts aren't supported any more, I'll drop the shared parameter,

If putting that aside, could _handle_unpickle_missing_attr() use something like PyDict_Update(_PyModule_GetDict(mod.module), mod.loaded) as an alternative to using the interpreter dictionary?

That is essentially the same as exec'ing the original __main__ file in the subinterpreter's __main__ module. Either way, I'd prefer to avoid modifying __main__ when calling a function across interpreters (e.g. via submit()).

One alternative is to add a new module, e.g. __main_main__, in which we'd exec the original script. We'd do that instead of stashing it away on the interpreter's internal dict. I'll look into that.

[ericsnowcurrently]

@neonene, thanks for all the super helpful comments and feedback. You've had a real impact on the outcome here!