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

Add support for passthrough args to the python repl #3423

Closed
stuhood opened this issue May 12, 2016 · 6 comments · Fixed by #19858
Closed

Add support for passthrough args to the python repl #3423

stuhood opened this issue May 12, 2016 · 6 comments · Fixed by #19858
Labels
backend: Python Python backend-related issues good first issue

Comments

@stuhood
Copy link
Sponsor Member

stuhood commented May 12, 2016

In particular, the ipython repl supports CLI arguments like:

ipython notebook

which launches a visual repl.

Supporting passthrough args would mean that:

./pants repl.py --ipython $target -- notebook $file

would work (assuming you added the relevant 'notebook' deps to --ipython-requirements)


To do this, you would enable passthru args on the PythonRepl task by overriding the supports_passthru_args property to True (example), and then pass any additional self.get_passthru_args() as args while launching the repl.

You could then add either a unit or integration test for the passthru.

@mistermocha
Copy link

Thanks @stuhood

@Eric-Arellano
Copy link
Contributor

I believe this works with both the v1 and v2 implementations. See https://pants.readme.io/docs/python-repl-goal.

@Eric-Arellano
Copy link
Contributor

Oh, nevermind. Confused with run. Reopening.

@gauthamnair
Copy link
Contributor

I am running into an issue here that is making this less straightforward than it seemed, and we need to choose which approach to take out of it.

The questions are:

  1. external: should pass-through work for all supported repls? Currently these are ipython, python and scala.
  2. internal: can the implementation of pass-through be repl-specific or should we try hard to stick to a goal-level implementation as we have for run?

Supporting all repls

Getting ipython to work is simple and #19858 does so. This was the original user's question (many many years ago).

Getting pass-through to work with the python shell will require some additional work. We make a python repl by executing a pex without an entry-point or script and rely on pex's emulation of the behavior you normally get when running python regarding options and interactivity. The emulation has a few issues (pex-tool/pex#2248, pex-tool/pex#2249) and I think it will be very tricky to get a pex to faithfully recreate all of python's command line behavior.

On the other hand, anyone planning to do significant work in the repl would probably be using ipython for a python project.

For scala it probably won't run into the same issues with passing command line flags, but we have to validate that they will work, and the way we run that repl:

args=[
    *jdk.args(bash, tool_classpath.classpath_entries(), chroot="{chroot}"),
    "-Dscala.usejavacp=true",
    scala_artifacts.repl_main,
    "-classpath",
    ":".join(user_classpath.args(prefix=user_classpath_prefix)),
],

I am not sure that tacking on pass-through args at the end will meet all the user's expectations from the scala CLI application. Is it too late to set a -D system property or change jvm limits with -J-X? Maybe, but probably it will work with the -i arg. And there's the issue that anyone planning to do significant repl work in scala would probably switch to ammonite.

goal-level vs tool-level impl

A goal-level impl would mirror what run does and is very straightforward if we are supporting passthrough for all repls (Looks like this). This is the way the run goal works.

Goal level is trickier if we do not support all repls. To be honest to the user we would probably need to add some kind of field in each ReplImplementation saying whether or not it supports pass-through and warn/error if they supply passthrough args to a repl that can't handle it.

A tool-level impl would look more like what we do with PyTest. Here the tool subsystem has its own passthrough args and the setup pytest rule makes use of those args to construct the generic Process request.

For us it would mean that the IPython subsystem would have its own args and we'd read and install them in the args field of the generic ReplRequest just as is done in this commit.

This is cleaner if not all repls will support pass-through.

My current inclinations

Without further input I would lean to just support it for ipython and wait to see if there is demand for getting it to work for scala. The implementation would be tool-level. Requesting your feedback.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Sep 22, 2023

Thanks for doing the deep dive on this! Ideally we would support all repls, but if this is tricky, then I think having a goal-level solution, as in #19858 and then erroring for repls that don't support passthrough args is fine! We can then work through those repls and add the support if possible.

@gauthamnair
Copy link
Contributor

Great, I'll add bits to #19858 so that it errors for repls that do not have proven support just yet.

stuhood pushed a commit that referenced this issue Sep 27, 2023
Fixes #3423

As discussed in
#3423 (comment)
we are implementing args at the goal-level but with a caveat that it is
only available for some shells. In this PR, we only enable it for
ipython. The repl will error out otherwise.

---------

Co-authored-by: Benjy Weinberger <benjyw@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants