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

include_dependency files that are in a different depot from the package are not found during loading #52161

Closed
KristofferC opened this issue Nov 14, 2023 · 27 comments · Fixed by #52750
Labels
compiler:precompilation Precompilation of modules
Milestone

Comments

@KristofferC
Copy link
Member

This is a MWE from a regression in an internal package. The end goal in that package is to cache some arbitrary piece of code using Julia's precompile system so that code can be loaded again quickly.

To show the issue, let's create a precompiled module (here in a single file) that we want to be invalidated based on some other file changing. This file could potentially be in a package depot (where it probably won't be changed) but we might also have that dependency file be in a devved package where it could be changed and we then want to recompile the module:

# Foo.jl
module Foo

using Example

readmefile = joinpath(pkgdir(Example), "README.md")
@show readmefile
include_dependency(readmefile)

end

Now, let's say we do not want to pollute our standard depot with cache files from this module (and for some reason we also want to store this module file somewhere else) so we do:

new_depot = joinpath(tempdir(), "depot")
mkpath(new_depot)
pushfirst!(DEPOT_PATH, new_depot)
pushfirst!(LOAD_PATH, new_depot)
foofile = joinpath(new_depot, "Foo.jl")
if !isfile(foofile)
    cp("Foo.jl", foofile)
end
using Foo

Upon running this we can see the path to the dependency file we injected:

julia> using Foo
┌ Info: Precompiling Foo [top-level]
└ @ Base loading.jl:2442
readmefile = "/Users/kristoffercarlsson/.julia/packages/Example/aqsx3/README.md"```

However, when looking what is actually stored in the precompile file, the path to the README file is different:

cachefile = joinpath(DEPOT_PATH[1], "compiled", "v1.11", "Foo.ji")
io = open(cachefile, "r")
Base.isvalid_cache_header(io)
modules, (includes, _, requires), _ = Base.parse_cache_header(io, cachefile)
includes

julia> includes
2-element Vector{Base.CacheHeaderIncludes}:
 Base.CacheHeaderIncludes( [top-level], "/var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/depot/Foo.jl", 0x0000000000000084, 0xd614849c, -1.0, String[])
 Base.CacheHeaderIncludes( [top-level], "/var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/depot/packages/Example/aqsx3/README.md", 0x0000000000000000, 0x00000000, 1.6745681561065354e9, String[])

The README.md file is stored with the location /var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/depot/packages/Example/aqsx3/README.md" which is not the same as what we actually used in the include_dependency call.

The issue with that is that when we load this module again, it will not find this file and recompile it:

julia> using Foo
┌ Debug: Rejecting stale cache file /var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/depot/compiled/v1.11/Foo.ji because file /var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/depot/packages/Example/aqsx3/README.md does not exist
└ @ Base loading.jl:3289
Info: Precompiling Foo [top-level

My guess to what is happening is that after #49866 the precompile system checks all file that are in a depot path and "reifies" them to be the same as that of the package getting loaded. But this fails in cases like this.

It would be good to have some solution to this because this use case is quite useful.

cc @fatteneder

@KristofferC KristofferC added this to the 1.11 milestone Nov 14, 2023
@KristofferC
Copy link
Member Author

Note that the

pushfirst!(LOAD_PATH, new_depot)
foofile = joinpath(new_depot, "Foo.jl")
if !isfile(foofile)
    cp("Foo.jl", foofile)
end

part is not really needed, if one does

pushfirst!(LOAD_PATH, ".")
using Foo

It still fails to load:

julia> using Foo
┌ Debug: Missing @depot tag for include dependencies in cache file /Users/kristoffercarlsson/.julia/compiled/v1.11/Foo.ji.
│   srcfiles = Set(["/Users/kristoffercarlsson/JuliaTests/Foo.jl"])
└ @ Base loading.jl:2727
┌ Debug: Rejecting stale cache file /Users/kristoffercarlsson/.julia/compiled/v1.11/Foo.ji because its depot could not be resolved

@fatteneder
Copy link
Member

I think you are right that #49866 introduced this regression. IIUC then the issue is here

jl_value_t **replace_depot_args;
JL_GC_PUSHARGS(replace_depot_args, 2);
replace_depot_args[0] = replace_depot_func;
replace_depot_args[1] = abspath;
ct = jl_current_task;
size_t last_age = ct->world_age;
ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
jl_value_t *depalias = (jl_value_t*)jl_apply(replace_depot_args, 2);
ct->world_age = last_age;
JL_GC_POP();
where we now do the depot path replacement for all paths.
Assuming this is correct, then the fix is easy.

Allowing include_dependencys to be relocatable can be useful too.
So I guess we should add a kw relocatable::Bool=true to include_dependency.
Or would you like it to default to false?

@fatteneder
Copy link
Member

fatteneder commented Nov 14, 2023

Thinking about it again, I am not sure if making include_dependencys relocatable can be done easily.

What happens in the above example is this:

  1. During serialization we insert the placeholder @depot to convert any path into a relative one and store that in the .ji and .so files. (include_dependency paths only go into .so). But the placeholder is only inserted when the file is located on a DEPOT_PATH, and this happens here, because the README.md is part of a installed package. Note that if Example where not on the DEPOT_PATH, we would have serialized an abspath for the readme and things would have just worked when loading (I think).
  2. During loading we replace the @depot tags by the first depot in which we find all included files for the pkg. This search does not include the include_dependencys. But when we find a suitable depot path we replace also the placeholders in the include_dependency paths of the pkg. For the above case, we now turned a relative path into an abspath that points to the wrong location.

In summary, this is just a rephrasing of your earlier comment:

My guess to what is happening is that after https://github.com/JuliaLang/julia/pull/49866 the precompile system checks all file that are in a depot path and "reifies" them to be the same as that of the package getting loaded. But this fails in cases like this.

It seems that it is too easy to find the wrong path for include_dependencys when loading, in particular, because the so tracked paths frequently don't lie in the pkg's root.

@stevengj
Copy link
Member

stevengj commented Nov 15, 2023

This search does not include the include_dependencys

The initial goal for include_dependency was that it should be treated just like include except that it doesn't load and interpret the file. So if subsequent development treated include_dependency paths differently, that seems undesirable to me.

It seems that it is too easy to find the wrong path for include_dependencys when loading, in particular, because the so tracked paths frequently don't lie in the pkg's root.

To me this is weird — normally, include_dependency should be for files that are "non-Julia source files" and hence lie within the package root, like any other source files.

Indeed, I did a search of packages using include_dependency, and every example I found was for files within the package root. In particular, there were two examples that were particularly common:

  1. Packages doing include_dependency("../Project.toml") so that they are recompiled whenever Project.toml changes
  2. Packages loading docstrings from a .md file, most commonly ../README.md for the module docstring.

(There were a handful of other use cases, e.g. packages loading global const arrays and other module data from data files so that you don't have a "wall of text" in source code. Or NBInclude.jl lets you use Jupyter notebook files as Julia source files, which requires include_dependency to manage cache invalidation. But all of the other examples I found were still files within the package tree.)

@stevengj stevengj added the compiler:precompilation Precompilation of modules label Nov 15, 2023
@vchuravy
Copy link
Member

vchuravy commented Nov 15, 2023

include_dependency paths only go into .so

That surprises me. The .ji file should have all the information that is needed, and the .so is a duplication for validation purposes.

This search does not include the include_dependencys

Maybe we should change that? I guess the only question is file's in scratch spaces.

@fatteneder
Copy link
Member

fatteneder commented Nov 15, 2023

That surprises me. The .ji file should have all the information that is needed, and the .so is a duplication for validation purposes.

I have to say I am still not 100% familiar with the precompile code and so I could be wrong too. But IIUC then src/precompile.c is responsible for writing the .ji files. And there is a comment in jl_write_srctext saying that we skip writing include_dependencies:

julia/src/precompile.c

Lines 48 to 50 in 221f074

// Dependencies declared with `include_dependency` are excluded
// because these may not be Julia code (and could be huge)
if (depmod != (jl_value_t*)jl_main_module) {

Inside that if is where the path is then written.

Maybe we should change that? I guess the only question is file's in scratch spaces.

I remember that initially I did not skip them and this gave me problems.
But now it does not make sense why I decided to skip but then later use that path also for replacement ...
I need to look into this again to better understand it.

@fatteneder
Copy link
Member

fatteneder commented Nov 15, 2023

It seems that it is too easy to find the wrong path for include_dependencys when loading, in particular, because the so tracked paths frequently don't lie in the pkg's root.

To me this is weird — normally, include_dependency should be for files that are "non-Julia source files" and hence lie within the package root, like any other source files.

You are right. I should not have added 'frequently' to my statement.
However, it does come up sometimes, e.g. in this issue and I definitely have seen it happening when working on the relocation PR, but then thought I solved it ...

And I think I remember reading a discussion in a julia PR related to something like CUDA or GPUCompiler and where something like a driver or so is tracked with a include_dependency and this might be a system path.
But I can't find this discussion in my history (probably because I did not add anything).
Or I am just going crazy at this point
I think I have a fever dream and I am confused now.

@fatteneder
Copy link
Member

Putting my confusion aside, I think there are two things actionable

  1. Also respect include_dependencys when determining the depot we want to restore. This avoids the ambiguity with finding the wrong files in different depots.
  2. Add a relocatable keyword. Default should be true, as it seems to be the default use case according to julia-hub.

Did I miss something?

@vchuravy
Copy link
Member

I think I have a fever dream and I am confused now.

No you didn't, CUDA does indeed do that: https://github.com/JuliaBinaryWrappers/CUDA_Runtime_jll.jl/blob/d7e700c623a51b93ef2421233f1dbc97d8de5904/.pkg/platform_augmentation.jl#L79

@fatteneder
Copy link
Member

oh, right, it was in the jll, which doesn't show up in the juliahub search -- puhhh, saved

@KristofferC
Copy link
Member Author

KristofferC commented Nov 16, 2023

To me this is weird — normally, include_dependency should be for files that are "non-Julia source files" and hence lie within the package root, like any other source files.

I agree that the way include_dependency is used here is non-standard so I will elaborate a little bit on the use case:

  • The only real way to cache parts of the Julia compiler pipeline is via package (or module) precompilation.
  • There are cases where you want (dynamically) cache parts of the compiler pipeline without having to create a standalone package. This could for example be if you package itself is some kind of "compiler" that takes input (e.g. in form of a "model file") and outputs optimized (or "processed") code to run that model.
  • An idea of doing this that I came up with (originally for add a load_model function and use it to allow loading multiple versions of models simultaneously bankofcanada/ModelBaseEcon.jl#45) is the following:
    • Based on some input file, wrap the content of that file in a module, run the "compiler" on it, add the needed include_dependency to make it invalidate when the input files are changed, stuff it somewhere, set up the LOAD_PATH and DEPOT_PATH, load that module which will cause it to precompile.
    • When the user give the same input file gain, the precompile file will be valid and the compilation that was earlier done is reused.

The input files (or at least auxiliary files) could be part of a package which is in a depot. We noticed this not working properly anymore when this custom made module had to precompile every time it was loaded and I found out it was due to the rewriting of the include_dependency path.

@KristofferC
Copy link
Member Author

KristofferC commented Nov 16, 2023

One possible way to solve this is to search through all DEPOT_PATHs for include_dependeny files (that assumes they are stored with the @depot/ path)?

@staticfloat
Copy link
Member

I think the central issue here, (from my perspective) is that there is now a requirement that all source files are found within the same depot. We didn't use to have this requirement, and I'm not sure why the requirement exists now. Could we just break that requirement and say that a cache file is loadable if all of the required files can be found in any depot, rather than in a single depot?

@fatteneder
Copy link
Member

My previous example did not make sense, hence, I deleted it.

@KristofferC
Copy link
Member Author

If we start searching at the DEPOT of the package file itself, then there shouldn't be any performance loss in the standard case I think?

@staticfloat
Copy link
Member

To speak directly to the issue of versioning (which I think your deleted example was about), I think that because we have the package version slugs (and artifact hashes) in the filenames, we are unlikely to run into issues with cross-contamination of files from different versions.

@fatteneder
Copy link
Member

If we start searching at the DEPOT of the package file itself, then there shouldn't be any performance loss in the standard case I think?

If the standard case is that pkg and cache are in the same depot, then yes.

@KristofferC
Copy link
Member Author

KristofferC commented Nov 20, 2023

One question, in the first post, the files are stored as:

julia> includes
2-element Vector{Base.CacheHeaderIncludes}:
 Base.CacheHeaderIncludes( [top-level], "/var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/depot/Foo.jl", 0x0000000000000084, 0xd614849c, -1.0, String[])
 Base.CacheHeaderIncludes( [top-level], "/var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/depot/packages/Example/aqsx3/README.md", 0x0000000000000000, 0x00000000, 1.6745681561065354e9, String[])

Where in the code is the absolute path

"/Users/kristoffercarlsson/.julia/packages/Example/aqsx3/README.md"

rewritten into

"/var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/depot/packages/Example/aqsx3/README.md", 

?

To me, we have failed to identify a @depot here (since the path to Foo.jl does not follow the depot format (it is not inside packages)) but still it seems that the include_dependency path gets rewritten as if assumed that everything is inside a depot (but not really since then it would use @depot...).

It feels like there should never be a case where a path gets rewritten into some other absolute path.

@fatteneder
Copy link
Member

To answer:

Where in the code is the absolute path ... rewritten into ... ?

The rewriting happens in two steps:

  • The absolute path is converted into a relative one when precompiling. We insert the @depot with this function https://github.com/JuliaLang/julia/blob/72cd63ce28c50c8c72e009df03dfec608802450e/base/loading.jl#L2639C50-L2654 which is called from two places on the C side: src/precompile.c and src/staticdata_utils.c. You can check the outputted .ji files; they should contain paths that start with @depot. (however, the include_dependency path only appears inside the .so file, and that is binary ...)
  • On loading a the relative @depot path is converted to a (wrong) absolute path. We find and restore the depot path with these functions:

    julia/base/loading.jl

    Lines 2656 to 2676 in 72cd63c

    function restore_depot_path(path::AbstractString, depot::AbstractString)
    replace(path, r"^@depot" => depot; count=1)
    end
    # Find depot in DEPOT_PATH for which all @depot tags from the `includes`
    # can be replaced so that they point to a file on disk each.
    function resolve_depot(includes::Union{AbstractVector,AbstractSet})
    if any(includes) do inc
    !startswith(inc, "@depot")
    end
    return :missing_depot_tag
    end
    for depot in DEPOT_PATH
    if all(includes) do inc
    isfile(restore_depot_path(inc, depot))
    end
    return depot
    end
    end
    return :no_depot_found
    end

Initially, I also required that the paths looked like <depot>/packages/<rest>. I reverted this later, because it prevented relocation of pkgs which are on a LOAD_PATH that is not in a packages dir of a depot. This was mainly done so that it works with the MWE given by Tim Holy here: #49866 (comment) (although I don't know if his intention was that it should be made to work like that; that was my idea + PkgEval feedback, although the latter is not sensitive when relocation fails, because then re-precompliations kicks in)

To me, we have failed to identify a @depot here (since the path to Foo.jl does not follow the depot format (it is not inside packages)) but still it seems that the include_dependency path gets rewritten as if assumed that everything is inside a depot (but not really since then it would use @depot...).

This is fixable by also using the include_dependency paths in the depot path search here:

julia/base/loading.jl

Lines 2761 to 2763 in 72cd63c

# determine path for @depot replacement from srctext files only, e.g. ignore any include_dependency files
srcfiles = srctext_files(f, srctextpos, includes)
depot = resolve_depot(srcfiles)

This is the fix 1. I suggested above as actionable (sorry, I did not find time yet to open a PR; also I was waiting a bit in case there are more questions that need to be resolved).


IIUC then we need a breaking change for include_dependency here, because atm it is used

  • with project READMEs (which are supposed to be relocatable)
  • and with system paths (drivers for CUDA_jll and friends).

Hence, my suggestion for fix 2. above.

@staticfloat
Copy link
Member

staticfloat commented Jan 3, 2024

So what's the path forward here, we have a nonstandard usage of include_dependency() that ends up recompiling a module every time it is included, which is a bit painful.

@stevengj
Copy link
Member

stevengj commented Jan 3, 2024

Seems like it should use a different function than include_dependency, or maybe a new keyword option to that function to change its behavior?

Is this addressed by #49866 and/or #51798?

@KristofferC
Copy link
Member Author

On loading the relative @depot path is converted to a (wrong) absolute path. We find and restore the depot path with these functions:

If the file after the absolutification of the path does not exist, could we not loop through all DEPOTS and try find the file that way (things are content addressed so as long as we find it, it is ok)?

@fatteneder
Copy link
Member

If the file after the absolutification of the path does not exist, could we not loop through all DEPOTS and try find the file that way (things are content addressed so as long as we find it, it is ok)?

This would require #51798, because include_dependencys are still tracked by mtime.

@KristofferC
Copy link
Member Author

Why not switch over to tracking content by default (just like for package files)?

@fatteneder
Copy link
Member

There are pkgs that rely on mtime to trigger re-precompilation: #49866 (comment)

@KristofferC
Copy link
Member Author

But if this is restricted to when looking inside a depot it should be ok because you cannot/shouldn't modify something in a depot anyway?

@staticfloat
Copy link
Member

But if this is restricted to when looking inside a depot it should be ok because you cannot/shouldn't modify something in a depot anyway?

Depots contain things like scratchspaces which are read/write.

fatteneder added a commit to fatteneder/julia that referenced this issue Jan 6, 2024
fatteneder added a commit to fatteneder/julia that referenced this issue Jan 6, 2024
fatteneder added a commit to fatteneder/julia that referenced this issue Jan 6, 2024
fatteneder added a commit to fatteneder/julia that referenced this issue Jan 6, 2024
staticfloat pushed a commit that referenced this issue Jan 8, 2024
…erent depots (#52750)

Fixes #52161

#52161 is an awkward use of the relocation feature in the sense that it
attempts to load the `include()` and `include_dependency` files of a pkg
from two separate depots.
The problem there is that the value with which we replace the `@depot`
tag for both `include()` and `include_dependency()` files is determined
by trying to relocate only the `include()` files. We then end up not
finding the `include_dependency()` files.

Solution:
@staticfloat noted that the pkg slugs in depot paths like
`@depot/packages/Foo/1a2b3c/src/Foo.jl` are already enough to (weakly)
content-address a depot.
This means that we should be able to load any `include()` file of a pkg
from any `depot` that contains a precompile cache, provided the hashes
match.
The same logic can be extended to `include_dependency()` files, which
this PR does.

Note that we continue to use only one file from the `include()` files to
determine the depot which we use to relocate all `include()` files.
[this behavior is kept from master]
But for `include_dependency()` files we allow each file to resolve to a
different depot. This way the MWE given in #52161 should be extendable
to two README files being located in two different pkgs that lie in two
different depots.

---

Side note: #49866 started with explicitly verifying that all `include()`
files come from the same depot. In #52346 this was already relaxed to
pick the first depot for which any `include()` file can be resolved to.
This works, because if any other file might be missing from that depot
then this is caught in `stalecache()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:precompilation Precompilation of modules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants