[WIP] Update for Julia 0.7 and 1.0 #96

GunnarFarneback · 2018-08-10T11:42:35Z

Let's get ProfileView working in Julia 0.7 and 1.0. This PR is so far incomplete and fixes the more straightforward deprecations.

GunnarFarneback · 2018-08-10T11:44:03Z

REQUIRE

@@ -1,4 +1,4 @@
-julia 0.5
+julia 0.7


Is it fine to drop both Julia 0.5 and 0.6 support on master? Otherwise some compatibility additions are needed.

Yes, absolutely!

GunnarFarneback · 2018-08-10T11:47:27Z

test/test.jl

@@ -4,10 +4,10 @@ function profile_test(n)
    for i = 1:n
        A = randn(100,100,20)
        m = maximum(A)
-        Afft = fft(A)
-        Am = mapslices(sum, A, 2)
+        # Afft = fft(A)


I assume the fft call is only intended to provide some nontrivial amount of computations that can be profiled. With FFTW having been moved out of Base, it might be better to replace it with something that doesn't require a testing dependency. Or is it fine to just skip it?

Agreed better not to use fft. You can either replace it or skip it, either is fine with me.

GunnarFarneback · 2018-08-10T11:54:15Z

The current stumbling block is that the conditional loading and LOAD_PATH magic in __init__ needs to be updated.

function __init__()
    push!(LOAD_PATH, splitdir(@__FILE__)[1])
    if (isdefined(Main, :IJulia) && !isdefined(Main, :PROFILEVIEW_USEGTK)) || !have_display()
        eval(Expr(:import, :ProfileViewSVG))
        @eval begin
            view(data = Profile.fetch(); C = false, lidict = nothing, colorgc = true, fontsize = 12, combine = true, pruned = []) = ProfileViewSVG.view(data; C=C, lidict=lidict, colorgc=colorgc, fontsize=fontsize, combine=combine, pruned=pruned)
        end
    else
        eval(Expr(:import, :ProfileViewGtk))
        @eval begin
            view(data = Profile.fetch(); C = false, lidict = nothing, colorgc = true, fontsize = 12, combine = true, pruned = []) = ProfileViewGtk.view(data; C=C, lidict=lidict, colorgc=colorgc, fontsize=fontsize, combine=combine, pruned=pruned)
...

Any hints on how to proceed with this?

GunnarFarneback · 2018-08-10T12:58:42Z

No idea whether that import fix is correct, but it's good enough to run into Gtk dependency errors instead. Some kind of progress.

timholy

This looks great, thanks for doing it.

Gtk should come very soon, it will be exciting to be ready here.

timholy · 2018-08-11T00:52:25Z

REQUIRE

@@ -1,4 +1,4 @@
-julia 0.5
+julia 0.7


Yes, absolutely!

timholy · 2018-08-11T07:21:02Z

src/ProfileView.jl

    return haskey(ENV, "DISPLAY")
 end

+include("ProfileViewSVG.jl")
+include("ProfileViewGtk.jl")


So, these used to be loaded conditionally. The reason is that Gtk requires compilation, and not everyone has a compiler installed; with this design, they won't even be able to use the package with ProfileViewSVG because it will error when they issue using ProfileView.

If you changed this because you couldn't get it working, what was the error?

I'm certain there's a better solution, but I neither managed to understand the subtleties of how the code used to work nor more exactly why it was failing. Without the last commit the error is:

ERROR: LoadError: LoadError: UndefVarError: ProfileViewGtk not defined Stacktrace: [1] #view#23(::Bool, ::Nothing, ::Bool, ::Int64, ::Bool, ::Array{Any,1}, ::Function, ::Array{UInt64,1}) at /tmp/ProfileView.jl/src/ProfileView.jl:54 [2] view at /tmp/ProfileView.jl/src/ProfileView.jl:54 [inlined] (repeats 2 times) [3] top-level scope at none:0 [4] include at ./boot.jl:317 [inlined] [5] include_relative(::Module, ::String) at ./loading.jl:1038 [6] include(::Module, ::String) at ./sysimg.jl:29 [7] include(::String) at ./client.jl:398 [8] top-level scope at none:0 [9] include at ./boot.jl:317 [inlined] [10] include_relative(::Module, ::String) at ./loading.jl:1038 [11] include(::Module, ::String) at ./sysimg.jl:29 [12] include(::String) at ./client.jl:398 [13] top-level scope at none:0 in expression starting at /tmp/ProfileView.jl/test/test.jl:40 in expression starting at /tmp/ProfileView.jl/test/runtests.jl:6

timholy · 2018-08-11T07:24:34Z

src/tree.jl

    data::T
    parent::Node{T}
    child::Node{T}
    sibling::Node{T}

    # Constructor for the root of the tree
-    function (::Type{Node{T}}){T}(data::T)
+    function (::Type{Node{T}})(data::T) where T


These can change to the newer and much easier syntax, function Node{T})(data) where T.

timholy · 2018-08-11T07:26:18Z

src/tree.jl

-Node{T}(data::T) = Node{T}(data)
-Node{T}(data::T, parent::Node{T}) = Node{T}(data, parent)
+Node(data::T) where {T} = Node{T}(data)
+Node(data::T, parent::Node{T}) where {T} = Node{T}(data, parent)


Now that I know more I'd write this as Node(data, parent::Node{T}) where T (dropping the ::T on data). Having the parent is enough to specify the T we need, and any data that can be converted to T will then work.

Completely optional, however.

timholy · 2018-08-11T07:26:33Z

src/tree.jl

@@ -50,7 +50,7 @@ function lastsibling(sib::Node)
    sib
 end

-function addsibling{T}(oldersib::Node{T}, data::T)
+function addsibling(oldersib::Node{T}, data::T) where T


likewise here

timholy · 2018-08-11T07:27:03Z

src/tree.jl

        n = new{T}(data)
        n.parent = n
        n.child = n
        n.sibling = n
        n
    end
    # Constructor for all others
-    function (::Type{Node{T}}){T}(data::T, parent::Node)
+    function (::Type{Node{T}})(data::T, parent::Node) where T


timholy · 2018-08-11T07:28:15Z

test/test.jl

@@ -4,10 +4,10 @@ function profile_test(n)
    for i = 1:n
        A = randn(100,100,20)
        m = maximum(A)
-        Afft = fft(A)
-        Am = mapslices(sum, A, 2)
+        # Afft = fft(A)


Agreed better not to use fft. You can either replace it or skip it, either is fine with me.

…move commented out fft call during testing.

GunnarFarneback · 2018-08-11T14:12:52Z

Review comments have been addressed except for the conditional loading, which is beyond my grasp for now. At this point I'm getting massive amounts of warnings from GtkReactive. It would help if JuliaGizmos/GtkReactive.jl#69 could be merged.

This reverts commit f67b9c1.

GunnarFarneback · 2018-08-12T08:34:25Z

Reverted the conditional loading fix since it was buggy (had forgot to remove the pop! of LOAD_PATH), turned out not to solve the problem anyway, and destroyed the actual conditionality of the loading.

The loading issues still need to be fixed though.

GunnarFarneback · 2018-08-17T08:58:21Z

Some progress. The immediate problem wasn't about changes to the code loading logic but that the import AST had changed. I.e.

eval(Expr(:import, :ProfileViewGtk))

wasn't actually doing anything (at least not visible) and should instead have been

eval(Expr(:import, Expr(:., :ProfileViewGtk)))

But as far as I can tell that's equivalent to

eval(:(import ProfileViewGtk))

and

@eval import ProfileViewGtk

so I'll assume the explicit construction of the Expr had historical reasons.

GunnarFarneback · 2018-08-17T11:44:26Z

At this point the SVG backend might be functional. The GTK backend is blocked by GtkReactive.

timholy · 2018-08-21T01:41:41Z

I don't have the bandwidth to look at this now. After the Interpolations rewrite is done and I get Rebugger announced, maybe I'll have time. Till then, sorry, and good luck!

GunnarFarneback · 2018-08-27T18:58:04Z

Closing in slowly. With JuliaGizmos/GtkReactive.jl#72 I get ProfileView to pass its tests on 0.7, although there are still some warnings. And well, it doesn't actually work. It manages to draw the blocks but is somewhat less than useful without the annotations.

The main remaining warning is

┌ Warning: Package ProfileView does not have ProfileViewGtk in its dependencies:
│ - If you have ProfileView checked out for development and have
│   added ProfileViewGtk as a dependency but haven't updated your primary
│   environment's manifest file, try `Pkg.resolve()`.
│ - Otherwise you may need to report an issue with ProfileView
└ Loading ProfileViewGtk into ProfileView from project dependency, future warnings for ProfileView are suppressed.

Apparently the package manager (or maybe more accurate the code loader) is not impressed by the LOAD_PATH trickery for optional dependencies, so some improvement is needed on that front.

timholy · 2018-08-28T01:07:43Z

Yes, we might have to split it out as a separate package. Or perhaps we can add a toml file that provides the path?

timholy · 2018-08-30T15:58:36Z

Integrated into #99

Fix Julia 0.7 deprecations.

dcdaac3

GunnarFarneback commented Aug 10, 2018

View reviewed changes

Attempt to fix conditional submodule import.

f67b9c1

timholy reviewed Aug 11, 2018

View reviewed changes

Simplify constructurs. Relax dispatch for addsibling and addchild. Re…

8fe3b02

…move commented out fft call during testing.

Revert "Attempt to fix conditional submodule import."

069910b

This reverts commit f67b9c1.

GunnarFarneback added 2 commits August 17, 2018 10:49

Make eval of import robust to AST changes.

6e0d291

Use __DIR__ macro directly instead of extracting dir name from __FILE__.

bc0dcb9

GunnarFarneback added 3 commits August 17, 2018 13:36

Add using Profile.

55a99af

Fix more deprecations.

754f61a

tree_aggregate is gone from Profile. Revive a local copy.

52a45c5

Fix ProfileViewGtk deprecations.

837daa1

blegat mentioned this pull request Aug 28, 2018

Some more fixes for Julia 1.0 jump-dev/Gurobi.jl#150

Merged

timholy closed this Aug 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Update for Julia 0.7 and 1.0 #96

[WIP] Update for Julia 0.7 and 1.0 #96

GunnarFarneback commented Aug 10, 2018

GunnarFarneback Aug 10, 2018

timholy Aug 11, 2018

GunnarFarneback Aug 10, 2018

timholy Aug 11, 2018

GunnarFarneback commented Aug 10, 2018

GunnarFarneback commented Aug 10, 2018

timholy left a comment

timholy Aug 11, 2018

timholy Aug 11, 2018

GunnarFarneback Aug 11, 2018

timholy Aug 11, 2018

timholy Aug 11, 2018

timholy Aug 11, 2018

timholy Aug 11, 2018

timholy Aug 11, 2018

GunnarFarneback commented Aug 11, 2018

GunnarFarneback commented Aug 12, 2018

GunnarFarneback commented Aug 17, 2018 •

edited

Loading

GunnarFarneback commented Aug 17, 2018

timholy commented Aug 21, 2018

GunnarFarneback commented Aug 27, 2018

timholy commented Aug 28, 2018

timholy commented Aug 30, 2018

[WIP] Update for Julia 0.7 and 1.0 #96

[WIP] Update for Julia 0.7 and 1.0 #96

Conversation

GunnarFarneback commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GunnarFarneback commented Aug 10, 2018

GunnarFarneback commented Aug 10, 2018

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GunnarFarneback commented Aug 11, 2018

GunnarFarneback commented Aug 12, 2018

GunnarFarneback commented Aug 17, 2018 • edited Loading

GunnarFarneback commented Aug 17, 2018

timholy commented Aug 21, 2018

GunnarFarneback commented Aug 27, 2018

timholy commented Aug 28, 2018

timholy commented Aug 30, 2018

GunnarFarneback commented Aug 17, 2018 •

edited

Loading