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

Fix different .cachepath initialization points for precompile load #45149

Merged
merged 1 commit into from
May 7, 2022

Conversation

Keno
Copy link
Member

@Keno Keno commented May 2, 2022

For quite some time I've been observing Revise randomly not pick
up changes to my packages. Usually I just write that off to Revise
getting into a bad state and restarting Julia fixes it. Today, I
got very annoyed by this again and decided to finally track it down.
What turns out to happen here is the packages in question are those
which:

A. Are being precompiled on load
B. Use Requires
C. Have an @require'd-dependency already loaded
D. The change to be revised is made in a file not loaded via
Requires.jl.

In this case the __init__ of the package triggers Requires,
which in turn calls back to Revise, which tries to start watching
the package. However, on the compilecache path (but not on the
path where we just find a pre-existing cache file), we used to
not set the .cachepath property of pkgorigins until after
the __init__ callbacks run, causing Revise not to be able
to see those files. Infuriatingly, restarting julia fixes this
because it just loads the .ji file that was just compiled.

Fix this by unifying the point at which the .cachepath is set,
always setting it just prior to the module initialization callback.

For quite some time I've been observing Revise randomly not pick
up changes to my packages. Usually I just write that off to Revise
getting into a bad state and restarting Julia fixes it. Today, I
got very annoyed by this again and decided to finally track it down.
What turns out to happen here is the packages in question are those
which:

A. Are being precompiled on load
B. Use Requires
C. Have an `@require`'d-dependency already loaded
D. The change to be revised is made in a file not loaded via
   Requires.jl.

In this case the `__init__` of the package triggers Requires,
which in turn calls back to Revise, which tries to start watching
the package. However, on the `compilecache` path (but not on the
path where we just find a pre-existing cache file), we used to
not set the .cachepath property of `pkgorigins` until after
the `__init__` callbacks run, causing Revise not to be able
to see those files. Infuriatingly, restarting julia fixes this
because it just loads the .ji file that was just compiled.

Fix this by unifying the point at which the .cachepath is set,
always setting it just prior to the module initialization callback.
@Keno Keno requested review from KristofferC and timholy May 2, 2022 08:04
@Keno Keno added the backport 1.8 Change should be backported to release-1.8 label May 2, 2022
Copy link
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me at least.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like this also might need to be dealt with on the compiled=false path too? I tried to unify that somewhat already (#43257), but it went mediocre.

@Keno
Copy link
Member Author

Keno commented May 7, 2022

It feels like this also might need to be dealt with on the compiled=false path too?

The particular revise issue was specific to .cachepath, which is not required for the compile=false path. I agree that it would be better to be very clear about what is initialized when and to make sure it's consistent to the greatest extent possible, but that's probably a larger change.

@Keno Keno merged commit 58ab4c7 into master May 7, 2022
@Keno Keno deleted the kf/reviserage branch May 7, 2022 02:18
KristofferC pushed a commit that referenced this pull request May 16, 2022
…45149)

For quite some time I've been observing Revise randomly not pick
up changes to my packages. Usually I just write that off to Revise
getting into a bad state and restarting Julia fixes it. Today, I
got very annoyed by this again and decided to finally track it down.
What turns out to happen here is the packages in question are those
which:

A. Are being precompiled on load
B. Use Requires
C. Have an `@require`'d-dependency already loaded
D. The change to be revised is made in a file not loaded via
   Requires.jl.

In this case the `__init__` of the package triggers Requires,
which in turn calls back to Revise, which tries to start watching
the package. However, on the `compilecache` path (but not on the
path where we just find a pre-existing cache file), we used to
not set the .cachepath property of `pkgorigins` until after
the `__init__` callbacks run, causing Revise not to be able
to see those files. Infuriatingly, restarting julia fixes this
because it just loads the .ji file that was just compiled.

Fix this by unifying the point at which the .cachepath is set,
always setting it just prior to the module initialization callback.

(cherry picked from commit 58ab4c7)
@KristofferC KristofferC mentioned this pull request May 16, 2022
67 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label May 26, 2022
@timholy
Copy link
Member

timholy commented Jun 26, 2022

Thanks for looking into this & fixing it!

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 this pull request may close these issues.

4 participants