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

cap julia version for probably-0.7-incompatible packages #685

Merged
merged 8 commits into from
Sep 19, 2018

Conversation

StefanKarpinski
Copy link
Member

@StefanKarpinski StefanKarpinski commented Aug 23, 2018

This is a heuristic so definitely imperfect, but follows this reasoning about each versions julia compatibility version set:

  • has interval only containing 0.7+ versions => 0.7 compatible
  • supports an julia ≤ 0.6 & doesn't use Compat => 0.7 incompatible
  • claims to support 0.7+ & tagged on July 14 or later => 0.7 compatible
  • passes PkgEval => 0.7 compatible
  • otherwise => 0.7 incompatible

@KristofferC
Copy link
Member

KristofferC commented Aug 23, 2018

Is it possible to rebase this on top of #554 (which has some other fixes, for example the 1.1 julia dummy release which is needed) or merge them somehow. #554 also has updated shamaps but these are much more extensive than the ones here for some reason.

The heruistic seems reasonable to me. Would be interesting to see the diff in the registry this produces.

@StefanKarpinski
Copy link
Member Author

Sure, I’ll rebase and can post the registry diff as a gist—it’s quite large. I just ran the update script against an up-to-date copy of the metadata repo.

@StefanKarpinski
Copy link
Member Author

Rebasing master of Pkg onto that branch is causing a ton of conflicts that are totally unrelated to this PR. I guess I should just cherry-pick this change on top of that branch?

@KristofferC
Copy link
Member

Yeah, the branch is very outdated so easiest to cherry-pick the commit from that branch here (which is an updated branch). Either your commit first or the one in that PR, whichever is easiest.

@StefanKarpinski
Copy link
Member Author

StefanKarpinski commented Aug 23, 2018

This doesn't really work since I can't get the git metadata updates to cherry-pick over and I'm just not willing to spend another three days waiting for those to update. We really need to be propagating git metadata updates back to master so that this doesn't take so long every time it needs to be updated. This branch works as-is and has up-to-date git metadata TOML files; I have no idea what the best way to integrate that with the out-of-date update branch is.

@KristofferC
Copy link
Member

Let me try then

@KristofferC
Copy link
Member

KristofferC commented Aug 23, 2018

Aha, you pushed an update to the shamaps to master. Ok, we can just ignore the maps in that PR then.

With the exception of julia since updating julia in the shamap was disabled with

(pkg == "julia" || isempty(p.versions)) && continue

@martinholters
Copy link
Member

Maybe also

  • has explicit upper bound => just trust the given bound

Or is that implicitly done right now? (I didn't check the code.)

@StefanKarpinski
Copy link
Member Author

Updated with @martinholters's suggestion. Here's a gist of the registry diff:

https://gist.github.com/StefanKarpinski/b9df8cc7eb9eca8f02a73e6a9d08a888

@StefanKarpinski
Copy link
Member Author

I also cherry-picked @KristofferC's update commit into there before the substantive changes here.

@KristofferC
Copy link
Member

KristofferC commented Aug 24, 2018

I'll deploy this, run it manually and open the changes as a PR so we can look at it with GitHub diff interface before committing to it.

PR: JuliaRegistries/General#16

@KristofferC
Copy link
Member

This caps https://github.com/JuliaGraphics/Measures.jl which passes tests on 1.0 and is a dependency of Plots. Hmm.

@StefanKarpinski StefanKarpinski force-pushed the sk/cap-0.7 branch 2 times, most recently from c7ad386 to 1739b2b Compare August 25, 2018 23:23
@StefanKarpinski
Copy link
Member Author

I've uncapped the maximum version of any package that's passing PkgEval.

@lobingera
Copy link

@StefanKarpinski Do i read correctly, that the list of passing on PkgEval is hardcoded? Where is the reference?

@KristofferC
Copy link
Member

Dynamically loading it doesn't have an advantage because any package that starts to pass tests will have a new tag and thus won't be capped.

bin/loadmeta.jl Outdated
@@ -174,6 +174,7 @@ const date_cutoff = 1531526400 # July 14, 2018
const all_vers = julia_versions()
const old_vers = julia_versions(v -> v < v"0.7")
const meta_dir = Pkg.Pkg2.dir("METADATA")
const passing = readlines("bin/passing.txt")
Copy link
Member

Choose a reason for hiding this comment

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

joinpath(@__DIR__, "passing.txt").

@KristofferC
Copy link
Member

Arguably, this should be capped to 0.7 upper bound instead of 0.6 since 0.7 should have deprecations in place?

@KristofferC
Copy link
Member

Things like WriteVTK now get capped which does pass test because, going along the dependency chain, TranscodingStreams fails tests which is a dependency, so WriteVTK gets skipped.

Also, this seems to cap PyPlot. That doesn't seem right?

@StefanKarpinski
Copy link
Member Author

So you think anything that’s not failing should not be capped? It seems to me that only packages that are actually passing should be exempt from capping logic. If a package is skipped we don’t know if it should be capped or not the heuristic should apply.

@KristofferC
Copy link
Member

Yeah, I'm just saying it is unfortunate that we cap working packages. Of course, it is a heuristic so that might happen, but we should try to keep that to a minimum.

@StefanKarpinski
Copy link
Member Author

There’s going to be some initial issues—they can be fixed by putting explicit correct caps on the packages.

@KristofferC
Copy link
Member

Could we run a pkgeval that just test all packages without doing the while "skipped" part. Many packages fail because their test code does something quirky but work fine with normal usage. They then block a bunch of working palette from getting tested.

@StefanKarpinski
Copy link
Member Author

@Keno could you do a PkgEval run like this so that we can get things capped?

@Keno
Copy link
Member

Keno commented Aug 27, 2018

sure

@StefanKarpinski
Copy link
Member Author

So what's the path forward here? Can I get a list of packages I should whitelist?

@KristofferC
Copy link
Member

The path forward here is to have a heuristic that doesn't recursively make most package uninstallable (like capping BinaryProvider would do).

I am guessing BinaryProvider gets capped because it has julia 0.7-rc3 in its REQUIRE and the v < v"0.7" check becomes true?

Then we need to generate a list like https://gist.github.com/KristofferC/6ef88542178e143fa9dc840fc1e13749 except also include recursively what packages are no longer installable and see that that list is not completely crazy.

@StefanKarpinski
Copy link
Member Author

@Keno, can you help us get such a list. Given a list, I can put it in here and we can finally cap.

@KristofferC
Copy link
Member

Hm, I think you missunderstood what list I meant (since you are asking Keno for help).

The list is just meant as a check that our cappening isn't effectively capping the whole ecosystem. The current heuristic does so by capping BinaryBuilder due to the likely "bug" that I mentioned in the previous post with the RC in REQUIRE.

So what we need to go ahead with this is a list of packages that are no longer installable by using Pkg.add (either because they are getting capped or a dependency of them is getting capped), manually look at that list and see that it is ok.

@StefanKarpinski
Copy link
Member Author

If I have a list of the packages that we believe to already be work, then I whitelist them and make sure their latest version is considered compatible. That's already how this works: there's an explicit whitelist that I was given in JSON format. There's no more tweaking of this heuristic that can really be done.

@martinholters
Copy link
Member

But are there any packages that do not get capped themselves, but become uninstallable because any of their dependencies get capped? If there are many, that might indicate a serious problem. If there are few, that might warrant a little more manual whitelisting.

@KristofferC
Copy link
Member

KristofferC commented Sep 14, 2018

But are there any packages that do not get capped themselves, but become uninstallable because any of their dependencies get capped? If there are many, that might indicate a serious problem.

Yes, we need to have such a list to evaluate the full impact this would have.

@codecov-io
Copy link

Codecov Report

Merging #685 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #685   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files          17       17           
  Lines        2959     2959           
=======================================
  Hits         2720     2720           
  Misses        239      239

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 80d9ba2...2e87b94. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #685 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #685   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files          17       17           
  Lines        2959     2959           
=======================================
  Hits         2720     2720           
  Misses        239      239

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 80d9ba2...2e87b94. Read the comment docs.

@StefanKarpinski
Copy link
Member Author

I just spent several hours trying to understand why BinaryProvider gets capped. But it does not actually get capped. Old versions of BinaryProvider get capped, but newer ones don't.

@StefanKarpinski
Copy link
Member Author

Based on the old "passing" list, PyPlot and YAML were the only dependencies that were not marked as passing but which are dependencies of packages that are.

@StefanKarpinski
Copy link
Member Author

Now the dependencies of passing packages are also marked as passing, which affected a whopping grand total of three packages.

@codecov-io
Copy link

Codecov Report

Merging #685 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #685   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files          17       17           
  Lines        2959     2959           
=======================================
  Hits         2720     2720           
  Misses        239      239

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 80d9ba2...f9093bf. Read the comment docs.

2 similar comments
@codecov-io
Copy link

Codecov Report

Merging #685 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #685   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files          17       17           
  Lines        2959     2959           
=======================================
  Hits         2720     2720           
  Misses        239      239

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 80d9ba2...f9093bf. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #685 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #685   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files          17       17           
  Lines        2959     2959           
=======================================
  Hits         2720     2720           
  Misses        239      239

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 80d9ba2...f9093bf. Read the comment docs.

@StefanKarpinski StefanKarpinski merged commit 50da0ef into master Sep 19, 2018
@bors bors bot deleted the sk/cap-0.7 branch September 19, 2018 16:04
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.

6 participants