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

Use max compatible Python interpreter rather than minimum #7048

Closed

Conversation

Eric-Arellano
Copy link
Contributor

Problem

When running ./pants3, we would expect to run subprocesses like Pytest using Python 3 as well. Instead, we resort to using Python 2—as it's the minimum acceptable—unless --python-setup-interpreter-constraints is explicitly set to require Python 3.

One workaround to this issue would be setting PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS to only allow Python 3, but this would mean that ./pants3 could no longer be used to run targets that require Python 2 like Antlr.

Further, we in general want to write Python 3-first code. Defaulting to the minimum interpreter possible does not coincide with this objective.

How to get to prior behavior

Users may go back to using the minimum interpreter through several ways:

  • temporarily via the --python-setup-interpreter-constraints flag
  • temporarily via PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS
  • globally via pants.ini and compatibility
  • at the target level via BUILD files and compatibility

Implications

  • CI's Python 3 shards will use Python 3.7, not 3.6
  • Once we merge Allow Pants to run with Python 3 via ./pants3 script #6959, Pants and CI will default to using Python 3 for subprocesses instead of Python 2—even when using Python 2 under-the-hood. This is intentional. We can use the compatibility mechanisms to force Python 2 (e.g. CI will use PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS to get our Py2 shards to use Python 2).
  • New Python major releases will potentially cause issues for Pants users if their code is not ready. For example, if they run brew upgrade when 3.8 comes out, and used our internal compatibility setting of >=2.7,<3 or >=3.6,<4 and have issues like not using collections.abc, then they will get a SyntaxError. Keep in mind this only happens one a year, though, when a new version is released. Further, it is easy to constrain their compatibility to no longer allow 3.8.

On macOS, if the interpreter is not available when running tests, then it throws an OSError for file not found. This is currently not being caught as intended.
@jsirois
Copy link
Contributor

jsirois commented Jan 9, 2019

This is the opposite of pex behavior and I can only imagine this makes confusing interpreter selection choices that much more confusing. Is this really needed? It just supports our internal dev efforts afaict.

One workaround to this issue would be setting PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS to only allow Python 3, but this would mean that ./pants3 could no longer be used to run targets that require Python 2 like Antlr.

This bit sounds wrong / broken and is what should be fixed imo.

@Eric-Arellano
Copy link
Contributor Author

Closing in favor of simply using PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS when running ./pants3, to avoid conflicting with PEX behavior.

As discussed with John over Slack, this may lead to a compatibility issue when trying to run the Antlr tests in CI. If this does become a problem and we can't resolve it otherwise, we'll reconsider this solution.

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jan 13, 2019
There is a matrix of what interpreter we use for under-the-hood and what we use for subprocesses. Presumably, when calling `./pants3`, you would expect to also run your tests and subprocesses with Py3.

This change is not only nice-to-have but also required because the native engine does not work when bootstrapped with Python 3 and running a Python 2 subprocess *if* that subprocess uses `native_engine.so`, as several of our tests do.

This may result in conflicts when trying to run Py2-only targets, e.g. python_antlr_library.py. If this is the case, we can resort to pantsbuild#7048 as an alternative solution.
@Eric-Arellano Eric-Arellano deleted the max-interpreter branch June 20, 2019 02:39
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