gentzkow / template_archive

19 stars 36 forks source link

PR for #67: Julia extension #68

Closed jc-cisneros closed 2 years ago

jc-cisneros commented 2 years ago

This pull request closes https://github.com/gslab-econ/gslab_make/issues/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:

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

snairdesai commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

snairdesai commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

Thanks @snairdesai! Let me explain the latest changes:

snairdesai commented 2 years ago

@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 commented 2 years ago

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

jc-cisneros commented 2 years ago

@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 https://github.com/gentzkow/template/issues/67#issuecomment-1273944247), 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 commented 2 years ago

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 !

jc-cisneros commented 2 years ago

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