Remove exception catching for __init__ functions, add InitError exception type

[jdlangs]

This PR is to continue the discussion from #12505 with a concrete implementation candidate. Sometime later this week I am interested to try running the package evaluator to see what level of breakage it causes due to people not bothering to fix warnings.

@JeffBezanson @StefanKarpinski @vtjnash

[stevengj]

+1. I don't think that code in __init__ should be semantically different (in the absence of precompilation) from the same code pasted at the end of your module.

[ivarne]

Marking this as 0.4.0 as this is a breaking change, that should get merged before feature freeze.

[StefanKarpinski]

Indeed, I think this needs to get resolved.

[ScottPJones]

👍

[ivarne]

If it should behave like the same code at the end of a module, we need to catch the exception and wrap it in a LoadError or a new InitError (to store a reference to the failing module, and different printing).

[ScottPJones]

@ivarne That sounds good. The principle here is that errors should not be swept under the rug (printing something to the terminal just doesn't cut it when you have lots of non-interactive processes anyway, and can be dangerous if the output device is been redirected to something else)

[StefanKarpinski]

I'm still confused by why we would ever be ignoring errors in __init__ in the first place. Can you spell that out, @vtjnash?

[vtjnash]

Jeff did the implementation of this. I think the basic assumption was that a failure in a __init__ may be non-critical for the state of the rest of the system. And if you ran:

try
  import Foo
  nofoo = !isdefined(current_module(), :Foo) # necessary since failure to define Foo isn't an error
catch e
  nofoo = true
end

and Foo did stuff like have multiple submodules with __init__ and loading other modules, you want all of the unrelated ones to get a chance to finish running.

As some food for thought, it may not be correct to propagate errors for code loaded from conditional modules (#6884) since that code is not run until after the matching module.

[stevengj]

@ivarne, I agree with wrapping it in a LoadError to make it like other exceptions thrown by module-body code. @phobon, this means keeping the try-catch block, but putting something like the following in the catch block:

jl_rethrow_other(jl_new_struct(jl_loaderror_type, f->linfo->file->name, f->linfo->line, jl_exception_in_transit));

or maybe defining a new InitError type as @ivarne suggested.

[jdlangs]

It would definitely be nice to have InitError so that "expected" problems were detectable. I can easily see someone trying to import a module, seeing if its initializer fails (e.g., because of a missing library) and proceed to do something else. Of course, this would also require allowing import/using within try (at global scope only) but for now you can do it with try reload(:BadMod) catch end.

I should have some time this evening to update the branch.

[jdlangs]

Updated to add a new InitError type. Note that exceptions from __init__ were being caught and put into a LoadError already when it was coming from a file. This checks specifically for a thrown InitError and rethrows it in that case. Would appreciate feedback on the implementation there.

[stevengj]

Looks good. Maybe add

@test_throws InitError include_string("module TestInitError\n__init__() = error(\"foo\")\nend")

to test/core.jl?

[stevengj]

Probably you should change precompilableerror(ex::LoadError, c) in base/loading.jl to

precompilableerror(ex::Union{LoadError,InitError}, c) = precompilableerror(ex.error, c)

to cover the (unlikely) event that the user calls __precompile(false)__ from their __init__ function instead of the module body.

[jakebolewski]

I feel this should be:

showerror(io::IO, ex::InitError) = print(io, "InitError: during initialization of module $(ex.mod"))

the backtrace will already be appended

[stevengj]

@jakebolewski, It seems like @phobon's code is correct; it should be analogous to showerror(io::IO, ex::LoadError), and you need to show ex.error at minimum, and you need to pass the backtrace through in case ex.error has a special backtrace handler.

[vtjnash]

to cover the (unlikely) event that the user calls precompile(false) from their init function instead of the module body.

the precompilation has already finished and saved the file by then, thus it seems like a bad place to be trying to assert !precompiled, unless there is some reason to conditionally permit usage of the precompiled module?

[stevengj]

@vtjnash, I agree that we normally shouldn't encourage people to call __precompile__ in __init__. I was just going by the principle that code in __init__ should be functionally equivalent to code pasted at the end of the module. But I'm fine with omitting it here.

[jdlangs]

@stevengj, currently if you define a module at the REPL, exceptions do not get wrapped in a LoadError. Would it be desirable to also not wrap __init__ exceptions in that case? Or is it the current non-wrapping behavior that should be changed?

[stevengj]

@phobon, as I understand it, LoadError is only something that gets thrown by include or similar; it has nothing to do with whether the included file defines a module or not. I don't think we want to throw LoadErrors inside a single file, and so I think it is fine not to do so for things evaluated at the REPL.

On the other hand, __init__ is specific to modules, and I think it makes sense to wrap __init__ exceptions in InitError no matter how the module is created.

[vtjnash]

i think you need a try/catch/rethrow at https://github.com/phobon/julia/blob/ddd476b1b7e5c3af0d34432109d37693604b11c6/src/toplevel.c#L208-L215 to clear the reinit list after an error to avoid leaving it in an invalid state

[jdlangs]

@vtjnash, I'm not completely sure I know what "clear the reinit list" entails. Do these lines do the job?

JL_TRY {
    size_t i, l=module_stack.len;
    for(i = stackidx; i < l; i++) {
        jl_module_load_time_initialize((jl_module_t*)module_stack.items[i]);
    }
    assert(module_stack.len == l);
    module_stack.len = stackidx;
}
JL_CATCH {
    arraylist_free(&module_stack);
    initialized = 0;
    jl_rethrow();
}

Is the same concern relevant for restoring compiled modules as well?

[vtjnash]

clear it by resetting the top pointer: module_stack.len = stackidx;

precompiled modules have their own array, so i don't think they should be a problem

[ivarne]

Not sure if this is useful for other cases, but this change kind of begs for abstract WrappedException <: Exception

[jdlangs]

I had thought about it, but it did seem like a pretty small corner case. I suppose there's no harm in future-proofing though.

[jdlangs]

I did have some interest in running this branch on the package evaluator but started to hesitate digging into when I saw it would require some hacking. Is there some fork or branch where someone has already set up this capability?

And just saw the merge went through...

[jdlangs]

Glad to see this resolved, thanks for the feedback everyone!

[jakebolewski]

Thanks @phobon, great work.

[tkelman]

it would require some hacking

Not much? It's a few clicks on the buildbots by anyone who has a login to make a custom generic binary from any branch or PR, then should be only one or two lines to change in PackageEvaluator to point it to a different download.

[jakebolewski]

It will be great when this can be done just by pinging a bot from Github.

[tkelman]

Webhooks aren't hard to write. Just a matter of hardware to run it on and plumbing it together.

[jakebolewski]

Hopefully soon (end of the month) we will have the hardware.

[jdlangs]

It would be nice to enable PackageEvaluator to check out and build an arbitrary branch so anyone without buildbot access could run it locally, or maybe on their own cloud computing account.

[tkelman]

Recent incarnations of PackageEvaluator don't build from source any more, but the buildbots are good at that. You can run make binary-dist from anywhere and the product should run fine on an equivalent distro, but to make generic binaries that run across many different distributions requires some work and custom setup.

[jdlangs]

That's fine, but will they be freely usable by anyone who wants? If not, then in the interest of simplicity it would seem useful to provide an option to PackageEvaluator that would have it also build julia from source.

[IainNZ]

@phobon I'd accept a pull request for that. I'd be a very short PR, its a really simple system.

[tkelman]

will they be freely usable by anyone who wants?

Custom builds from the buildbots get uploaded to https://s3.amazonaws.com/julianightlies/ with a sha, just like normal nightlies. They'll stay up there for roughly 3-4 weeks. In fact the releases of 0.3 have been made that way, just manually moving the files over from julianightlies to julialang once a month.

[tkelman]

Note that this change is really bad for embedding. Now when any dependency libraries aren't present, Julia can't start at all instead of being just unable to use the functionality from that library.

[ivarne]

In that case the library should warn("Missing X dependency ") instead of error("Missing X dependency ")

[jdlangs]

Or it should be possible to put the import or using call in a try block.

[ivarne]

If we actually manage to agree on something to move out of Base in #5155, we wouldn't need to __init__() so many unused modules at startup time.

[tkelman]

PCRE, GMP, dSFMT, openblas, and suitesparse_wrapper all need to be turned into warnings instead of InitError when they are not present, to get a REPL to start.

[vtjnash]

Or pull them out of base? dSFMT adds significantly to Julia's startup time (also openblas, but less so)

[tkelman]

Yes, but not right now. We need to make the lack of their presence non-fatal right now, otherwise 0.4.0 will be substantially less usable for embedders.

[ScottPJones]

Hopefully pulling them out of base will be done early in 0.5, but this change is already in 0.4, so it needs to be dealt with somehow now.

[JeffBezanson]

It's really weird to print something if the exception isn't being handled. If you tried to trap and hide all of these exceptions, you would still get empty lines scrolling in stderr.

EDIT: I see this is after a rethrow so I guess it's just dead code.

[JeffBezanson]

@tkelman Very good point. All I can think of at the moment is to wrap those specific Base __init__ methods in try..catch to avoid the breakage.

It's also quite likely I was thinking of this issue when I put the try-catch in there originally. Before __init__ existed, we had the property that loading a module and never using it was unlikely to cause problems. With __init__, you are forced to run code in the module.

[jdlangs]

To be continued in #12742 ...