PR for #18: Improve error handling

[lucamlouzada]

Closes #18.

Changes and deliverables:
In issue #18, I made significant changes to the way errors are handled in the lab template. These changes were pushed in one commit per file, as follows:

• In 796332a, I changed run_python.sh
• In fc0e95e, I changed run_R.sh
• In 182cce5, I changed run_stata.sh
• In efc8d95, I changed run_latex.sh
• In 404e2d7, I changed run_shell.sh
• In af61c3e, I changed all the four make.sh files in the 1_data, 2_analysis, 3_slides, and 4_paper directories

A more detailed explanation for the changes is written in #18 (comment).

Review:
These are significant changes and therefore require a careful and thorough review. My suggested framework for review is as follows:

• Review the changes and explanations in the scripts and understand the main logic behind them, keeping in mind that one of the goals of these scripts is that they are as direct and short as possible and therefore if you think of ways to make them more efficient, please let me know
• Run example scripts as usual for R, Python, Stata, shell (run the my_shell_script), and compile paper and slides with latex. Check the successful output in the terminal and log files of the whole repository.

To test the behavior of error handling, force some errors as follows:

• Use a library or command that doesn’t exist in each of the R, Python, Stata and shell scripts, as well as latex

• Specify a path for the copy inputs line in make.sh that leads to a non-existing file

• Comment out the run_xx line for the appropriate language so that the shell script runs into an error like “run_python command not found"

• Try to call a script that doesn’t exist (e.g run_python fakescript.py)

• Change local_env.sh so that the paths for each program are wrong (e.g change python3 to python5)

• To test latex: in 4_paper, try running my_project.tex without copying my_project.bib to the source directory (this will cause the latex to compile with errors); also test adding a non-existing jpg to the .tex script

• (added in Sep 27th) : Conduct tests above in both zsh and bash

I am assigning either @ShiqiYang2022 , @Xingtong-Jiang , or @linxicindyzeng to review.

[ShiqiYang2022]

Just a note for our workflow for peer reviews in template development and other projects that have >=2 RAs:

I am requesting @Xingtong-Jiang, @linxicindyzeng and myself as initial reviewers. Whoever come first to review this pull request should remove the request from other labmates, and post a comment here indicating the peer review is in progress.

After the labmates sign off for this PR, we will invite Matt to review this PR.

[Xingtong-Jiang]

Hi all, I’ll take on the initial review for this PR. I’m removing the request from other labmates and starting the review.

[ShiqiYang2022]

Per conversation with @Xingtong-Jiang, I can take over this PR.

[lucamlouzada]

Thanks @ShiqiYang2022 ! Note that I just pushed some updates discussed in #18 (comment).

[lucamlouzada]

Hi @ShiqiYang2022 , I have added a final fix described in #18 (comment) and this should now be ready to review. Note the extra step in the review framework to test behaviors in zsh as well. Thanks again!

[gentzkow]

@lucamlouzada @ShiqiYang2022 This is looking great.

One issue I noted: If I try to run a script that doesn't exist:

run_stata wrangle_dataXXX.do "${LOGFILE}"

the error message makes it sound like the script exists but fails:

I think we probably want to add a check that the target script exists in the run_xxx commands so we return a nicer error message in this case.

Longer term to dos (not part of this issue):

1. Consider abstracting some of the setup and cleanup steps in make.sh to separate .sh helper scripts, both to simplify make.sh and to reduce redundancy.
2. Make unit tests for the template so we can check that everything is working correctly automatically.

[lucamlouzada]

I think we probably want to add a check that the target script exists in the run_xxx commands so we return a nicer error message in this case.

That makes sense, @gentzkow, thanks. Just implemented that in 30a11ed.

Consider abstracting some of the setup and cleanup steps in make.sh to separate .sh helper scripts, both to simplify make.sh and to reduce redundancy.
Make unit tests for the template so we can check that everything is working correctly automatically.

Also great points, will add these to the next steps file in #16.

[ShiqiYang2022]

Thanks @lucamlouzada for the detailed work here! The error handling looks overall very great, while I also think there are a lot of places we can improve. I left my comment in each conversation.

Meanwhile, I will keep reviewing the rest of items per #19 (comment). I will start addressing those later tomorrow or Wednesday.

[ShiqiYang2022]

We don't have exit 1 here, so that means the script captures the exit code of the Python command inside this shell but does not properly propagate this back to the main make.sh script. If there’s an error, it logs it but does not exit with the appropriate error code.

[lucamlouzada]

You are right here @ShiqiYang2022 , but we need to think about what behavior we want. I suggest you look into that while testing the scripts and let me know what you think - see my reply in your previous comment.

[ShiqiYang2022]

@lucamlouzada My worries is that we have nothing returned to make.sh here, not about choosing between exit 1 and return 1. I think we at least need to return the value, either error or not error into make.sh. Does that make sense?

[lucamlouzada]

Yes, I think you are right in terms of proper coding. Perhaps the correct approach is to propagate the error code whenever it happens (with no need to propagate 0 if it is successful). Do you agree? If so, I will implement this now before you start testing the scripts. @ShiqiYang2022

[ShiqiYang2022]

@lucamlouzada Yes that's exactly what I meant, just add another exit 1 or return 1 in the corresponding script (and check all run_xxx.sh) to make sure it returns 1 to the make.sh!

[lucamlouzada]

I have pushed these changes in 9e7d338. Now, the run_xx scripts will propagate the error status to make.sh as I added exit 1 to all the error clauses. The behavior of exit 1 vs return 1 is not the same in this case (see here for a great explanation), but I conducted some tests and believe exit 1 is better since we are running these scripts inside a subshell in make.sh. This also increases the consistency between zsh and bash: now, from what I have tested, the difference is that in bash the error in run_xx causes make.sh to exit the subshell, without triggering the make.sh error trap, while in zsh it exits the subshell and triggers the make.sh error trap. If we used return 1 instead, the parent make.sh does not detect the error and does not leave the subshell. I believe this is now ready for you to test @ShiqiYang2022 . Thanks again for all your help and feedback