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

Turn PythonInterpreterCache into a subsystem #6765

Merged
merged 1 commit into from Nov 27, 2018

Conversation

Projects
None yet
2 participants
@benjyw
Copy link
Contributor

benjyw commented Nov 13, 2018

It effectively depended on other subsystems (expecting instances to be passed into it), so it's a natural fit.

Switches it to use a local logger instead of one passed in from task context. The logger was only used for debug output anyway, and we want all such output to go through standard logging in the future anyway.

Also simplifies the interface of PythonInterpreterCache.setup(). Previously you could pass a list of paths to it, overriding its internal logic. However this was only used in tests, which can easily be converted to use the appropriate option instead.

This is the first step in simplifying the path-selection logic to be entirely option-based, instead of the current chain of ors that can't be fully overridden in options.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Nov 13, 2018

I'm not sure of your plans here, but pantsbuild/pex#607 will kill the interpreter cache in Pants soonish. Just fair warning.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Nov 13, 2018

Ah, my changes are more concerned with the base interpreters themselves and how they're located on the local system. My reading of pantsbuild/pex#607 is that this would still need to happen...?

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Nov 13, 2018

Ah, yes. The changes in pantsbuild/pex#607 will obviate "setting up" the interpreters after they are found. The finding still needs to happen.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Nov 13, 2018

Or is the plan to install python interpreters as needed? Say using pyenv.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Nov 13, 2018

Or is the plan to install python interpreters as needed? Say using pyenv.

That would be wonderful but nigh on impossible. It just doesn't work on some systems too often to maintain from what I've observed.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Nov 14, 2018

So the next change (the one that this change is intended to pave the way for) is to fix this logic:

setup_paths = (self.pex_python_paths()

Right now it's opaque and rigid - you can have exactly one of these three possibilities, and it's not obvious how to even get the one you want.

Instead, I want all paths to be set via the interpreter_search_paths option, with special symbols for pex_python_paths and for the PATH.

Then a third change will add the ability to easily use pyenv's interpreters (presumably via some other special symbol).

@benjyw benjyw requested review from jsirois and CMLivingston Nov 14, 2018

self._python_setup = python_setup
self._python_repos = python_repos
def __init__(self, python_setup=None, python_repos=None, logger=None):
self._python_setup = python_setup or PythonSetup.global_instance()

This comment has been minimized.

@jsirois

jsirois Nov 14, 2018

Member

How about making PythonInterpreterCache a subsystem that has deps on PythonSetup and PythonRepos? This is an accident waiting to happen.

This comment has been minimized.

@benjyw

benjyw Nov 14, 2018

Contributor

Could do. How to pass the logger in becomes a little tricky, but in practice (outside of tests) we always use a logger that is a thin wrapper around the global RunTracker, which is itself a subsystem, so...

This comment has been minimized.

@jsirois

jsirois Nov 14, 2018

Member

Perhaps just using a logging logger? If we don't wire up logging <-> reporting we probably should be to allow for lighter weight decentralized logging that respects pants logging config once its online and initialized.

This comment has been minimized.

@benjyw

benjyw Nov 14, 2018

Contributor

See #6771, which I think is a win even disregarding this change here.

This comment has been minimized.

@benjyw

benjyw Nov 14, 2018

Contributor

We probably should wire up logging, but that's more of a yak shave than I want to go down right now. I'm sure there are nuances that will ripple...

This comment has been minimized.

@jsirois

jsirois Nov 15, 2018

Member

I think #6771 is great. That said, I was more pointing out what this says more cleanly:

$ git grep "import logging" src/python/ | wc -l
93

I suspect you'll want to wait on #6771 since you went through the effort, but a good deal of non-task code just uses logging and this could too.

@benjyw benjyw force-pushed the benjyw:interpreter_cache_fixes branch from 6b79db3 to a32e1c1 Nov 19, 2018

@benjyw benjyw changed the title Simplify the signature of PythonInterpreterCache.setup() Turn PythonInterpreterCache into a subsystem Nov 19, 2018

@benjyw benjyw force-pushed the benjyw:interpreter_cache_fixes branch 2 times, most recently from 5d0ef46 to 53fa49f Nov 19, 2018

@benjyw benjyw force-pushed the benjyw:interpreter_cache_fixes branch from 53fa49f to c7b84f5 Nov 27, 2018

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Nov 27, 2018

Hi, this is green now, so PTAL when you can. Thanks!

@benjyw benjyw merged commit 765f2d8 into pantsbuild:master Nov 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:interpreter_cache_fixes branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment