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

discourage packages from depending on Revise? #728

Closed
vtjnash opened this issue Jan 16, 2023 · 18 comments · Fixed by #731
Closed

discourage packages from depending on Revise? #728

vtjnash opened this issue Jan 16, 2023 · 18 comments · Fixed by #731

Comments

@vtjnash
Copy link
Collaborator

vtjnash commented Jan 16, 2023

There are packages embedding Revise as a dependency (https://juliahub.com/ui/Packages/Revise/M1Qoh/3.5.0?page=2). This seems like generally a user-hostile API design for these packages, since they are implicitly forcing the user to depend on Revise for a random subset of packages (whatever happens to get loaded after Revise happened to get loaded).

This also can show up as a bug during precompilation, since Revise starts tracking some files in the precompile process, which may lead to incorrect evaluation (attempting to load and evaluate files that should not be part of the package) and other similar issues.

I wonder if we should define __init__() = precompile(false) here, so that those packages are discouraged from depending on a package which makes that precompilation less reliable (this one)

@LilithHafner
Copy link
Contributor

Most of these packages either have very few users (< 5 downloads in the lsat month) or have a legitimate dependency on Revise.

 Row │ name                  downloads 
─────┼─────────────────────────────────
   1 │ Genie                      2332
   2 │ JET                        1061
   3 │ GenieAutoReload             203
   4 │ GenieFramework              200
   5 │ GenieDevTools               164
   6 │ PlutoLinks                  139
   7 │ Scruff                       38
   8 │ DataDrop                     28
   9 │ Books                        25
  10 │ FluxArchitectures            18
  11 │ OpticSim                     10
  12 │ ClimateMARGO                  7
  13 │ PowerModelsADA                7
  14 │ FinEtoolsVoxelMesher          6
  15 │ TopOpt                        5
  16 │ ADSeismic                     5

DataDrop and Scruff both depend on Revise but don't actually load it, so the dependency should have no impact on user experience or precompilation (except to unnecessarily download and precompile Revise.jl)

If Revise can't participate in precompilation, that is a separate bug but this would not be a great solution.

@timholy
Copy link
Owner

timholy commented Feb 1, 2023

I'm all in favor of expunging needless dependencies, but I don't understand why this package makes precompilation less reliable. And there are a few packages that build on Revise's functionality.

@vtjnash
Copy link
Collaborator Author

vtjnash commented Feb 1, 2023

Revise appears to start tracking changes immediately, which would result in miscompilation (or more likely eval errors) if any were detected. It seems to need some different cue then for what and when to watch for changes if users are going to try to precompile with it. I had assumed that Revise was the interactive package, while most of the utilities were implemented elsewhere (such as LoweredCodeUtils and CodeTracking). Whereas this package relies on some pretty heavy-weight and expensive interactive packages additionally (such as Pkg, REPL, JuliaInterpreter)

@timholy
Copy link
Owner

timholy commented Feb 1, 2023

I think I get it now: the issue is that Revise's __init__ starts some Tasks that watch certain files. Revise itself is fine because its __init__ doesn't run when it is being precompiled, but its __init__ does run (during precompilation) when it's a dependency for another package.

Presumably you'd like it if we wait until the "last package loaded" is the one that launches these Tasks? But isn't that a bit ugly?

@timholy
Copy link
Owner

timholy commented Feb 2, 2023

Personally I am leaning towards the view that the best solution is to revert JuliaLang/julia#46571 (sorry). There are genuine reasons packages need to start tasks in their __init__, and it's not crazy for those tasks to persist throughout the session.

@vtjnash
Copy link
Collaborator Author

vtjnash commented Feb 2, 2023

Maybe what Revise could do is check whether isdefined(Main, :Revise) && getfield(Main, :Revise) === Revise periodically (i.e. in the REPL statement callback hook). That way it avoids activating the special REPL behavior of tracking all packages loaded after Revise until the user requests it specifically in the Main module / at the REPL. This ensures that packages can interact with Revise to use the API. But does not simultaneously opt a randomized subsection of all other packages dependencies in the session into having Revise track them simply because they appeared later in the load order?

@timholy
Copy link
Owner

timholy commented Feb 2, 2023

There are workarounds for specific issues. But I'm mulling over the bigger picture. Suppose someone was worried about data loss from a task that periodically writes to disk. This user requested that we make it impossible to exit Julia unless all Tasks had finished. Would you agree with that proposal?

@KristofferC
Copy link
Collaborator

I personally think that the failure mode of JuliaLang/julia#46571 seems too harsh for these type of simple mistakes.

@vtjnash
Copy link
Collaborator Author

vtjnash commented Feb 2, 2023

@timholy You seem to be onto my plan for stage 2 of this haha. I had not been discussing it since I did not want to work out all of the details yet, but I believe it was briefly mentioned in the original PR. Yes, in the future I envision you can set a flag that defers normal program exit until the application has shutdown and cleaned up itself sufficiently. It would have to be opt-in, and you could still bypass it simply with any abnormal program exit code (non-zero).

Languages, like C and C++, already do this (by waiting for all destructors to run before any exit), so it should not be too surprising that we also are doing this more now too. Although the blend of exactly which things we cancel vs. wait for completion is very implementation specific, and we can still work to adjust those categorizations too.

@timholy
Copy link
Owner

timholy commented Feb 2, 2023

I don't pretend to understand this landscape as well as you do, but IMO Kristoffer's point is right: we're already in the territory where fears over correctness are making people's lives harder. Heck, until I sit down and spend half a day finally learning about pipes and other advanced I/O features, even I don't know how to fix JuliaDebug/Cthulhu.jl#344. And if someone like me has a worthy goal, "shave 6s off the startup time," but I'm stymied simply because I can't figure out how to clean up a Task nicely, I think we're entering dangerous territory.

@timholy
Copy link
Owner

timholy commented Feb 2, 2023

What about moving forward the

and you could still bypass it

and getting that done first?

@vtjnash
Copy link
Collaborator Author

vtjnash commented Feb 2, 2023

That is already done. Just call exit(1)

@timholy
Copy link
Owner

timholy commented Feb 2, 2023

module StuckPackge

Timer(0.0; interval=1) do _
    println("fire")
end

sleep(3)
exit(1)

end # module StuckPackge

gives

julia> using StuckPackge
[ Info: Precompiling StuckPackge [554d14fb-36ad-4f3e-a0a8-a984ff197c47]
fire
fire
fire
ERROR: Failed to precompile StuckPackge [554d14fb-36ad-4f3e-a0a8-a984ff197c47] to "/home/tim/.julia/compiled/v1.10/StuckPackge/jl_SzjmTs".
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] compilecache(pkg::Base.PkgId, path::String, internal_stderr::IO, internal_stdout::IO, keep_loaded_modules::Bool)
   @ Base ./loading.jl:2198
 [3] compilecache
   @ ./loading.jl:2071 [inlined]
 [4] _require(pkg::Base.PkgId, env::String)
   @ Base ./loading.jl:1715
 [5] _require_prelocked(uuidkey::Base.PkgId, env::String)
   @ Base ./loading.jl:1570
 [6] macro expansion
   @ ./loading.jl:1558 [inlined]
 [7] macro expansion
   @ ./lock.jl:267 [inlined]
 [8] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:1521

@LilithHafner
Copy link
Contributor

The ongoing discussion is not Revise.jl specific. I'd like to move it to JuliaLang/julia#48506

@vtjnash
Copy link
Collaborator Author

vtjnash commented Feb 3, 2023

PkgEval had previously indicated that Revise was the only (remaining) affected package.

@essenciary
Copy link

The approach that the users should set up Revise as part of their startup.jl file is not beginner friendly. I went through a huge amount of work to provide seamless Revise support in Genie so the users would get code-reload out of the box. I was getting significant support queries about it given that hot reload is the norm in all modern web frameworks and programming languages.

Many people have experience with other languages and web frameworks and just want to try building a quick app in a few lines of code before committing to spending more time to learn Julia (even so, many don't bother reading the manual).

In addition, these days people don't necessarily go through the trouble of setting up a full programming environment to test Julia. If I want to try a language I just spin up something like Repl.it and test some demos. These kind of environments make configuring startup.jl hard or impossible.

@vtjnash
Copy link
Collaborator Author

vtjnash commented Feb 3, 2023

Yes, that makes sense. Packages like Genie and JET are using the API here in explicit ways to build upon it, so they are sensible consumers of this package, and we should not break that. The trouble is they don't necessarily want to also opt the user into the magic __init__-driven behavior for all packages loaded later. Particularly during precompile (and to a lesser extent after precompile), those REPL hooks are not desirable to enable right away however. So my updated proposal is to only enable the magic package-loaded callback hooks after both the REPL is running and the user has explicitly loaded it into the REPL module / Main (or once a downstream package enables it explicitly based on its own conditions, such as managing a REPL of its own). #728 (comment)

LilithHafner added a commit to LilithHafner/Revise.jl that referenced this issue Feb 3, 2023
This might be a very bad idea. Tries to fix timholy#728
@timholy
Copy link
Owner

timholy commented Feb 3, 2023

PkgEval had previously indicated that Revise was the only (remaining) affected package.

I didn't agree with that at the time. Of course it is weird that a package like Cthulhu didn't show up.

The approach that the users should set up Revise as part of their startup.jl file is not beginner friendly

@essenciary can you open a separate issue about configuration? And any thoughts you have about an alternative? (Meaning, the obvious alternative is to not tell people how to run Revise by default and let them figure that out by themselves, but I'm not sure that's what you mean.)

timholy pushed a commit that referenced this issue Feb 3, 2023
This might be a very bad idea. Tries to fix #728
jcampolongo pushed a commit to charles-river-analytics/Scruff.jl that referenced this issue Feb 10, 2023
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