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

feat: provide access to arbitrary interpreters #2507

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

philsc
Copy link
Contributor

@philsc philsc commented Dec 16, 2024

There are some use cases that folks want to cover here. They are
discussed in this Slack thread. The high-level summary is:

  1. Users want to run the exact same interpreter that Bazel is running
    to minimize environmental issues.
  2. It is useful to pass a target label to third-party tools like mypy
    so that they can use the correct interpreter.

This patch adds to @rickeylev's work from #2359 by adding docs
and a few integration tests.

rickeylev and others added 10 commits October 29, 2024 21:05
Run a specific interpreter:
* `bazel run @rules_python//tools/run --@rules_python//python/config_settings:python_version=3.12`

Run interpreter from a binary:
* `bazel run @rules_python//tools/run --@rules_python//tools/run:bin=//my:binary`
    $ bazel run //python/bin:repl
    $ bazel run //python/bin:repl --//python/bin:repl_dep=//python/runfiles
`@rules_python//python/bin:interpreter_src` target.

```
$ bazel run @rules_python//python/bin:interpreter --@rules_python//python/bin:interpreter_src=//path/to:bin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to find an "aha!" example for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using the publishing twine binary? Something that exists within rules_python could do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but I am looking for something that would produce different results from the default. AFAICT if I leave out --@rules_python//python/bin:interpreter_src=//path/to:bin then it'll default to the default Python toolchain. Since the twine would also use the default Python toolchain, it wouldn't really change the outcome.

@rickeylev , could you provide insight here? What use case are you covering with pulling the toolchain from a py_binary. I suspect I'm missing something simple here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The case I had in mind is a versioned py_binary. In such a case, the binary may have a different version from the top-level config.

Copy link
Collaborator

Choose a reason for hiding this comment

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

dev_pip//sphinx is available for 3.11 and 3.13.0, we could use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look at those, thank you.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

I know this is still a draft, so take my suggestions with a grain of salt :)

docs/toolchains.md Outdated Show resolved Hide resolved
python/bin/interpreter.bzl Outdated Show resolved Hide resolved
`@rules_python//python/bin:interpreter_src` target.

```
$ bazel run @rules_python//python/bin:interpreter --@rules_python//python/bin:interpreter_src=//path/to:bin
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using the publishing twine binary? Something that exists within rules_python could do the trick.

self._run_module_test("3.11")

def test_run_module_3_12(self):
self._run_module_test("3.12")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to add a test that imports:

  • The code that is in the //python/bin:interpreter_src
  • The code that is in the transitive deps of the //python/bin:interpreter_src. Maybe depending on twine could be an option in the tests and then we could attempt importing one of its deps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm misunderstanding something, that's currently not possible with the current implementation. The current implementation is purely there to get the interpreter, nothing else.

I'd like to follow this PR up with another PR that enables the use of the REPL with a specific py_* target (and its dependencies) importable.

docs/toolchains.md Outdated Show resolved Hide resolved
@@ -120,6 +120,11 @@ rules_python_integration_test(
py_main = "custom_commands_test.py",
)

rules_python_integration_test(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a bazel-in-bazel integration test, use transitions to set the appropriate state. The BiB tests are slow and painful to run and debug. There is some code in //tests/support:sh_py_run_test.bzl that has most of this already you should be able to use with some modifications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but figured most use cases involve a bazel run //python/bin:python so I wanted to make sure that that's tested. But I can see the argument about it being annoying etc. Will change it.

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.

3 participants