Fix our use of pyjulia so we can use the conda-forge version. #2444

rwest · 2023-05-19T15:34:29Z

This is a subset of what was in #2443 but I'm breaking it off into a smaller PR that can be merged first.

Motivation or Problem

We have an old and customized version of pyjulia on the rmg conda channel that we fail to maintain.
Debugging things with pyjulia involved can be a bit tricky.

Description of Changes

switches to the conda-forge version of pyjulia, instead of the customized (and old) rmg channel one, so we are more standardized and have less to maintain.
fixes our CI workflow so that we can use the new pyjulia (remove the symbolic linking step that made python-jl masquerade as python).
adds developer instructions for how to debug (in VSCode)

Testing

Runs on my intel Mac, following instructions with a fresh anaconda environment and no system-wide Julia.
Runs on the CI test suite.

Reviewer Tips

Most of this has already been reviewed by @JacksonBurns when it was #2443 so should be a quick review, but I'd like @mjohnson541 's input.

Possible concern.

The readme for pyrms says "to work with conda python efficiently currently requires relinking the python executable to the python-jl executable"
so perhaps @mjohnson541 will tell us that skipping this step (as proposed here) is a bad idea?
I can't see anything in the commit messages when those instructions were added to explain the details.
I would note that

our developer instructions do not recommend making the symlink (commit 5d20826 almost added it? but didn't)
our Docker container does currently make the symlink (probably that should be removed in this PR, if this change is approved)

Environment alert ⚠️

This updates the conda environment file again.
So to be consistent, you'll need

a fresh conda environment (using pyjulia from conda-forge instead of from the rmg channel)
you might need to re-do the whole step of installing julia in python and python in julia etc. (following the instructions)
if you have symlinked python to point to python-jl, ether in your normal environment or in some other workflows, revert this.

JacksonBurns

This LGTM but I have a question that may impact changes (thus "Request changes") - do we now have to use "python-jl rmg.py ..." without a system image and then "python rmg.py ..." otherwise? The docs should clarify this, unless I am just misunderstanding. We should also replace #2310 with this PR, I think.

codecov · 2023-05-19T18:19:17Z

Codecov Report

Merging #2444 (83a6de3) into main (955c8f1) will not change coverage.
The diff coverage is n/a.

❗ Current head 83a6de3 differs from pull request most recent head 9aa3998. Consider uploading reports for the commit 9aa3998 to get more accurate results

@@           Coverage Diff           @@
##             main    #2444   +/-   ##
=======================================
  Coverage   48.13%   48.13%           
=======================================
  Files         110      110           
  Lines       30626    30626           
  Branches     7988     7988           
=======================================
  Hits        14743    14743           
  Misses      14353    14353           
  Partials     1530     1530

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

rwest · 2023-05-19T19:13:57Z

After this, we should always use python-jl to launch rmg.py. Thanks for reminding me that the "running" instructions may be out of date. (nb. the previous instructions wouldn't have worked for folks installing via the "source code for developers" install instructions).
I've now

updated the running instructions (telling people to use python-jl)
updated the Docker build file (removing the symlink step)
cherry-picked change shebang in update_headers to use python-jl instead of python3 #2310

rwest · 2023-05-19T19:16:29Z

do we now have to use "python-jl rmg.py ..." without a system image and then "python rmg.py ..." otherwise?

Dealing with system images is happening on #2443 and instructions for what to do when you have one will be added there. (FWIW, I currently can't get it it work).

mjohnson541 · 2023-05-23T18:52:03Z

Relinking is mostly a convenience thing. All it does is makes it so when you use python you're using python-jl if you use python-jl when needed there's no reason to do the relinking operation.

rwest · 2023-05-23T20:45:09Z

Great. Thanks for the confirmation @mjohnson541. Do you think this PR is Ok then?

mjohnson541 · 2023-05-24T03:37:43Z

LGTM!

We used to force a symlink of python-jl to where python used to be, so that when you ask for python you get python-jl. This would work if the python-jl itself called to python3, (which is what our hacked version of it did). But the conda-forge version of python-jl calls python, and if python points to python-jl, then you get an infinite loop of calling python-jl. That's my hunch. This will test the theory, and see what happens. Maybe we'll have to explicitly call python-jl in a few places we used to be lazy and called python. But it seems to work and makes us compatible with a standard python-jl. Also, "explicit is better than implicit!"

The rmg conda recipe just changes 'python-jl' to point to a python3 instead of python binary. Maybe this can be avoided. Would be nice to not maintain a non-standard package. Also merged Calvin's commit that did the same and forced it to >=0.6 Co-authored-by: Richard West <r.west@northeastern.edu> Co-authored-by: Calvin Pieters <calvinpieters@gmail.com>

These could be helpful. It seems some of the code changes on this branch were trying to get things to work in a debugger, so I thought I'd share how I got it to work (in VSCode)

(Annoyingly, this builds all the julia stuff and takes for ever. Grrr)

If we remove the symlink from python to python-jl, so that people are more aware that they are in fact running python-jl, we need to update the running instructions.

Now that we're using the conda-forge::pyjulia in the environment, we need to not do this symlink step in the Docker image. We want the environment for our docker users to match everyone else, so we have one set of instructions.

Good review - I've addressed the concern by updating the documentation in a subsequent commit.

rwest · 2023-05-24T14:37:58Z

The build-documentation job ran on the CI test (or this wouldn't have auto-merged) but when I try on my mac I get an error culminating in ERROR: LoadError: Failed to precompile SteadyStateDiffEq [9672c7b4-1e72-59bd-8a11-6ac3964bc41f] to /opt/miniconda3/envs/rmg_env5/share/julia/compiled/v1.8/SteadyStateDiffEq/jl_HZAEMz. .

I need to run sphinx through python-jl, like so:

The make documentation would essentially do::

cd documentation
sphinx-build -b html -d build/doctrees   source build/html

instead I do::

cd documentation
python-jl $(which sphinx-build) -b html -d build/doctrees   source build/html

Not sure if this is true for others?
If so maybe we fix the makefile and instructions to do this? (until python-julia integration is less painful)

rwest requested review from JacksonBurns and mjohnson541 May 19, 2023 15:34

rwest mentioned this pull request May 19, 2023

Make and use a julia system image #2443

Closed

JacksonBurns previously requested changes May 19, 2023

View reviewed changes

rwest marked this pull request as draft May 19, 2023 19:17

rwest marked this pull request as ready for review May 19, 2023 19:18

JacksonBurns mentioned this pull request May 19, 2023

change shebang in update_headers to use python-jl instead of python3 #2310

Closed

rwest mentioned this pull request May 22, 2023

Updating Conda environment requirements #2322

Closed

2 tasks

mjohnson541 approved these changes May 24, 2023

View reviewed changes

rwest and others added 7 commits May 24, 2023 00:15

Added developer instructions for debugging.

010983c

These could be helpful. It seems some of the code changes on this branch were trying to get things to work in a debugger, so I thought I'd share how I got it to work (in VSCode)

Add developer instructions for building documentation.

eaa3bca

(Annoyingly, this builds all the julia stuff and takes for ever. Grrr)

Change running instructions to use python-jl

eace65f

If we remove the symlink from python to python-jl, so that people are more aware that they are in fact running python-jl, we need to update the running instructions.

Docker: Don't create python to python-jl symlink

d86c603

Now that we're using the conda-forge::pyjulia in the environment, we need to not do this symlink step in the Docker image. We want the environment for our docker users to match everyone else, so we have one set of instructions.

change shebang in update_headers to use python-jl

9aa3998

rwest force-pushed the pyjuliafix branch from a9ad0d1 to 9aa3998 Compare May 24, 2023 04:17

rwest enabled auto-merge May 24, 2023 04:29

rwest merged commit 54b478d into ReactionMechanismGenerator:main May 24, 2023

rwest deleted the pyjuliafix branch May 24, 2023 14:03

rwest mentioned this pull request May 24, 2023

Improve conda installation instructions #2246

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix our use of pyjulia so we can use the conda-forge version. #2444

Fix our use of pyjulia so we can use the conda-forge version. #2444

Uh oh!

rwest commented May 19, 2023

Uh oh!

JacksonBurns left a comment

Uh oh!

codecov bot commented May 19, 2023 •

edited

Loading

Uh oh!

rwest commented May 19, 2023

Uh oh!

rwest commented May 19, 2023

Uh oh!

mjohnson541 commented May 23, 2023

Uh oh!

rwest commented May 23, 2023 •

edited

Loading

Uh oh!

mjohnson541 commented May 24, 2023

Uh oh!

rwest commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix our use of pyjulia so we can use the conda-forge version. #2444

Fix our use of pyjulia so we can use the conda-forge version. #2444

Uh oh!

Conversation

rwest commented May 19, 2023

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

Possible concern.

Environment alert ⚠️

Uh oh!

JacksonBurns left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rwest commented May 19, 2023

Uh oh!

rwest commented May 19, 2023

Uh oh!

mjohnson541 commented May 23, 2023

Uh oh!

rwest commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjohnson541 commented May 24, 2023

Uh oh!

rwest commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 19, 2023 •

edited

Loading

rwest commented May 23, 2023 •

edited

Loading