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

Revise loaded in -E/-e for -i not updating definitions #202

Open
mortenpi opened this issue Oct 14, 2018 · 18 comments · May be fixed by #349
Open

Revise loaded in -E/-e for -i not updating definitions #202

mortenpi opened this issue Oct 14, 2018 · 18 comments · May be fixed by #349
Labels
pending pr There is a pending PR that may fix this

Comments

@mortenpi
Copy link
Contributor

mortenpi commented Oct 14, 2018

When loading a Julia script as follows:

julia -i -e 'using Revise; includet("script.jl")'

Revise will not update the function definitions etc. afterwards. The original version of the script gets loaded without issues though.

Tested with Revise master (dd6e970) on Julia 1.0.0 and the normal Revise workflow (using Revise; includet("script.jl") in the REPL) works without problems.

Edit: just a bit more debugging:

sh$ julia -i -e 'using Revise'
julia> includet("script.jl")

also fails (i.e. not updating), so it seems to be a problem with loading Revise in the -E/-e option.

@timholy
Copy link
Owner

timholy commented Dec 30, 2018

Seems to work on the master branch for me.

@mortenpi
Copy link
Contributor Author

I still see the issue with both Julia 1.0.3 and 1.2.0-DEV.74 on the Revise master. I'll see if I can put together a script + env to reliably reproduce this.

@mortenpi
Copy link
Contributor Author

mortenpi commented Jan 1, 2019

I've put together a restricted environment and an exact list of commands that should reproduce the issue. Since it is dependent on the REPL loop, I couldn't really make it a single script, but at least the Julia commands can be just pasted into the REPL all at once:

https://gist.github.com/mortenpi/c944f5affbff54b1ddb13a7c14945f02#file-mwe-txt

Just to note that I am running this on Linux, as it may make a difference.

@timholy
Copy link
Owner

timholy commented Jan 1, 2019

Thanks for the reproducer. I must have been fooled by the fact that I launch Revise from my startup file.

So, the problem is that with julia -i -e 'using Revise', Revise's __init__ gets called before the REPL launches. As a consequence, this line doesn't run. Your script will revise automatically if you preface it with

julia> Revise.steal_repl_backend(Base.active_repl_backend)

It's possible that one could @async all that logic, but I recently replaced a bunch of them (#220) because they were slowing down startup. What do you use this for? I'm trying to gauge how important it is, and whether to fix with code or with documentation.

@mortenpi
Copy link
Contributor Author

mortenpi commented Jan 3, 2019

I think I ran into this when I was working on some functions and I needed to restart Julia over and over again (I was probably updating type definitions). So I wanted to includet the script right there in the command line so that I would have a REPL with all my functions etc. right away.

But not being able to do that is a pretty minor inconvenience, so I think just documenting the workaround is fine if there is no good way to fix it at the moment. Would it be possible to make Revise print a warning if it is loaded in this way?

@mortenpi
Copy link
Contributor Author

I got hit with this again, but this time I figured out that I can just use the same solution as the docs recommend for startup.jl:

julia -i -e'atreplinit() do repl
    @eval using Revise
    @async Revise.wait_steal_repl_backend()
    @eval includet("script.jl")
end'

Would there be any way to detect and warn if Revise is not loaded properly? It could also be useful when someone naively puts just using Revise in startup.jl. But I guess it might not really be possible to define when Revise is loaded "properly" and it isn't?

@timholy
Copy link
Owner

timholy commented Apr 5, 2019

The concern with issuing a warning is that it would be intrusive for people who launch Revise in their startup.jl file but occasionally run a script with julia script.jl.

@mortenpi
Copy link
Contributor Author

mortenpi commented Apr 7, 2019

It should not be a problem if you follow the recommended way though? The atreplinit should never get called if you're running a script, right?

@timholy
Copy link
Owner

timholy commented Apr 8, 2019

Yes, good point. I'd be willing to give this a try. Would you do me a favor, though? Make this change

$ git diff
diff --git a/src/Revise.jl b/src/Revise.jl
index 49f670b..4d0841d 100644
--- a/src/Revise.jl
+++ b/src/Revise.jl
@@ -1034,6 +1034,8 @@ function __init__()
             steal_repl_backend(Base.active_repl_backend::REPL.REPLBackend)
         elseif isdefined(Main, :IJulia)
             Main.IJulia.push_preexecute_hook(revise)
+        else
+            @warn "Revise may not be appropriately configured, see documentation"
         end
         if isdefined(Main, :Atom)
             setup_atom(getfield(Main, :Atom)::Module)

and play with it a little while? If it doesn't cause you any problems then I could go with it.

@mortenpi
Copy link
Contributor Author

Hmm, this particular error will pop up when you just start julia and you have the atreplinit() logic in startup.jl.

Would it be possible to have the @async Revise.wait_steal_repl_backend() line in __init__()? Or would that just horribly break everything?

@timholy
Copy link
Owner

timholy commented Apr 10, 2019

Check out the call to steal_repl_backend from both __init__ and wait_steal_repl_backend.

@mortenpi
Copy link
Contributor Author

Hmm, I indeed didn't look at the code that closely. However, if I understand correctly, steal_repl_backend does not get called in __init__ unless you are doing using Revise in the REPL.

I am more or less just thinking out loud here at the moment. I am currently running Revise with the following change:

diff --git a/src/Revise.jl b/src/Revise.jl
index 49f670b..8aee247 100644
--- a/src/Revise.jl
+++ b/src/Revise.jl
@@ -1034,6 +1034,10 @@ function __init__()
             steal_repl_backend(Base.active_repl_backend::REPL.REPLBackend)
         elseif isdefined(Main, :IJulia)
             Main.IJulia.push_preexecute_hook(revise)
+       else
+           @info "Setting up @async task to steal backend" # debug
+           @async Revise.wait_steal_repl_backend()
         end
         if isdefined(Main, :Atom)
             setup_atom(getfield(Main, :Atom)::Module)

and nothing in the startup.jl. This fixes the original problem

$ julia -i -e'using Revise; includet("test.jl")'

as far as I can tell. It also seems to allow you to just have using Revise in startup.jl, instead of the atreplinit() stuff.

@mlhetland
Copy link
Contributor

Just as a data point, I ran into the same issue just now, trying to run a script to set things up before interactive use.

@timholy
Copy link
Owner

timholy commented May 1, 2019

If you don't discover any negatives to this, I'd welcome a PR.

@mlhetland
Copy link
Contributor

I've tried putting this into a makefile:

atreplinit() do repl
    @eval using Revise
    @async Revise.wait_steal_repl_backend()
    # some setup
    @eval using MyModule
end

As long as no recompilation is required upon starting, this works just fine, but otherwise I get the following error:

┌ Warning: REPL initialization failed, Revise is not in automatic mode. Call `revise()` manually.
└ @ Revise ~/.julia/dev/Revise/src/Revise.jl:993

I guess that's to be expected, given the timeout behavior in wait_steal_repl_backend (i.e., it waits at most 1 second). If I just add a long sleep to the @async call (or even wrap it in my own loop along the lines of wait_steal_repl_backend's), it works well enough, though I'm not sure if that's the best solution?

I guess an option could be to let the 20 limit for iter be a keyword argument (tries?) or something, and I could just supply a really high value.

@timholy
Copy link
Owner

timholy commented Sep 1, 2019

What if we couple the suggestion in #202 (comment) with a check of Base.isinteractive()? Does that solve everyone's problem?

mortenpi added a commit to mortenpi/Revise.jl that referenced this issue Sep 1, 2019
@mortenpi mortenpi linked a pull request Sep 1, 2019 that will close this issue
@mlhetland
Copy link
Contributor

I don't really know!-) Maybe? How would you use the check? Wait until it becomes true, or something? Or is it perhaps true from the beginning, as long as Julia knows it's "heading toward" a REPL (just not quite there yet)?

@timholy
Copy link
Owner

timholy commented Sep 3, 2019

Discussion may move to #349

@timholy timholy added the pending pr There is a pending PR that may fix this label Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending pr There is a pending PR that may fix this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants