Julia 1.0 support #183

tkf · 2018-08-17T01:18:44Z

It is based on #178. It's probably better to pull #178 first to make reviewing easier.

stevengj · 2018-08-17T01:24:42Z

Needs a rebase.

tkf · 2018-08-17T01:31:18Z

Awesome! Thanks for the merge! It's rebased now.

tkf · 2018-08-17T02:10:32Z

CIs are all green now!

mprogram · 2018-08-17T13:19:59Z

Why then do we have:

test/test_core.py exception occured? None
exception occured? None
.exception occured? 139909767752752
...exception occured? None
exception occured? None
.exception occured? None
exception occured? None
exception occured? None
exception occured? None
exception occured? None
.......Executing: /home/travis/build/JuliaPy/pyjulia/.tox/py/bin/python -c from julia import Base
┌ Warning: `Pkg.dir(pkgname, paths...)` is deprecated; instead, do `import PyCall; joinpath(dirname(pathof(PyCall)), "..", paths...)`.
└ @ Pkg.API /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Pkg/src/API.jl:454
...exception occured? None
.....                                         
test/test_magic.py Initializing Julia interpreter. This may take some time...
.Initializing Julia interpreter. This may take some time...
.Initializing Julia interpreter. This may take some time...
.Initializing Julia interpreter. This may take some time...
Exception 'ArgumentError' occurred while calling julia code:
pop!([])
.Initializing Julia interpreter. This may take some time...
Exception 'ErrorException' occurred while calling julia code:
1 += 1
.

in the Travis log (checked that of Python 3.6)?

stevengj · 2018-08-17T13:32:41Z

julia/core.py

-                 :(Main.Base.include($PyCall_depsfile)),
-                 :(println(pyprogramname))))
+             include(PyCall_depsfile)
+             println(pyprogramname)


I thought the reason for putting this in an anonymous module was to avoid putting pyprogramname into the global namespace. Is this no longer a concern?

I thought it was Julia 0.4 compatibility or something. This is executed in a subprocess which exits right after printing pyprogramname. So I don't think we have namespace problem. It is added in fde40d7#diff-2f4f81856f2b92c6a4bb1e5bba2cf28aR260

stevengj · 2018-08-17T13:34:00Z

julia/with_rebuilt.py

+    try:
+        yield
+    finally:
+        signal.signal(sig, s)


Can we have a comment about what's going on here?

I added a docstring.

stevengj · 2018-08-17T13:36:52Z

test/test_core.py

@@ -97,8 +97,8 @@ def test_from_import_non_existing_julia_name(self):
    def test_julia_module_bang(self):
        from julia import Base
        xs = [1, 2, 3]
-        ys = Base.scale_b(xs[:], 2)
-        assert all(x * 2 == y for x, y in zip(xs, ys))
+        ys = Base.fill_b(xs[:], 10)


Why not ys = Base.fill_b(xs, 10) ?

But I would just set ys = [2, 4, 6] and keep the old test all(x * 2 == y for x, y in zip(xs, ys))

If we want to test the "bang" in-place operators we should do so with a numpy array where we can check that it is actually operating in-place.

The old test does not work since scale! is gone in Julia 1.0. The easiest bang method that I could find both in Julia 0.6 and 1.0 was fill!. And indeed xs[:] is redundant. Thanks for catching thins.

I realize that, but the new test doesn't actually test much. That's why I suggested just setting ys = [2, 4, 6] and keeping the test of all as-is.

My intention when writing the test was just to test variable renaming code path (back then I wasn't sure if it was OK to import numpy). But now that we have it via tox, I suggest something like:

xs = numpy.zeros(3) Base.fill_b(xs, 10) assert all(ys == 10)

or

xs = numpy.array([1, 2, 3]) ys = xs.copy() if julia.eval('VERSION >= v"0.7-"'): Base.rmul_b(xs, 2) else: Base.scale_b(xs, 2) assert all(2 * ys == xs)

Which one do you like?

Edit: ys = xs[:] -> ys = xs.copy(). Coding to much in Julia. I start forgetting Python :)

Actually, the test I proposed does not work. Strange...

def test_julia_module_bang(self): from julia import Base xs = numpy.zeros(3) ys = Base.fill_b(xs, 10) assert all(ys == 10) > assert all(xs == 10) E AssertionError: assert False E + where False = all(array([0., 0., 0.]) == 10)

Ah, that was because py"xs" was converted to PyAny. If I do fill!(PyArray(PyCall.maindict()["xs"]), 10) in Julia, mutation works (= it's reflected on Python side).

I'm not sure what's the right illustration of this problem in Julia. Something like this?:

julia> py""" def apply(f): xs = numpy.zeros(3) f(xs) return xs """ julia> py"apply"(xs -> fill!(xs, 10)) 3-element Array{Float64,1}: 0.0 0.0 0.0

See JuliaPy/PyCall.jl#531 (comment) … you need to specify the type conversion you want for an argument if you want in-place operations to be possible in general.

Got it. Yes, I think explicitly denoting which argument is mutating is a good idea.

So, for this test, we just need to check the returned value, right?:

def test_julia_module_bang(self): from julia import Base xs = [1, 2, 3] ys = Base.fill_b(xs, 10) assert all(y == 10 for y in ys)

Although it may be better if we just drop _b since users have to implement a callback on Julia side to avoid allocations.

OK. So I think I finally found a meaningful test:

def test_julia_module_bang(self): from julia.Base import Channel, put_b, take_b chan = Channel(1) sent = 123 put_b(chan, sent) received = take_b(chan) assert sent == received

What do you think?

In the future, I think we need an API to denote some Python values have to be passed without copying. Something like fill_b(asref(arr), 10).

I think the right thing to do is to make it easy to use the pyfunction method from Julia, for cases where you need control over precisely how arguments are passed.

tkf · 2018-08-17T20:29:40Z

@mprogram Those are expected debug messages. We pass -s option to pytest so that it's easier to debug CI but I agree it's not very clean.

noahstier · 2018-08-18T00:00:51Z

With julia --version -> 1.0.0 and python --version -> Python 3.5.5 :: Anaconda, Inc., if I do this:

pip install git+https://github.com/tkf/pyjulia.git@jl10
python -c "import julia; julia.Julia()"

I get this error:

┌ Warning: `Pkg.dir(pkgname, paths...)` is deprecated; instead, do `import PyCall; joinpath(dirname(pathof(PyCall)), "..", paths...)`.
└ @ Pkg.API /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Pkg/src/API.jl:454
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "~/miniconda3/envs/35/lib/python3.5/site-packages/julia/core.py", line 562, in __init__
    self.__julia = Julia(*args, **kwargs)
  File "~/miniconda3/envs/35/lib/python3.5/site-packages/julia/core.py", line 440, in __init__
    "\"lib\", \"pyjulia%s-v$(VERSION.major).$(VERSION.minor)\"))" % sys.version_info[0])
  File "~/miniconda3/envs/35/lib/python3.5/site-packages/julia/core.py", line 488, in _call
    self.check_exception(src)
  File "~/miniconda3/envs/35/lib/python3.5/site-packages/julia/core.py", line 517, in check_exception
    .format(exception, src))
julia.core.JuliaError: Exception 'UndefVarError' occurred while calling julia code:
pushfirst!(Base.LOAD_CACHE_PATH, abspath(Pkg.Dir._pkgroot(),"lib", "pyjulia3-v$(VERSION.major).$(VERSION.minor)"))

Investigating:
julia -e "println(Base.LOAD_CACHE_PATH)"
gives
ERROR: UndefVarError: LOAD_CACHE_PATH not defined

and
julia -e "using Pkg; println(Pkg.Dir)"
gives
ERROR: UndefVarError: Dir not defined

tkf · 2018-08-18T00:29:04Z

@noahstier #185

mprogram · 2018-08-18T10:36:46Z

@noahstier look at /issues/148 for a patching hint (presently blocked by #185)

tkf · 2018-08-20T06:25:08Z

julia/core.py

+             PyCall_depsfile = Pkg.dir("PyCall","deps","deps.jl")
+         else
+             modpath = Base.locate_package(Base.identify_package("PyCall"))
+             PyCall_depsfile = joinpath(dirname(modpath),"..","deps","deps.jl")


These lines above are for suppressing "Pkg.dir(pkgname, paths...) is deprecated" warning.

Before this change: https://travis-ci.org/JuliaPy/pyjulia/jobs/417810499#L540-L542

After this change: https://travis-ci.org/JuliaPy/pyjulia/jobs/418083304#L540

I took the code from Pkg.dir: https://github.com/JuliaLang/Pkg.jl/blob/df546844d98dfe233889e91a86f076e01926f69e/src/API.jl#L456-L463 (Note: Base.locate_package(::Nothing) = nothing is defined so I don't think we need to guard against Base.identify_package returning nothing as Pkg.dir does.)

stevengj · 2018-08-21T15:50:21Z

Ready to merge?

tkf · 2018-08-21T21:23:22Z

What about #183 (comment)? The problem you pointed about test_julia_module_bang is not solved yet. I can apply my last suggestion if you think that's better.

tkf · 2018-08-21T23:58:06Z

test/test_core.py

+        sent = 123
+        put_b(chan, sent)
+        received = take_b(chan)
+        assert sent == received


I go ahead and I do #183 (comment) (to make sure that it works on CIs). If you prefer fill_b or something else let me know.

If CIs and you are happy with it then I think it's ready for merge.

tkf · 2018-08-22T01:03:55Z

I wonder why appveyor is not running. There is "Intermittent issues with Chocolatey packages installation" https://appveyor.statuspage.io/incidents/kqwjgg14m71g but I don't think it's related to our setup (unless it is used internally somewhere).

tkf mentioned this pull request Aug 17, 2018

PyJulia/PyCall.jl not working well with new compilation cache management in Julia 0.7/Pkg3.jl JuliaLang/julia#28518

Closed

tkf added 5 commits August 16, 2018 18:29

Pass runtime to juliainfo

e33636b

Ignore SIGINT in with_rebuilt so that it works well with debuggers

d6dd5db

Fix with_rebuilt for Julia 1.0

4c916ab

Make tests Julia 1.0-compatible

5b02ab5

Test with Julia 1.0 in Travis CI

24022ed

tkf force-pushed the jl10 branch from e297089 to 24022ed Compare August 17, 2018 01:30

Fix .travis.yml and appveyor.yml

1d75179

tkf force-pushed the jl10 branch from 5a3b723 to 1d75179 Compare August 17, 2018 01:40

stevengj reviewed Aug 17, 2018

View reviewed changes

Document with_rebuilt.ignoring

a1f0b4b

tkf mentioned this pull request Aug 18, 2018

Fail with a helpful message if separate cache is not supported #186

Merged

tkf mentioned this pull request Aug 19, 2018

Resolve runtime path #187

Merged

Skip test_import_without_setup if JULIA_EXE is set

33f62a6

tkf force-pushed the jl10 branch from e551bef to 33f62a6 Compare August 19, 2018 06:05

This was referenced Aug 19, 2018

Small cleanups #188

Merged

Make untyped mutating callbacks work JuliaPy/PyCall.jl#531

Closed

Don't use Pkg.dir in Julia >= 0.7

c1c579d

tkf commented Aug 20, 2018

View reviewed changes

Don't call isfile(nothing)

e31331a

tkf mentioned this pull request Aug 20, 2018

Support Julia 1.0 tkf/IPython.jl#10

Merged

Test mutation in test_julia_module_bang

8cb559e

tkf commented Aug 21, 2018

View reviewed changes

tkf mentioned this pull request Aug 22, 2018

AppVeyor stopped working #191

Closed

stevengj merged commit 138d47d into JuliaPy:master Aug 22, 2018

tkf deleted the jl10 branch March 23, 2019 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Julia 1.0 support #183

Julia 1.0 support #183

tkf commented Aug 17, 2018

stevengj commented Aug 17, 2018

tkf commented Aug 17, 2018

tkf commented Aug 17, 2018

mprogram commented Aug 17, 2018

stevengj Aug 17, 2018

tkf Aug 17, 2018

stevengj Aug 17, 2018

tkf Aug 17, 2018

stevengj Aug 17, 2018 •

edited

Loading

tkf Aug 17, 2018

stevengj Aug 17, 2018

tkf Aug 17, 2018 •

edited

Loading

tkf Aug 18, 2018

tkf Aug 19, 2018

stevengj Aug 20, 2018

tkf Aug 20, 2018

tkf Aug 21, 2018

stevengj Aug 22, 2018

tkf commented Aug 17, 2018

noahstier commented Aug 18, 2018 •

edited

Loading

tkf commented Aug 18, 2018

mprogram commented Aug 18, 2018

tkf Aug 20, 2018

stevengj commented Aug 21, 2018

tkf commented Aug 21, 2018

tkf Aug 21, 2018

tkf commented Aug 22, 2018

Julia 1.0 support #183

Julia 1.0 support #183

Conversation

tkf commented Aug 17, 2018

stevengj commented Aug 17, 2018

tkf commented Aug 17, 2018

tkf commented Aug 17, 2018

mprogram commented Aug 17, 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

stevengj Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkf Aug 17, 2018 • edited Loading

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

tkf commented Aug 17, 2018

noahstier commented Aug 18, 2018 • edited Loading

tkf commented Aug 18, 2018

mprogram commented Aug 18, 2018

Choose a reason for hiding this comment

stevengj commented Aug 21, 2018

tkf commented Aug 21, 2018

Choose a reason for hiding this comment

tkf commented Aug 22, 2018

stevengj Aug 17, 2018 •

edited

Loading

tkf Aug 17, 2018 •

edited

Loading

noahstier commented Aug 18, 2018 •

edited

Loading