Support user-specified callbacks

[tkluck]

This feature is one half of what Revise.entr already does. The function entr allows the user to install callbacks for when a file or module changes, and also enters a blocking loop that catches any errors that might arise from the callbacks.

This new add_callback function allows installing callbacks and keeping them around in the background. This commit also reimplements entr in terms of add_callback and remove_callback.

(Basically, add_callback now does what I thought entr would do after seeing its signature.)

I'm hoping to use this in HAML.jl for updating generated code when its source file changes. It may be similarly useful for hot code reloading in web servers, etc.

[timholy]

Just to see if I understand, the goal here is to allow more granular control: the ability to control behavior when errors are thrown, and to remove callbacks from the queue? If so, I'm all in favor.

However, this failed tests on every platform but Linux. One possible fix is another sleep(mtimedelay) before writing "def".

[tkluck]

Just to see if I understand, the goal here is to allow more granular control: the ability to control behavior when errors are thrown, and to remove callbacks from the queue? If so, I'm all in favor.

Exactly. Happy that you're in favour, I'll look into the test failures then!

[codecov]

Codecov Report

Merging #443 into master will decrease coverage by 1.25%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #443      +/-   ##
==========================================
- Coverage   81.88%   80.62%   -1.26%     
==========================================
  Files          12       12              
  Lines        1551     1667     +116     
==========================================
+ Hits         1270     1344      +74     
- Misses        281      323      +42

Impacted Files	Coverage Δ
src/pkgs.jl	88.85% <100%> (+0.03%)	⬆️
src/Revise.jl	73.62% <90.74%> (-1.07%)	⬇️
src/recipes.jl	88.07% <0%> (-1.84%)	⬇️
src/utils.jl	57.14% <0%> (-1.1%)	⬇️
src/types.jl	60.34% <0%> (+1.59%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 373d4e5...5eebd46. Read the comment docs.

[timholy]

I'm going to cycle CI on this and see how it does now that #450 is merged. There's a failure on 1.0 that I don't understand.

[tkluck]

Still some CI failures on non-linux platforms. It's obviously important to support those as well.

Do you have any best practices that might help me tackle this? Myself, I'd be tempted to replace all reliance on sleep by adding a small API that creates a Base.Event that fires exactly when a certain change has been noticed by revise. But maybe/hopefully that's overkill and there's a better way?

[timholy]

The entr tests have been a persistent low-grade problem, and no one has yet come up with a fix. If you've made the failure reliable, that's actually quite encouraging. Event-based handling seems like cheating as no one will use events in their normal workflow. Since test failures occur for entr and nowhere else, it suggests that a localized fix should be all that is required.

In the past it has been observed that magic dust occasionally helps:

Revise.jl/src/Revise.jl

Line 904 in 836d261

yield() # magic bug fix for the OSX test failures. TODO: figure out why this works (prob. Julia bug)

.

Would someone with a mac or windows machine check out this branch and see if they can figure out the problem? I've been thinking @mlhetland should review this anyway.

[mlhetland]

Hm. Just trying out the test more manually, my first observation is that the callback is not triggered unless the file is closed. However, if you (e.g.) replace flush(io) with close(io), it seems to work just fine. Maybe that's acceptable? Or perhaps unavoidable? After all, that's how a text editor would behave, I guess? (I.e., a save would involve opening, writing and then closing the file?)

I'm not sure if the callback is supposed to be triggered automatically; it's only triggered once I call revise, but I suppose that's normal outside entr?

[tkluck]

Hm. Just trying out the test more manually, my first observation is that the callback is not triggered unless the file is closed. However, if you (e.g.) replace flush(io) with close(io), it seems to work just fine. Maybe that's acceptable? Or perhaps unavoidable? After all, that's how a text editor would behave, I guess? (I.e., a save would involve opening, writing and then closing the file?)

That's a great point. I pushed a commit that hopefully fixes it in the way you describe.

I'm not sure if the callback is supposed to be triggered automatically; it's only triggered once I call revise, but I suppose that's normal outside entr?

Yes, that's the idea of these callbacks. If someone wants them to be called "asap" then they don't need Revise but just FileWatching + a background task. But using Revise ensures that the callbacks executes when the interactive user expects it.

My personal use case is loading custom template code in HAML.jl, but one might also imagine using this as (untested):

const COUPLING_CONSTANTS = Ref(read_json("coupling_constants.json"))
Revise.add_callback(["coupling_constants.json"]) do
    COUPLING_CONSTANTS[] = read_json("coupling_constants.json")
end

Then the user can be sure that the coupling constants don't change in mid-computation.

[timholy]

For some reason Travis doesn't seem to be reporting back (not sure whether that's a Travis problem or a GitHub problem), but you can monitor it by navigating to https://travis-ci.org/github/timholy/Revise.jl, clicking on "More options," choosing "Requests," and then clicking on the build number for the most recent request. Looking very promising! I expect failure on non-Linux nightly due to JuliaLang/julia#35297, so that's nothing to worry about.

[timholy]

Now that we're passing tests, here's a more serious review. Perhaps @mlhetland will have other specific comments.

[timholy]

It only recently became safe to call track more than once on the same file, so this seems OK. But, under what circumstances would these not already be tracked?

[tkluck]

I don't actually see any reason why they would already be tracked? What if someone does Revise.add_callback([], Base) do .... ? I'm probably missing something here.

[timholy]

I guess I'm reacting to the fact that you're pulling pre-existing files out of the pkgdata and adding them to files. Tracking would be on if you'd said using MyPkg and then called this later with add_callback(..., modules=(MyPkg,),...). Stated differently, if they're already in pkgdata how are they not tracked?

Perhaps I don't understand the purpose, and a comment would be helpful.

[tkluck]

Ah, now I see what you mean! Yes, this is faulty code then -- I'm trying to guard for the fact that the module may not be under tracking at all yet. Under that logic, we should first call track(mod) and only then read the data from pkgdata, right?

If that's the case, I should also add a doctest that tests for this correct behaviour.

[timholy]

Any time you've said using MyPkg it should be tracked. Though perhaps the watch_package callback (see how it gets registered in __init__) has to fire first. Is that the issue?

I think I've make track safe for re-entry (see #433), but I tend to be jittery (probably, needlessly) about things getting done twice and messing up the internal accounting. Maybe poke at this a bit first and try to define more clearly what circumstances require this?

[tkluck]

Any time you've said using MyPkg it should be tracked.

The two things that I have in mind are:

• packages loaded before Revise was loaded, e.g.

using MyPackage
using Revise
Revise.add_callback([], [MyPackage]) do ....

• packages in the system image (e.g. Base or anything the user added in PackageCompiler).

The first one could potentially be tested by spawning another Julia process. The second one seems almost impossible to test. But in both cases, calling Revise.track(<module>) before reading pkgdata seems like it's the right thing to do -- what do you think?

I wasn't thinking about a race condition related to when watch_package is being called; do you think we should?

[timholy]

packages loaded before Revise was loaded

If package precompilation were required for all modules, I could support that, but the existence (even in principle) of non-precompiled packages makes robust support for such a feature impossible. The problematic use case is loading the package, making some changes to the source, and then tracking with Revise: in this case Revise has no way of knowing that what's in the files does not match the code in your running session. Conversely, if you load Revise first, it "defensively" parses the entire package when it's loaded and then it has a baseline to compare any file changes. This is not necessary for precompied packages because I added a source-cache to *.ji files in JuliaLang/julia#23898, and did the same for Julia itself in JuliaLang/julia#24120. This means that you can always recover the state of the source at the time at which the *.ji file was built. But there is no such fix available for non-precompiled packages.

So I plan to support this feature by saying "load Revise first, otherwise you're screwed" 😄. More seriously, I could contemplate supporting this specifically for precompiled packages, but we'd need to enforce it.

packages in the system image (e.g. Base or anything the user added in PackageCompiler).

That's a much more important use-case. Do such files record their source text? If not, we're screwed for now but should fix that ASAP.

But in both cases, calling Revise.track() before reading pkgdata seems like it's the right thing to do -- what do you think?

Yeah, track populates the structure and then you can do stuff with it.

[tkluck]

@ffevotte this is the thing that needs resolving: in add_callback we need to move the call to Revise.track(mod) to the place before reading pkgdatas[id]. We don't have to call Revise.track(mod, absname) after that.

After that change, hopefully this is done.

[tkluck]

Looks like tests are passing on all platforms now 😀 There's only an unrelated line-number issue on Julia nightly. Anything I can still improve?

[mlhetland]

I haven't done a thorough review, but I've looked at the code, and it looks good to me! I also think this make the whole idea of callbacks a more natural part of Revise; entr on its own might seem like more of an outlier (as witnessed by @timholy's initial suggestion of placing it in a separate package).

I guess we might consider renaming entr as well (through a deprecation cycle, or just keeping the old name as a shortcut), somehow reflecting how it's now more of a callback + file monitoring, rather than an atomic piece of functionality. I originally took the name from the entr project (where it's short for Event Notify Test Runner… 🤷‍♂). Of course, using that name – while behaving similarly to that utility – makes the functionality potentially more predictable to those who know of it, but I'm not sure that's a weighty argument.

[tkluck]

Thanks for adding your comments @mlhetland ! I also understand your point about the naming: I was one of the users that didn't know about the entr project so I was a bit confused about it 😅

I have no real opinion on whether we should also rename or add an alias to entr but I do agree with your reasoning.

[timholy]

This seems basically ready to merge, maybe just a couple tweaks?

[timholy]

I guess I'm reacting to the fact that you're pulling pre-existing files out of the pkgdata and adding them to files. Tracking would be on if you'd said using MyPkg and then called this later with add_callback(..., modules=(MyPkg,),...). Stated differently, if they're already in pkgdata how are they not tracked?

Perhaps I don't understand the purpose, and a comment would be helpful.

[ffevotte]

FYI, it looks like this PR also fixes #481, for which I submitted a (much less ambitious) fix in #482

Looking at what's proposed here, I do think that my PR should be ignored in favor of this proposal. Please do not hesitate to ask me if I can help in any way. And thanks for everything!

[ffevotte]

And for the record, it looks like this PR also fixes #470 by not relying on FileWatching.watch_folder anymore.

[timholy]

I'd love to see this merged. @ffevotte, perhaps @tkluck would like some help pushing it over the finish line?

[tkluck]

Yes, I'd appreciate the help! Sorry for not finishing + pushing this sooner -- I just started at a new employer and need to jump through a few legal hoops before I can continue contributing to open source again. Shouldn't be a problem but it takes some time. So feel free to beat me to it and finish this one :)

[ffevotte]

OK, I'll try to re-read this discussion, paying more attention to details and trying to determine what's missing for this to get merged.

But if the next steps are clear in your mind and you can spend a few minutes listing them here, I'd greatly appreciate it!

[tkluck]

@ffevotte thank you! Just tagged you in the conversation that has an actionable.

[ffevotte]

OK, I started working on this, but am not sure how to proceed to add commits to this PR. Should I make a PR from my working branch (https://github.com/ffevotte/Revise.jl/tree/tkluck/support-callbacks) to this one?

In any case, here is what I have so far:

1. ffevotte/Revise.jl@9233aa9: the last required change, as I understand it
2. ffevotte/Revise.jl@2fadde7: some additional docstrings, in the hope that they can help future contributors (or my future self) better understand the implementation of this feature
3. ffevotte/Revise.jl@50a05c3: merge from the current master and conflict resolution. There have been significant changes in master since this branch has been created, which caused several conflicts. In particular, I'm not sure I correctly handled the new version of revise_file_queued, especially because of conflicts related to 9c580e4:
   • l.662: file deletions seem to not be explicitly processed anymore by Revise, but I still think we should notify entr here.
   • l.679: I'm not sure I fully understand the logic here, but I think no notification is needed here, since sending notifications to files watched by entr should have already been taken care of a few lines above

@timholy @tkluck I think it would be best if you could carefully review this: I fear I'm too new to the Revise codebase to be 100% sure of what I did; I might very well have made a few mistakes (especially regarding that last merge...)

(Tests seem to pass on my system (Linux), but revise_file_queued does not seem to be called on this platform)

[timholy]

Thanks @ffevotte! Submitting a fresh PR that preserves authorship would be good. I suspect we're not going to want the full version history, so perhaps squashing all of @tkluck's into a single commit, and then squashing your own changes into a different commit right before we merge your PR makes the most sense.

I like your changes. I think the file deletion is actually OK, with the new watching logic the only part of it that it has to cover (?) is whether it should keep watching the file/directory. But since there's no CI for BSD, revise_file_queued is not well tested, perhaps we should do a separate run on Travis with watching_files[] = true. No BSD-users have complained, though...

[ffevotte]

Thanks for the review! Is something like #488 what you had in mind?

[tkluck]

Closing this one now #488 has been merged. Thank you so much @ffevotte !