Closed ShiqiYang2022 closed 2 months ago
@ShiqiYang2022 Thanks! Will do this after I finish the initial onboarding in the DChan Lab :D
@ShiqiYang2022 Sounds good, thanks! I'll start by completing the practice tasks with the archived template - first on my personal PC and again on the work Macbook when I get it next week. I'll then look over the new template.
I have created a repo using this template to run tests as suggested. I also created a separate branch to conduct the tests, deriving from a dedicated issue.
I first ran the whole repo to test, then pushed versions after running the output with python (164c752), stata (a209c6c), and R (099cf07). My comments are below:
Clicking on "Use this template" as suggested in wiki - Setup does not work properly. The repository is copied but all the large files in inputs_for_examples
are corrupted, which causes issues when cloning the repo locally. This is because files stored with Git LFS fail to copy from the template. Per Github documentation , “template repository cannot include files stored using Git LFS.”
inputs_for_examples
using regular Git for the templateIn wiki - Setup, the code to run check_setup.sh
as written uses absolute root paths and yields an error: bash: /lib/shell/check_setup.sh: No such file or directory
.
bash /lib/shell/check_setup.sh
bash ./lib/shell/check_setup.sh
The latex code for compiling the slides gives errors which does not prevent it from compiling the files, but prevents the run_latex
bash script to finish the cleanup. This leads to more errors because the bash file does not move the output to the output folder, it is instead saved in the source folder and does not get removed during cleanup (and does not get pushed because of .gitignore . In the slides .tex template, errors show up for example because of the use of the metropolis package. Similar issues would arrise if there are any errors with the paper .tex file.
lib/shell/run_latex
file to somehow ensure the cleanup part is always run even if there are errors in .tex compilation.In examples/latex/README.md, the suggested wildcard use seems incorrect and gives an error.
Some of these might be already covered in the Roadmap, but my main general comments are:
check_setup
command should maybe yield errors if programs are not found in path for command line usage?make.sh
always copy the files in the output folder of the previous directory to the input folder of the new one? I realize this may not be optimal but it would reduce the amount of work for the users, thinking of people not used to using bash (removing the need to worry about the "cp XX" lines in each of the make.sh
directories). @ShiqiYang2022 , please let me know what you think! Thanks :D
@lucamlouzada Thanks! This is very clear, and the suggestions are great and super helpful.
For your specific comments, I agree with all. Would it be convenient for you to open a branch linked to this issue and address those accordingly? We will bring those fixes back to master
. For Git-lfs item, I think this item is also in our to-do list here. Maybe we can try to combine the fix with having this context in mind.
For General suggestions, those all make sense to me. We will discuss more in our meeting. If you look at my lyx draft which pulls all the to-dos, you can find that the 1st bullet related to (M), 2nd and 3rd related to (A), 4th related to (C). Please feel free to add those as your comment into the lyx file (in the "Notes" part) and push to related branch 16-plan-supervise
so we pull thoughts together for the meeting!
Thanks @ShiqiYang2022 !
I have pushed three changes referring to my specific suggestions above:
lib/shell/run_latex
file to ensure the cleanup part is always run (see my comment here). I basically put the clean up part of the code in a dedicated function and used trap
to ensure it is always called on exit (see explanation here).examples/latex/README.md
file so the wildcard is properly used in the sample lines of code I have also edited the Wiki in d2d6479 to add a small fix to the sample code in Setup, also per my comment here.
Regarding the other points, I will read the relevant notes carefully and add comments to the associated issue in 16-plan-supervise.
Please confirm whether I should create a PR to merge the three above changes to main. Thanks again.
@lucamlouzada Thanks so much! The changes all looks great to me.
For PR, I suggest we can hold up merging for a bit and wait for @linxicindyzeng to familiarize herself with new template. Let me know if you view a different way, thanks!
On the Git LFS problem Luca mentions here, storing all files using regular git is one solution but not the only one, especially if we prefer to keep open the option to exploit advantages of LFS.
How Git LFS in templates works in my understanding:
When someone uses our template to create a new repository, it clones all files, including any Git LFS pointers, into their new repository, but not the actual large files. The large files are stored in the LFS server of our repository and are not automatically transferred to the user's repository’s LFS server, so the user must set up Git LFS in their new repository and download the large files. After running git lfs install
, they should also run git lfs pull
in the local directory that they have cloned:
[404] Object does not exist on the server
If A) is done correctly, they can then push the metadata (pointer files) to their own Git repository (git push origin main
) and the large file data to their LFS server (git lfs push --all origin
).
Currently there is another minor problem preventing this procedure. git clone git@github.com:gentzkow/GentzkowLabTemplate.git
would return this message:
Encountered 1 file that should have been a pointer, but wasn't: examples/powerpoint/output/slides.pdf
I did some checks and found that when Matt committed slides.pdf
on Aug 2, it was not committed as a Git LFS pointer file but as a full binary file. Since the .gitattributes
file specifies that .pdf files are tracked by Git LFS, this file was likely added before Git LFS was set up or before the .gitattributes
was configured properly. So we could consider keep using Git LFS if we fix this minor issue and write detailed guidelines in README.md.
@ShiqiYang2022 @lucamlouzada
Nice @linxicindyzeng ! I had seen these messages about the pointer, but did not investigate further. Thanks for the great explanation. We can discuss these alternatives in the meeting tomorrow, though I suspect using regular git for these files will be a simpler approach as it requires no extra steps from the users running the examples - and does not prevent the remaining parts of the template from using Git LFS if needed. cc @ShiqiYang2022
@lucamlouzada I agree. If the purpose of using Git LFS in the template is just to demonstrate two .jpg's and one .csv, we should get rid of LFS and use normal git. If we want to make room for future adjustments in the template where LFS may become essential, we should keep it.
Hi all: Chiming in here on the LFS issue, I agree w/ this conclusion. We've actually known about this issue in the past and I just forgot about it when I was adding LFS to the template.
What we need to achieve, ideally, is:
The template can run out of the box without Git LFS installed and it functions correctly as a template on Github.
The template is preconfigured so that someone who does want to use LFS can do so easily.
We have good ways of enforcing that all of our lab projects must be using LFS.
One possibility for (2) would be that we can make LFS an extension in /examples/
-- the directory would just need to have a readme explaining the steps and our default .gitattributes
.
@gentzkow I agree. Making it an available but not necessary extension is a great idea. Using normal Git for example inputs should solve 1. For 3., since we are not structurally forcing the usage of LFS due to 1., we can ensure this elsewhere (e.g., in a manual or README). If we want to have the choice of structurally ensuring the usage of LFS (such that not using it would cause errors), we can also code it up and include this check in the extension folder?
bash run_all.sh
and bash run_latex.sh
no longer report errors after Luca's three commits in branch 15-familiarize.Hi @linxicindyzeng, I just wanted to confirm whether you have any other suggestions to add to this branch or if we can wrap up this issue and commit my three changes to main. Thanks!
I don’t have other suggestions :) thanks!
On Mon 16 Sep 2024 at 13:23, Luca Moreno Louzada @.***> wrote:
Hi @linxicindyzeng https://github.com/linxicindyzeng, I just wanted to confirm whether you have any other suggestions to add to this branch or if we can wrap up this issue and commit my three changes to main. Thanks!
— Reply to this email directly, view it on GitHub https://github.com/gentzkow/GentzkowLabTemplate/issues/15#issuecomment-2353901839, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAGR7UI2RDU25XQINBHRCD3ZW4445AVCNFSM6AAAAABNW4IQF2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJTHEYDCOBTHE . You are receiving this because you were mentioned.Message ID: @.***>
Threads continues in its PR #17.
Summary: In this issue we noted some minor improvements to the new lab template. Some changes were applied in the associated PR #17.
The changes were:
In https://github.com/gentzkow/GentzkowLabTemplate/commit/6e793d89a5dcc6f3b1f75ab7ea1d2881cfaca8a0, I fixed the examples/latex/README.md file so the wildcard is properly used in the sample lines of code. There was a minor issue with my fix which was handled in dd6e30a following a review by @ShiqiYang2022 .
Deliverables:
The goal of this issue is for @lucamlouzada @linxicindyzeng to familiarize themselves with the new lab template. All of our lab's new projects will use the new
GentzkowLabTemplate
, though we are continuing using our archived template for existing projects.Specifically, I would like both of you complete the following steps:
issue15-test-yourname
and push the results from each of the steps to that branch.I think @lucamlouzada has not get the push access to this template yet, so you can prioritize your other onboarding tasks for now. Matt will grant your access soon.
Thanks all for your work!