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

Allow for reloading of python code changes in a repl or development session #9462

Closed
rmangi opened this issue Apr 3, 2020 · 10 comments · Fixed by #13178
Closed

Allow for reloading of python code changes in a repl or development session #9462

rmangi opened this issue Apr 3, 2020 · 10 comments · Fixed by #13178
Assignees

Comments

@rmangi
Copy link

rmangi commented Apr 3, 2020

One of the frequent complaints we get from developers in our pants environment is how long it takes to reload a repl or restart a service they're working on due to the chroot creation overhead. In a non-pants world we could use imp or ipython autoreload in a repl and the restart time for an api-server outside of pants is usually negligible. The feedback loop time is challenging.

Would it be possible to sync local code changes or even symlink to it from the chroot to enable something like this?

If this is actually something that's possible today and we're not aware of it that would be great too :)

Thanks!

@Eric-Arellano
Copy link
Contributor

Thanks for opening this, @rmangi. We're actively working on speeding up the performance of ./pants repl and ./pants run, particularly when using the V2 implementation.

Right now, the biggest issue is that a change to your source file will mean that we re-resolve all your 3rd party requirements, even though that's not necessary. This is our first priority to fix.

As mentioned on Slack, it's possible to have this auto-reload mechanism via the option --loop. Once we make these improvements to the repl goal, we expect auto-reloading to work peformantly.

@Eric-Arellano
Copy link
Contributor

Hi @rmangi, update that we made the first major performance enhancement in 1.27.0rc0 for the V2 implementation of repl. We no longer re-resolve requirements unless those have changed.

We don't have hot reloading working quite yet. If you make a change, you'll still need to close out of the REPL and restart it with ./pants repl, unfortunately. But, that should be much less painful than it was before, albeit still not ideal.

We do plan to put some time into getting hot reloading working properly. It's not an immediate priority, but it's something we're thinking about.

--

Let us know either on this issue or on Slack if you run into any issues with this! I don't recall if you were using the V2 implementation of REPL before? If not, we'd love to help you get that set up.

@rmangi
Copy link
Author

rmangi commented Apr 20, 2020

Geraet, we're using the v2 repl. We'll upgrade and see how it goes.

@rmangi
Copy link
Author

rmangi commented Apr 22, 2020

Hey Eric, we're actually not using the v2 repl yet, we'll get that going. I'm not on the pants slack but the rest of my team is and they say I should be too so I'll get on that. Thanks!

@Eric-Arellano
Copy link
Contributor

We'd love to have you join Slack! See https://pants.readme.io/docs/getting-help-1 for instructions on how to join.

@stuhood stuhood added this to the 2.0.0 important milestone Oct 26, 2020
@stuhood
Copy link
Sponsor Member

stuhood commented Oct 26, 2020

This also applies to Django runserver, and possibly also to jupyter. cc @benjyw

--loop (Pants' existing "watch" mode) does not immediately work for this usecase because it won't kill an InteractiveProcess or @_uncacheable rule (including @goal_rules) if a file has changed under it. In some cases it should... it's possible that goals/processes/nodes should be marked interruptible/restartable (which would split that property out of the @_uncachable decorator).

It's also possible that this is a user-by-user decision, rather than being goal/tool specific, such that we would have --loop and --loop-restart (or --watch/--watch-restart), with an override, but the default continuing to be that we don't restart a process until it has completed.

@butlern
Copy link

butlern commented Mar 1, 2021

👍 Just ran into this testing out pantsv2.

Our use case is as follows:

Developers want to run a local django server locally with pants run command which ultimatley calls django's runserver command. They want to make code changes and have the server restart automatically on detected code changes.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Mar 1, 2021

That is likely an extremely common use-case, and one we should definitely be able to support.

@stuhood stuhood self-assigned this Oct 7, 2021
@stuhood
Copy link
Sponsor Member

stuhood commented Oct 7, 2021

Sketching this out a bit, my thinking is that it is completely fine to interrupt a @goal_rule and any underlying process, as long as it hasn't had a side-effect (yet). That connects this issue to #10542: namely, we should track whether a @goal_rule has had a side-effect, and allow it to be interrupted as long as it has not.

That leaves only the problem of determining whether individual InteractiveProcesses are interruptible/side-effecting. My feeling is that to safely support the usecase of this ticket, it would be reasonable to require that users set a flag like:

pex_binary(
  ..,
  interruptible=True,
)

...to indicate that it is safe to interrupt the process. This would be propagated into a flag on InteractiveProcess (with a default of False: i.e., assuming by default that a process running in the foreground might have had a side-effect).

Usecases like test --debug and repl would set the InteractiveProcess flag explicitly: the former to True (since tests which usually run in a sandbox should always be safe to interrupt) and the latter probably to a repl-specific flag like repl --interruptible (which would default to False, since most of the time you wouldn't want your repl to die when underlying files had changed).

Thoughts?

@benjyw
Copy link
Sponsor Contributor

benjyw commented Oct 8, 2021

I think that sounds right. So the interruptible field on a target would be for the run goal?

stuhood added a commit that referenced this issue Oct 11, 2021
…d lift `SideEffecting` to an abstract class (#13199)

To prepare to support restarting `Graph` `Node`s up until they have had a side-effect (for #9462), this change:
* Splits the `Node::restartable` property out from `Node::cacheable`, to allow for dynamically indicating whether a `Node` can be restarted. 
* Renames `engine::core` to `engine::python`, both for clarity, and to avoid collisions in poorly behaved macros like `derivative`.
* Converts `SideEffecting` into an abstract class to help track when side-effects have been triggered.
stuhood added a commit that referenced this issue Oct 11, 2021
…change (#13178)

Adds `pex_binary(.., restartable=True)` and `repl --restartable` to optionally allow user processes to be restarted when files change. See `run_integration_test.py` for an example.

`InteractiveProcess` is used for any process that needs to run in the foreground, including those for `run`, and `repl`. To support restarting those processes, we make invoking an `InteractiveProcess` async, which allows it to (optionally) be restarted when files change.

To make `InteractiveProcess` async, we convert it into a rust-side `Intrinsic`, and introduce a new `Effect` awaitable type, which is used in place of `Get` when a type is `SideEffecting`. This also prepares the other `SideEffecting` param types to be converted to `Effect`s as well (in order to accomplish #10542).

Fixes #9462.
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 a pull request may close this issue.

5 participants