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

Do not track files during pre-compilation #731

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

LilithHafner
Copy link
Contributor

This might be a very bad idea. Tries to fix #728

This might be a very bad idea. Tries to fix timholy#728
@timholy
Copy link
Owner

timholy commented Feb 3, 2023

Yep, this might be necessary eventually, but let's wait for resolution of JuliaLang/julia#48506 first. No point making changes in a specific package when we should settle the language itself first.

@vtjnash
Copy link
Collaborator

vtjnash commented Feb 3, 2023

This seems best to me. It would be exceptionally disconcerting to me for a precompile process to detect and react to file system changes with some random probability depending on the performance of the filesystem. It might then attempt to call eval, causing the task to error because it was not in the right module, or to arbitrarily attempt to change the current module (again, causing the task to error or miscompile, since replacing methods is undefined behavior).

@timholy
Copy link
Owner

timholy commented Feb 3, 2023

Alright, I'll merge this and release a new version. My prediction, though, is that there are going to be more independent packages failing because of JuliaLang/julia#48506. Merging this will indeed help us discover things that are Revise-independent.

@timholy timholy merged commit 4eb14a8 into timholy:master Feb 3, 2023
@LilithHafner LilithHafner deleted the patch-1 branch February 3, 2023 20:08
@vtjnash
Copy link
Collaborator

vtjnash commented Feb 3, 2023

My remaining question–that is not related to JuliaLang/julia#48506 but inspired by it–is the intended interpretation of this statement in the docs:

julia> using Revise # you must do this before loading any revisable packages

at https://github.com/timholy/Revise.jl/blob/master/docs/src/cookbook.md#pkgtemplates

Is intended as a behavior that occurs from the arbitrary moment when Revise gets __init__ called, or is intended as a literal instruction for the user to type at the REPL or startup.jl file? The former interpretation means that we could not include this as a default stdlib in the sysimg, since it is always "active" and cannot be avoided or turned off. This similarly also means that when loaded transitively (e.g. by Genie), it will watch a random subset of Genie's dependencies as well as Genie itself–whatever got loaded after the __init__ ran, which may be different from run to run (depending on the order in which we iterated some hash tables).

But the later interpretation means there needs to be a created an action that implies a stateful user intent to enable it broadly, and to be a trigger that is not usually serialized (so that it has to be explicitly enabled for each current session). For example, I think this could check whether Main.Revise === Revise, but there could be other detection triggers too, such as detecting the REPL being defined after Revise.__init__ ran.

Finally, one other alternative option is that Revise should instead start to watch every package, outside of the sysimg, regardless of the order in which Revise was loaded.

Do those questions and thoughts make sense?

@timholy
Copy link
Owner

timholy commented Feb 3, 2023

Finally, one other alternative option is that Revise should instead start to watch every package, outside of the sysimg, regardless of the order in which Revise was loaded.

I think this is best, and probably achievable once we started caching source text in .ji files.

@vtjnash
Copy link
Collaborator

vtjnash commented Feb 4, 2023

Do we not do that caching already?

@timholy
Copy link
Owner

timholy commented Feb 4, 2023

Yes. I was unclear: "was probably achievable..."

alhirzel added a commit to alhirzel/PythonPlot.jl that referenced this pull request Apr 20, 2023
Use Revise.jl's "trick" that disables __init__() when precompiling.

See: timholy/Revise.jl#731
alhirzel added a commit to alhirzel/PyPlot.jl that referenced this pull request Apr 21, 2023
Use Revise.jl's "trick" that disables __init__() when precompiling.

See: timholy/Revise.jl#731
stevengj pushed a commit to JuliaPy/PyPlot.jl that referenced this pull request Apr 21, 2023
Use Revise.jl's "trick" that disables __init__() when precompiling.

See: timholy/Revise.jl#731
stevengj pushed a commit to JuliaPy/PythonPlot.jl that referenced this pull request Apr 21, 2023
Use Revise.jl's "trick" that disables __init__() when precompiling.

See: timholy/Revise.jl#731
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.

discourage packages from depending on Revise?
3 participants