Transits.jl icon indicating copy to clipboard operation
Transits.jl copied to clipboard

switching to `PythonCall.jl`

Open icweaver opened this issue 3 years ago • 8 comments

Trying out PythonCall.jl for our Python-based tests here. It uses scratch spaces and self contained envs, which is chef's kiss, and I think activating already existing envs is on the creator's to-do. If we can get this to play nice with CI in terms of caching, should we just make the switch entirely over from PyCall.jl?

At first, needing to do things like explicitly indicating what Python packages we want in global scope and applying conversions to Py objects manually seemed like a trade-off to me, but the more I use it, the more it is starting to grow on me as a self-documenting feature

To-Do

  • [ ] Re-use Python env in CI
  • [ ] Handle precompilation https://cjdoris.github.io/PythonCall.jl/stable/pythoncall/#Writing-packages-which-depend-on-PythonCall
  • [ ] Decide on CondaPkg.toml location? (only seems to be found if in root directory I think, but maybe that can change)

icweaver avatar Jan 15 '22 02:01 icweaver

Codecov Report

Merging #32 (c0d0e64) into main (16fc5c9) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #32   +/-   ##
=======================================
  Coverage   91.70%   91.70%           
=======================================
  Files          14       14           
  Lines        1181     1181           
=======================================
  Hits         1083     1083           
  Misses         98       98           

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 16fc5c9...c0d0e64. Read the comment docs.

codecov[bot] avatar Jan 15 '22 08:01 codecov[bot]

This looks like it's worth a look! Thanks for digging into it, Ian!

Decide on CondaPkg.toml location?

I think you can just put it in test/CondaPkg.toml and it will sync with the PythonCall.jl used in the test environment. This is how I did it with the benchmarks here, and it created a virtualenv in the bench folder (which I added to .gitignore)

mileslucas avatar Jan 15 '22 18:01 mileslucas

Sounds good!

I think you can just put it in test/CondaPkg.toml and it will sync with the PythonCall.jl used in the test environment

Oh, I was hoping that would work! I think I am doing something incorrectly though because PythonCall.jl doesn't seem to find it there and just falls back to the CI temp env I think:

CondaPkg Creating environment
     /home/runner/.julia/scratchspaces/0b3b1443-0f03-428d-bdfb-f27f9c1191ea/install/micromamba
     -r
     /home/runner/.julia/scratchspaces/0b3b1443-0f03-428d-bdfb-f27f9c1191ea/root
     create
     -y
     -p
     /tmp/jl_vuX7W8/.CondaPkg/env
     --no-channel-priority
     python >=3.5,<4
     -c
     conda-forge

icweaver avatar Jan 15 '22 18:01 icweaver

oh, I see. I haven't used it on CI. In that run, though, is the CondaPkg.toml in the root folder or the test folder?

mileslucas avatar Jan 15 '22 18:01 mileslucas

Test folder. Oh, I've been trying it locally too and seem to be getting the same issue. Only having it in the root folder seems to work for now, but I can double check

icweaver avatar Jan 15 '22 18:01 icweaver

Also started a discussion here, but could just be jumping the gun

https://github.com/cjdoris/PythonCall.jl/discussions/96

icweaver avatar Jan 15 '22 18:01 icweaver

Good snooping, we'll need some sorcery to overcome the "magic" 🪄

mileslucas avatar Jan 15 '22 18:01 mileslucas

So I fell down a rabbit hole of package tooling lore. I guess the dream of having truly separate project and test envs just isn't there quite yet. Until #1233 comes, how do you feel about going "old school" with the setup in c0d0e64?

Having a top-level view of everything in a single Project.toml feels kind of nice, but that might just be the Stockholm syndrome talking at this point

icweaver avatar Jan 16 '22 02:01 icweaver