Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create --pex-path argument for pex cli and load pex path into pex-info metadata #417

Merged
merged 8 commits into from
Sep 28, 2017

Conversation

CMLivingston
Copy link
Contributor

@CMLivingston CMLivingston commented Sep 15, 2017

Problem:
Pex environments rely on the PEX_PATH environment variable to resolve modules from other pexes when composing them into a single pex. Python scripts that re-execute (like a server listening for fs changes, a Jupyter notebook) throw an ImportError upon re-exec due to environment scrubbing that removes this PEX_PATH information.

Solution:
Add a --pex-path argument to the pex client and plumb the data through to the pex-info metadata. Resolve pexes to compose into the current pex being built by reading from the new pex_path property of the pex-info metadata. This will ensure a self-contained environment that does not lose context on a python script re-exec.

Relates to: pantsbuild/pants#4682

@CMLivingston CMLivingston changed the title Clivingston/re-exec-bugfix Create --pex-path argument for pex cli and load pex path into pex-info metadata Sep 15, 2017
Copy link
Contributor

@kwlzn kwlzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good. handful of small things.

pex/bin/pex.py Outdated
dest='pex_path',
type=str,
default=None,
help='Pex path for resolving pexes that should be composed into the output pex environment.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about something a little more descriptive here, like:

help='A colon separated list of other pex files to merge into the runtime environment.'

pex/pex.py Outdated
# in either the environment or with the --pex-path arg to pex cli
if pex_info.pex_path is None:
pex_info.pex_path = ''

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about guarding the for loop here with an if statement instead of overwriting the None with '' just so that you can perform a .split() on it later?

e.g.

if pex_info.pex_path:
  for pex_path in filter(..., pex_info.pex_path.split(...)):
    ...

pex/pex_info.py Outdated
"""The PEX_PATH to search in to resolve dependencies.

This is meant to replace PEX_PATH environment variable which gets scrubbed on a python
module re-exec and loses context for the executing environment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on more descriptive here.

This is also not meant to replace the PEX_PATH environment variable, but to persist it within a built pex for reuse.

pex2_path = os.path.join(output_dir, 'pex2.pex')
res2 = run_pex_command(['--disable-cache', 'flask', '-o', pex2_path])
res2.assert_success()
pex_path = os.path.join(output_dir, 'pex2.pex') + ":" + os.path.join(output_dir, 'pex1.pex')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more concise maybe?

pex_path = ':'.join(os.path.join(output_dir, name) for name in ('pex1.pex', 'pex2.pex'))

here and below.

# create test file test.py that attmepts to import modules from pex1/pex2
test_file_path = os.path.join(output_dir, 'test.py')
with open(test_file_path, 'w') as fh:
fh.write(dedent('''\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't need the \ escape here since this block is triple quoted.

run_pex_command(['--disable-cache',
'--pex-path={}'.format(pex_path),
'wheel',
'-o', pex_out_path], env=env)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for the env here.

@@ -186,3 +186,102 @@ def test_pex_multi_resolve():
assert len(included_dists) == 4
for dist_substr in ('-cp27-', '-cp36-', '-manylinux1_x86_64', '-macosx_'):
assert any(dist_substr in f for f in included_dists)


@pytest.mark.xfail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to pass a reason=str arg to xfail for context on why this is expected to fail. linking a ticket etc that describes the issue would be sufficient.

e.g.

@pytest.mark.xfail(reason='See http://github.com/...')

# set up env for pex build with PEX_PATH in the environment
env = os.environ.copy()
env['PEX_PATH'] = pex_path
env['RAN_ONCE'] = '0'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could kill the need to set RAN_ONCE = 0 by doing a if 'RAN_ONCE' in os.environ: as the check in the test script in both tests.

else:
env = os.environ.copy()
env['RAN_ONCE'] = '1'
subprocess.call([sys.executable] + [env['PEX']] + sys.argv, env=env)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than passing the PEX var via the env, you could just hardcode it in the script content via parameterization with pex_out_path.


# run test.py with composite env
stdout, rc = run_simple_pex(pex_out_path, [test_file_path], env=env)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it seems you can kill the env here too when the need to set RAN_ONCE and PEX goes away.

Copy link
Contributor

@kwlzn kwlzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thanks!

@kwlzn kwlzn merged commit 5c663c8 into pex-tool:master Sep 28, 2017
@kwlzn kwlzn mentioned this pull request Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants