Allow failures of indirect dependencies during parallel precompillation #2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
As a means to not block failures by packages that weren't going to be loaded anyway, sub-dependenncy compilation failures only warn now (main deps still fail). Idea from @tkf here #2018 (comment) Note that in this case, ColorTypes is a non-deved package that I hacked an error into |
IanButterworth
changed the title
tkf
reviewed
Sep 16, 2020
tkf
reviewed
Sep 16, 2020
|
Requires JuliaLang/julia#37596 |
|
Quoting the time-complexity consideration I commented in JuliaLang/julia#37596 (comment)
|
|
Do we think this and the julia PR is good as it stands? |
tkf
reviewed
Sep 17, 2020
IanButterworth
changed the title
IanButterworth
changed the title
IanButterworth
force-pushed
the
ib/parallel_precomp_optional
branch
from
906f46e to
e227bc2
Compare
IanButterworth
commented
Sep 17, 2020
tkf
reviewed
Sep 18, 2020
src/API.jl
Outdated
Comment on lines
907
to
910
| # stdlibs that aren't loaded might need precompiling if not part of sysimage | ||
| function is_stdlib_and_loaded(pkg::Base.PkgId) | ||
| return Pkg.Operations.is_stdlib(pkg.uuid) && Base.root_module_exists(pkg) | ||
| end |
tkf
reviewed
Sep 18, 2020
Co-authored-by: Takafumi Arakaki <takafumi.a@gmail.com>
tkf
reviewed
Sep 18, 2020
tkf
reviewed
Sep 18, 2020
Co-authored-by: Takafumi Arakaki <takafumi.a@gmail.com>
|
Now also requires JuliaLang/julia#37652 for |
fredrikekre
reviewed
Sep 22, 2020
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
|
That CI failure seems to be a connection glitch https://travis-ci.com/github/JuliaLang/Pkg.jl/jobs/389278321#L419 |
|
Passed now. Just the docs that auto-cancelled with the first fail |
tkf
suggested changes
Sep 22, 2020
tkf
approved these changes
Sep 22, 2020
tkf
reviewed
Sep 22, 2020
Co-authored-by: Takafumi Arakaki <takafumi.a@gmail.com>
tkf
reviewed
Sep 22, 2020
|
Some optimizations have been added to A no-op, now looks like this, where there are 215 deps in the manifest: |
|
I think this is good to go now, along with #2033 to allow opting-in to make it automatic after the common pkg calls |
Closed
tkf
reviewed
Sep 23, 2020
| any_dep_recompiled = any(map(dep_uuid->was_recompiled[dep_uuid], pkg_dep_uuid_lists[i])) | ||
| if !errored && (any_dep_recompiled || is_stale(paths, sourcepath)) | ||
| any_dep_recompiled = any(map(dep->was_recompiled[dep], deps)) | ||
| if !errored && (any_dep_recompiled || _is_stale(paths, sourcepath, toml_c)) |
There was a problem hiding this comment.
If we are going to rely on Base.TOMLCache to be "async-safe" (which is, BTW, pretty easy to break; e.g., a @debug can ruin this property), I don't think it's necessary to move _is_stale out. It's actually where closure is useful. Although there is nothing bad in factoring out _is_stale this way.
There was a problem hiding this comment.
@staticfloat did some probing of the list of toml files that were accessed when no tomlcaches were provided, and it was just the current environment manifest and project files (2 files), so the thinking was that it was async safe as it's read-only after the first use in the for-loop. Would that be broken by @debug?
There was a problem hiding this comment.
It'd be good to resolve this, and merge, to fix #2035
There was a problem hiding this comment.
parsed_toml(cache::TOMLCache, project_file::String) reuses TOML.Parser: https://github.com/JuliaLang/julia/blob/2a36f83ce9bdfd17c572472f60b830aedcf181f0/base/loading.jl#L174-L175
So, for example, TOML.parse(::TOML.Parser) has to be async-safe. It probably is and likely be so forever but relying on "likely" is not the best approach especially in concurrent programming. For example, maybe somebody will implement TOML.parse(::IO) at some point and the parsing can happen in a streaming fashion.
(It's also a bit concerning that the I/O read(project_file, String) is happening in get! but that's probably handled by the .age field in Dict.)
Anyway, I don't think my comment should block this PR. We can fix it in Base if we want.
There was a problem hiding this comment.
Ok, merging this as-is seems like the thing to do for now
tkf
reviewed
Sep 23, 2020
Closed
Co-authored-by: Takafumi Arakaki <takafumi.a@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Edit: The mechanism below has now been removed from this PR, given a better approach of allowing failures for indirect dependencies
Builds on #2018 to add the option to revert to the old-style
Pkg.precompile()behavior if the user just wants/needs to precompile dependencies that are loaded.