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

Have bootstrap_pex_env pull in contents of pexrc files #747

Closed

Conversation

rtkjliviero
Copy link

@rtkjliviero rtkjliviero commented Jul 25, 2019

Added optional arg to bootstrap_pex_env which will cause pexrc configuration files from default locations to be pulled in.

This PR relates to the following issues:

  1. PEX_ROOT doesn't seem to be honoured for all directories. #155 (I'm having the same problem symptoms with the same basic cause)
  2. OSError: : [Errno 2] No such file or directory #293 (sort of related)

In my current use-case, I am activating one of several .pex files programmatically, and importing certain classes from those files. This works well, but since I'm using the Python API I can't reasonably pass environment variables to the command. os.environ seems to be ignored by bootstrap_pex_env(); having that dict be optionally respected would be another possible solution.

As an aside...while I think this is a good idea, right now I'm open to building my .pex files with the env vars I require, if that is currently supported by the tooling. I have tried pex --pex-root '/tmp/.pex/' -r <(pip freeze) -o some_output.pex, but that still results in ~ being used as PEX_ROOT.

@rtkjliviero
Copy link
Author

Also allowing the passing in of a dict or JSON string, to override vars for a single invocation.

@jsirois
Copy link
Member

jsirois commented Aug 6, 2019

As an aside...while I think this is a good idea, right now I'm open to building my .pex files with the env vars I require, if that is currently supported by the tooling. I have tried pex --pex-root '/tmp/.pex/' -r <(pip freeze) -o some_output.pex, but that still results in ~ being used as PEX_ROOT.

Pex does not in fact support embedding things like machine-specific paths in its metadata (PEX_INFO) at build time. You can only set PEX_ROOT at runtime via env var or .pexrc, '~/.pexrcor/etc/pexrccontaining aPEX_ROOT=x` line.

Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

Thanks for digging in here. This looks fine.

As noted in the review feedback - having an API consumer is terrifying. PEX is not properly setup for use as a library API as noted in #584. As such, I'd like to block on unit test being added that help enforce the idea this is depended upon functionality.

pex/variables.py Outdated Show resolved Hide resolved
pex/pex_bootstrapper.py Outdated Show resolved Hide resolved
pex/pex_bootstrapper.py Outdated Show resolved Hide resolved

# Override env settings using extra passed-in vars
if extra_vars:
if isinstance(extra_vars, dict):
Copy link
Member

Choose a reason for hiding this comment

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

How about just accepting **extra_vars and pushing json back to the user that needs it. It's terrifying that we have a consumer of this API at all, and as a relunctant / conservative library maintainer I'd much rather keep API surface area minimal.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not totally sure what you mean here, would you mind clarifying?

Are you saying this should only allow properly formatted json here, and require the user to convert from a dict if necessary?

Copy link
Author

@rtkjliviero rtkjliviero Aug 21, 2019

Choose a reason for hiding this comment

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

@jsirois, if you could clarify I'm more than happy to update this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I just mean support the minimal api, which is allowing the user to pass environment variables via **extra_vars. If code that call this wants to support a json encoding of the env vars that code can decode the json to a dict to pass in as **extra_vars.

Concreteley, just:

def _bootstrap(entry_point, include_pexrc=False, **extra_vars):
  from .pex_info import PexInfo
  pex_info = PexInfo.from_pex(entry_point)
  env = dict(PEX_IGNORE_RCFILES=str(not include_pexrc), **extra_vars)
  pex_info.update(PexInfo.from_env(env))

Calling code in an application can do this if it want to unburdening Pex from supporting json in this API in perpetuity:

_bootstrap(entry_point, **json.loads(extra_vars_json_string))

And, of course, where this is really important is in the public API function, so the **extra_vars style should propagate up to bootstrap_pex_env.

@rtkjliviero rtkjliviero force-pushed the include-pexrc-in-bootstrap-env branch from 6de33e2 to a6ccc62 Compare August 21, 2019 15:03
@rtkjliviero
Copy link
Author

Added some unit tests, as per

I'd like to block on unit test being added that help enforce the idea this is depended upon functionality.

I still need to add unit tests for considering pexrc files, but that requires some input from the maintainer. I'd like to put back my previous changes (DEFAULT_PEXRC_LOCATIONS, 7451a62), so that we can just mock.patch that object and avoid polluting the test by pulling in any pexrc files that might already be on the file system. If that is acceptable to @jsirois, I'll go ahead!

@rtkjliviero
Copy link
Author

@jsirois, any input on the above?

Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

I'm sorry for the delay getting back to this. Hopefully I've clarified the issue of acception dict or json. By accepting only a dict, Pex gains freedom without hindering a user from envoding thier env vars however they wish and then decoding to a dict before calling the PEX API.


# Override env settings using extra passed-in vars
if extra_vars:
if isinstance(extra_vars, dict):
Copy link
Member

Choose a reason for hiding this comment

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

I just mean support the minimal api, which is allowing the user to pass environment variables via **extra_vars. If code that call this wants to support a json encoding of the env vars that code can decode the json to a dict to pass in as **extra_vars.

Concreteley, just:

def _bootstrap(entry_point, include_pexrc=False, **extra_vars):
  from .pex_info import PexInfo
  pex_info = PexInfo.from_pex(entry_point)
  env = dict(PEX_IGNORE_RCFILES=str(not include_pexrc), **extra_vars)
  pex_info.update(PexInfo.from_env(env))

Calling code in an application can do this if it want to unburdening Pex from supporting json in this API in perpetuity:

_bootstrap(entry_point, **json.loads(extra_vars_json_string))

And, of course, where this is really important is in the public API function, so the **extra_vars style should propagate up to bootstrap_pex_env.

Base automatically changed from master to main March 18, 2021 21:23
@jsirois
Copy link
Member

jsirois commented Jul 11, 2022

@rtkjliviero thanks for working through things here. Your work is not in vain and got me thinking hard about how to solve this problem more generally in a way that does not require coding against internal PEX APIs.

I have a branch that should obviate the need to use the PEX bootstrap API like this. It allows you to place a PEX on the sys.path, say via PYTHONPATH, and activate it with a single import __pex__ early in your code or else directly using __pex__ as a psuedo package like so: from __pex__ import thing.inside.the.pex. I think this should remove the need the need for things like Lambdex and other tools that work around PEX files not working like traditional Python zip archive sys.path entries. I'm going to close this PR since Pex has tried really hard to lock down its API to just the CLI and this was the last compelling reason this couldn't be done. I'll add you to that PR review.

@rtkjliviero
Copy link
Author

Thanks for the update @jsirois!

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