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

PR for #67: Julia extension #68

Merged
merged 17 commits into from
Oct 22, 2022
Merged

PR for #67: Julia extension #68

merged 17 commits into from
Oct 22, 2022

Conversation

jc-cisneros
Copy link
Contributor

@jc-cisneros jc-cisneros commented Oct 11, 2022

This pull request closes gslab-econ/gslab_make#54 and #67. This template branch is currently pulling the 54-create-run_julia branch of the gslab_make submodule, so gslab_make PR for 54 should be closed first.

The proposed steps to test the new Julia wrapper are the following:

  • Clone template and switch to 67-julia-extension branch
  • Create the template_julia conda environment (i.e., run conda env create -f setup/conda_env.yaml from root)
  • Load the submodule by running:
git submodule init
git submodule update
  • Navigate to lib/gslab_make and run git switch 54-create-run_julia (we are testing the 54-create-run_julia branch of gslab_make). After that, run git branch. The current branch of the submodule should be 54-create-run_julia.
  • Go back to root and activate the template_julia environment. Run check_setup.py from the setup module.
  • Run julia julia_conda_env.jl from root to install the Julia packages in the conda environment (these are managed by Julia's built-in Pkg). Following the same logic of the conda_env.yaml file, the julia_conda_env.jl file installs the latest versions of the packages and has the versions used in the last working run commented out.
  • Run run_all.py from root. A Julia script will be called on the analysis module. The file julia_scatter.png should be created and saved in analysis/output

Once the tests are successful, we can proceed to merge the 54-create-run_julia branch of gslab_make to master (through gslab_make PR for 54), point the submodule to master and run the entire repo again.

cc. @gentzkow @snairdesai @Houdanait @meyer-carl

@jc-cisneros jc-cisneros linked an issue Oct 11, 2022 that may be closed by this pull request
2 tasks
@snairdesai
Copy link
Contributor

@jc-cisneros I think we will need to update the submodule to point to the development branch commit you reference; it is currently still pointing to the latest commit in master. I'll re-review following this change.

@jc-cisneros
Copy link
Contributor Author

@snairdesai I updated the instructions to fetch the correct submodule. The additional error you were getting regarding the packages (e.g. run_julia() throwing the error that it did not find any of the install packages) has been fixed on my end. In particular we want to run julia --project=.. <filepath> instead of julia <filepath>. By doing that, we use the Julia environment that was instantiated in root.

@snairdesai
Copy link
Contributor

Thanks @jc-cisneros! I'll proceed with the review then.

@snairdesai
Copy link
Contributor

snairdesai commented Oct 12, 2022

@jc-cisneros Seems like this is closer to running. I'm getting the following errors, the first of which originates when I attempt to instantiate our objects in the interactive Julia terminal, and the second (which I believe follows directly from the first) is called when I execute run_all.py. Any feedback on resolving this (I've already rebuilt the template_julia environment just in case)?

(1)

Screen Shot 2022-10-12 at 12 12 21 PM


(2)

Screen Shot 2022-10-12 at 12 11 30 PM

@jc-cisneros
Copy link
Contributor Author

Thanks @snairdesai ! I rebuilt the Manifest.toml and Project.toml files. I uninstalled my local Julia and cleared my .julia cache. The run was successful in my end. Let me know if it works for you @snairdesai.

Screen Shot 2022-10-12 at 6 30 42 PM

@snairdesai
Copy link
Contributor

@jc-cisneros still no luck - here's the new error messages from the PyCall build:

Screen Shot 2022-10-12 at 9 46 53 PM

@jc-cisneros
Copy link
Contributor Author

@snairdesai I changed the process to build the Julia environment using a script instead of the .toml files. The Manifest.toml and Project.toml files will now be created in the /conda/base/envs/template_julia/share/julia/environments/template_juliadirectory (i.e., the Julia installed in the conda environment is managing a template_julia Julia environment with Pkg). Let me know if everything works on your end.

@snairdesai
Copy link
Contributor

@jc-cisneros No luck - errors seem a bit more severe. I've attached some screenshots below:


Screen Shot 2022-10-13 at 2 22 09 PM


Screen Shot 2022-10-13 at 2 22 30 PM


Screen Shot 2022-10-13 at 2 22 55 PM


Screen Shot 2022-10-13 at 2 23 20 PM


Screen Shot 2022-10-13 at 2 23 51 PM

@jc-cisneros
Copy link
Contributor Author

@snairdesai I have not encountered this error on my end, but my current best guess is that it might be related to the Python distribution the Julia environment is pointing to. The error message states that it is indeed using the Python distribution in the Conda package, but it might be using a Python distribution you have on your miniconda/base/bin instead of the one in your template julia environment. Let me test a possible solution on my end, and I will let you know when you can give it another try. Thank you!

@jc-cisneros
Copy link
Contributor Author

@snairdesai building the Conda environment + Julia environment setup has been tricky (trickier than writing the wrapper). I have completed a full run on a Docker Debian container (8b1c26b) and on my local Mac OS afterwards (4863c67). The Conda.jl package should be capable of dealing with the Python path issue. Let me know if it works on your end.

@snairdesai
Copy link
Contributor

@jc-cisneros Flagging I have the same errors as in the comment above.

Screen Shot 2022-10-18 at 11 22 11 AM


Screen Shot 2022-10-18 at 11 23 20 AM

@jc-cisneros
Copy link
Contributor Author

Thanks @snairdesai! Let me explain the latest changes:

  • All of the errors above are caused by the PyCall.jl package. By default, this package uses the Python version in the Conda directory. This might lead to issues when there are several Python versions living in the Conda directory (i.e. there are several environments). We can fix this in an interactive Julia session by changing the path of the Python Environment variable. Nevertheless, to make this change global we might have to touch a user configuration file after creating the Conda environment (see https://docs.julialang.org/en/v1/manual/environment-variables/). I think being able to call Python functions within Julia is not a feature we really need (we could simply run Python scripts within the same project), so this effort is not worth pursuing and it is out of the scope of this PR.
  • The new test script does not use the matplotlib backend for the plot. @snairdesai let me know if the version conflicts are gone and if the run_julia wrapper works.

@snairdesai
Copy link
Contributor

@jc-cisneros We're very close here - the plots are compiling now. Small issue, I had another terminal window open on my local computer when the Julia script ran, which I had to manually exit out of for the code to finish compiling. Ideally we would want this to automatically exit for the user. A screenshot of the outputs are below:


Screen Shot 2022-10-20 at 2 55 31 PM

Screen Shot 2022-10-20 at 2 56 48 PM

@snairdesai
Copy link
Contributor

@jc-cisneros Post your latest commit a45a0a2 - this compiles successfully! I'll proceed to a code review now.


using Pkg

Pkg.add(["DataFrames", "CSV", "StatsPlots", "Plots", "GR"])
Copy link
Contributor

Choose a reason for hiding this comment

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

@jc-cisneros For a future PR (perhaps not this one), we probably want to:

  1. specify versions of these packages explicitly, to maintain the transition to this format across R and Python
  2. in our check_setup script, notify the user if they have not properly installed the correct Julia packages.

Just tagging this here.

yearly_plot(df)

# Close gksqt plot backend program
run(`killall -9 gksqt`)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jc-cisneros

Interesting way to kill the terminal plot environment, agree it's the best we have for now.

- linearmodels
- julia #=1.8.2
# - pip=19
# - pip: # if there is a package available via pip but not with conda, can import like this:
# - EXAMPLE_PACKAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

@jc-cisneros

We'll want to track developments made by the conda-forge team such that when they have implemented the proper fixes, we migrate the calls to Julia and package installer directly to this conda_env.yaml file, rather than our separate julia_conda_env.jl script (much like we do for pip in R.

Copy link
Contributor

@snairdesai snairdesai left a comment

Choose a reason for hiding this comment

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

Small comment on the conda_env.yaml package versions, but otherwise this looks good! Hopefully the conda-forge developments coming down the pipeline will make this process easier.

setup/conda_env.yaml Outdated Show resolved Hide resolved
@jc-cisneros
Copy link
Contributor Author

@snairdesai thanks for the careful review! In the latest commit 8f0cec7, I added the versions of all packages used (as in master). I also commented out all the Julia-related lines, so that the default behavior in master is not installing Julia and producing the test plot. Throughout all these tests, the run_julia wrapper has been working as expected. I will stress in the final summary that until the julia-feedstock is further developed on conda-forge (see #67 (comment)), we will need this julia_conda_env.jl workaround (which as shown in this PR, can create conflicts if Python is called from Julia).

If everything looks good, I will proceed to merge the development branches of this PR and the PR on gslab_make to their respective master branches. The final step then would be to add summaries that close #67 and #54 on gslab_make.

@Houdanait
Copy link
Contributor

Hey @jc-cisneros @snairdesai !
Thanks so much for all the brilliant work here !
Please ping me once you're done merging this to the template as we will need this extension in another repo !

Copy link
Contributor

@snairdesai snairdesai left a comment

Choose a reason for hiding this comment

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

Thanks @jc-cisneros! The revisions look good to me - I'm approving this PR, and simultaneously the one for #54 on gslab_make.

@jc-cisneros
Copy link
Contributor Author

Thanks again for the review, @snairdesai! I will close the issues with a summary and merge this branch to master.

@jc-cisneros jc-cisneros merged commit 03cde18 into master Oct 22, 2022
@jc-cisneros jc-cisneros deleted the 67-julia-extension branch October 22, 2022 01:33
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.

Create run_julia wrapper Julia extension
3 participants