Adds -m option to saved .pex file interpreter emulation

[markyen]

This allows you to invoke a module from a saved PEX file with app.pex -m <module>, in addition to invoking a script with app.pex <file> and a REPL with app.pex, as could already be done previously.

This makes a saved PEX file more suitable as a basic replacement for the python binary on platforms that allow you to do that but that don't have good support for Python dependency management themselves, such as PySpark.

[kwlzn]

thanks for the PR, this looks reasonable to me w/ one comment.

would also like to see an integration test exercising the new feature before landing.

[kwlzn]

seems like this unchecked access of index position 2 would cause an unhandled IndexError for the case of ./a.pex -m.

[lorencarvalho]

isn't this what PEX_MODULE=foo.bar:main and PEX_INTERPRETER=1 already do?

[markyen]

Added an extra check to avoid possibleIndexErrors and added integration tests.

It's true that this new flag does something you could already accomplish by setting PEX_MODULE: instead of running ./app.pex -m <module>, you could run PEX_MODULE=<module> ./app.pex and get the same result.

But my use case involves trying to use the built PEX file as the Python binary with a framework that doesn't know about PEX_MODULE; this framework just takes a configurable Python binary tries to invoke it with $PYTHON <script> or $PYTHON -m <module>. I'd like to be able to point $PYTHON at a built PEX file.

[lorencarvalho]

it might be worth noting that the .pexrc feature can inject environment variables for you if your environment doesn't support that. Placing a .pexrc file in the same CWD (or in $HOME) the pex file will read it.

Another way of achieving what you want without modifying pex source is to simply have a wrapper script that handles this sys.argv inspection and then runs an os.execve with environment variables set. Or even just a bash script, here's an example of one a tool I use builds:

#!/bin/bash

exec /usr/bin/env PEX_ROOT="$BASEDIR/libexec" PEX_MODULE="apollo:main" $BASEDIR/bin/server.pex "$@"

It should also be noted that you can use pex files as the shebang of a script a la #!/usr/bin/env yourpex.pex.

Not trying to discourage this change, just offering potential solutions that might work w/o cutting a new feature.

[kwlzn]

my understanding is that the -m <module> mode is specifically to smooth pex usage in PySpark. it's not the first time I've heard this request and since the change is limited to the default entrypoint handler it seems perfectly fine to me.

[markyen]

Indeed, in order to get pex working with PySpark now, I'm doing something very similar to what @sixninetynine suggested: with every PySpark job we write, I include a bash script that contains ./app.pex interpreter.py $@, and an interpreter.py that contains a copy of the pex.execute_interpreter function with the added ability of handling the -m flag, just as it appears in this PR.

This has been working fine for us for the last couple months, but as the number of PySpark jobs grows, so too does the number of places where this set of shim scripts needs to be duplicated, and shipped along with every run of every job.

I feel it would be cleaner to move the functionality into the upstream interpreter logic, but of course I'll leave the final design decision to you!

[kwlzn]

thanks for adding tests - one last comment and we should be good to merge.

[kwlzn]

sys.argv should not be modified here.

[kwlzn]

noting that sys.argv is modified below in the existing code too.. but I don't think that's necessary either. mind yanking it too while you're in here?

[kwlzn]

actually.. scratch that.. I see why the sys.argv modification is needed now (sorry about the thrash).

[markyen]

I'm actually not sure what the reason for sys.argv modification was, so I was just trying to match the behavior below. If a different modification is needed here, let me know and I can make that change.

[markyen]

@kwlzn If you let me know what the desired behavior is for modifying sys.argv, I'm happy to make a change to match it.

[markyen]

If anything else needs to be changed before this can be merged, please let me know and I'll be happy to change it.

[markyen]

@kwlzn Any thoughts on what further changes are needed?

[markyen]

Superseded by #563