-
-
Notifications
You must be signed in to change notification settings - Fork 575
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: Add support for REPLs #2723
base: main
Are you sure you want to change the base?
Conversation
This patch adds three different ways to invoke a REPL for a given target, each with slightly unique use cases. Deployed binaries: Sometimes it's really useful to start the REPL for a binary that's already deployed in a docker container. You can do this with the `RULES_PYTHON_BOOTSTRAP_REPL` environment variable. For example: $ RULES_PYTHON_BOOTSTRAP_REPL=1 bazel run --//python/config_settings:bootstrap_impl=script //tools:wheelmaker Python 3.11.1 (main, Jan 16 2023, 22:41:20) [Clang 15.0.7 ] on linux Type "help", "copyright", "credits" or "license" for more information. (InteractiveConsole) >>> import tools.wheelmaker >>> `py_{library,binary,test}` targets: These targets will now auto-generate additional `<name>.repl` targets. $ bazel run --//python/config_settings:bootstrap_impl=script //python/runfiles:runfiles.repl Python 3.11.1 (main, Jan 16 2023, 22:41:20) [Clang 15.0.7 ] on linux Type "help", "copyright", "credits" or "license" for more information. (InteractiveConsole) >>> import python.runfiles >>> Arbitrary `PyInfo` providers: Spawn a REPL for any target that provides `PyInfo` like this: $ bazel run --//python/config_settings:bootstrap_impl=script //python/bin:repl --//python/bin:repl_dep=//tools:wheelmaker Python 3.11.1 (main, Jan 16 2023, 22:41:20) [Clang 15.0.7 ] on linux Type "help", "copyright", "credits" or "license" for more information. (InteractiveConsole) >>> import tools.wheelmaker >>>
Broadly in favor of this, had a similar macro feature aligned with the verbs pattern at a former employer. I don't love having magical behavior that runs ipython if it's on the path, IMO an ipython shell or any other notebook-like thing is a different kind of target than a vanilla shell and we shouldn't overload the one with try-import behavior which is generally an antipattern. IMO the kind of shell to run should be explicitly selected either by having separate targets or by having a feature flag we can While REPL is the more familiar term to me from the Lisp world, the Python ecosystem appears to prefer the words "shell" or "console". My previous macros emitted |
What is the difference between 2 and 3? Could we just get away with 3? |
I generally agree, but everywhere I've implemented this (3 places), one of the first questions is "how do I get ipython?". So I've just given up on that front and made these automatically use that. I can be convinced to drop it, but I suspect without it, folks are just going to implement it themselves which reduces the value of having it in a shared place.
Perhaps we could add an additional label_flag to let the user specify a .py file that invokes the actual shell (to replace |
The difference is convenience. I have found that folks picked up the "bazel run this library with .repl at the end" a whole lot easier and they even taught it to other folks. |
Hmm. Perhaps I'm old 😛 REPL does appear in the docs: https://docs.python.org/3/tutorial/appendix.html#tut-interac
And there's even an environment variable for configuration: I'm happy to change it to ".shell" or similar if you all want, but I don't think REPL is un-Pythonic so to speak. |
One extra question about what is the difference between your Should we have the approach where we:
|
Can you elaborate on this a bit? Is the use case to do something like The goal with this PR here is to create a REPL in an environment identical to what you would see when running your code in a
You don't get that with only the interpreter, even if it could import the deps. I think if we can figure out how to move all those things into a pre-generated venv, then that could work. But it's not obvious to me that we can do that at this time.
Removed the env var in the bootstrap script and the macro stuff for future PRs. |
The concrete usecase is that I am using Right now I am hacking this around by having a import subprocess
import sys
if __name__ == "__main__":
subprocess.run([sys.executable] + sys.argv[1:]) # noqa: S603 This basically re-execs the interpreter from the environment with any parameters that may be passed to the target. If I run this target I get the If on the other hand I pass the My question here is:
|
My 2c here. I would prefer to have this in user-space. We have a similar need at work, and we added a small macro to create an ipython py_console_script_binary with the dependencies. (It occurs to me with the new This deals with most of the concerns listed above?
Im worried about adding more complexity into the toolchains and bootstrapping areas at the moment. If your main motivation is convenience for users, it's unclear why you couldn't create some user facing macros in your repository (automated or not via gazelle) that wrap the native py_library and py_binary targets with an ipython repl and targetnames of your choosing? |
I am curious if having the
I agree with @groodt here that running I personally liked the already existing target |
This patch adds three different ways to invoke a REPL for a given
target, each with slightly unique use cases.
Deployed binaries: Sometimes it's really useful to start the REPL
for a binary that's already deployed in a docker container. You can do
this with the
RULES_PYTHON_BOOTSTRAP_REPL
environment variable. Forexample:
py_{library,binary,test}
targets: These targets will nowauto-generate additional
<name>.repl
targets.Arbitrary
PyInfo
providers: Spawn a REPL for any target thatprovides
PyInfo
like this: