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

Pull request for #61: restructure YAML in template #74

Merged
merged 7 commits into from
Aug 24, 2017
Merged

Conversation

Shun-Yang
Copy link

@stanfordquan , please test it and check if I have achieved our goals listed in #61. Note that you will need to wait for gslab-econ/gslab-python#106 to pull in and switch to the 4.1.0 dev verson of gslab_python.

@Shun-Yang Shun-Yang changed the base branch from master to 4.1.0 August 23, 2017 05:56
@Shun-Yang Shun-Yang requested a review from qlquanle August 23, 2017 05:57
@yuchuan2016
Copy link
Contributor

Excuse me for my interrupting, but do we have any rule of using 'abc' versus "abc" in a python script? I think we might want to use quotes consistently .

@qlquanle
Copy link
Contributor

@arosenbe and I had a decent debate about it and we settled on consistency within scripts. I'm fine with specifying one over the other. My habit of using double quotes came from Stata.

@Shun-Yang
Copy link
Author

Good point. I wasn't paying attention on this. All double quotes work for me.

@yuchuan2016
Copy link
Contributor

I think we are using single quotes throughout the current SConscript and SConstruct, as well as all gslab_python scripts. So I would propose we stick to single quotes as default.

@yuchuan2016
Copy link
Contributor

@stanfordquan , I can review this if you have not started.

@Shun-Yang Shun-Yang requested a review from yuchuan2016 August 23, 2017 18:59
@qlquanle qlquanle removed their request for review August 23, 2017 20:23
@yuchuan2016
Copy link
Contributor

@Shun-Yang , actually I'm not quite sure about why we need a config_user_template.yaml.

  • We would prompt to users if no config_user.yaml is detected in the load_yaml_value function here and create a config_user.yaml automatically.
  • We would provide default stata executable in Windows/Unix when no stata executable is specified.
  • The cache path and release path in the template do not work almost 100%.
  • If the user does not need stata/release/cache, why do we force them to have such a config_user.yaml file to confuse them?

Perhaps I miss something, but I do not see/remember any benefits of this template. Could you remind me of the discussion about this? I would prefer that a config_user.yaml is created only if people call it using load_yaml_value function.

@qlquanle
Copy link
Contributor

Good point. The default values and prompts given by our yaml reader does serve as a "template". Once @Shun-Yang weighed in we may want to ping PIs about this.

@Shun-Yang
Copy link
Author

@yuchuan2016 , this is indeed good point. I think the config_user_template idea is first raised here by Matt. In retrospect, I agree with you that there is really no need on creating such a template as config_user will be created anyway if the yaml reader cannot find one. Currently, we only ask users to type in their Stata executable. Do we want to also prompt for cache_dir and release_dir (of course, they can type none for all these)? This way, the user at least know that they can set these up via config_user.

@yuchuan2016
Copy link
Contributor

@Shun-Yang , I think it will prompt to user to enter the release directory if people want to make a release here, and it will prompt to users to enter the cache directory if people specify mode=cache here.

This question is whether we want the repo to contain a user-config.yaml at the beginning regardless of whether it's needed or not, or create such a file when the users need one. I would vote for the second. The first is more likely to cause future breakdown, since after we copy the template, it won't prompt to users to enter the value, but just use the incorrect default value in the template.

@Shun-Yang
Copy link
Author

Per discussion, we decide that we don't implement this config_user_template.yaml for now, as if the user does not have a config_user.yaml file, the default is to create a new one with prompt asking user for information. I confirm that both stata-executable and cache_dir prompt will properly appear. release.py is currently down. A new issue gslab-econ/gslab_python#111 will fix that.

@gentzkow FYI. Please see discussion in this thread. Let us know if you think there is still need to create config_user_template.yaml.

@Shun-Yang
Copy link
Author

@yuchuan2016 , I have removed config_user_template. Let me know if you have other comments on this, or I can pull this in.

@yuchuan2016
Copy link
Contributor

@Shun-Yang , great. I have nothing to add. We perhaps want to update the README.md to give instructions on how to turn on the dependencies. But it's up to you whether we want to update it now or update after everything has finished.

@Shun-Yang Shun-Yang merged commit 91f3990 into 4.1.0 Aug 24, 2017
@Shun-Yang Shun-Yang deleted the issue61_YAML branch August 24, 2017 18:07
qlquanle pushed a commit that referenced this pull request Mar 3, 2018
* Pull request for #62: Separate sub-directory for analysis and paper/slides (#72)

* #62 reorganize repo

* #62 add provenance

* #62 allow provenance to check missing files

* #72 response to PR and comments

* #72 small name change

* Pull request for #67: Create release/lg for large files (#73)

* #67 add code to fake lg dataset, add lg to gitignore

* #67 add lg protocol to readme.md

* Update constants.yaml

* #67 blow up data file to do large release

* #67 change hist in default stata code to limit number of pins

* #67 delete old logs

* #67 readme change

* #67 manually copy new analysis files to paper input

* rerun paper-slides

* Pull request: move top level logs to release (#75)

* #61 first pass

* #61 second pass

* #61 switch to single quote

* #93 aux first pass

* #93 first pass

* #93 second pass

* Pull request for #61: restructure YAML in template  (#74)

* #61 first pass

* #61 second pass

* #61 switch to single quote

* #61 rm config_template

* add instruction on switching required softwares

* rm unused config_user_template

* #97 testing anything template

* gslab-python#97 demonstration scripts

* Pull Request for #66: Force integration with gslab_stata (#77)

* #66 revise stata scripts

* #66 modify README

* #66 small change in readme

* #66 add note in README

* gslab-python#97 change generator syntax

* gslab-python#97 respond to PR

* Pull Request for #64: Port example scripts to multiple languages (#76)

* #64 add examples for python stata matlab R, mute everything except python

* #64 add example of latex

* #64 modify dependencies

* #64 turn on git-lfs

* #64 response to PR

* #64 update readme and modify sample code

* solve conflict after merge

* rm accidental commit of local scripts

* Pull Request for #63: Make current dependencies optional (#78)

* #63 add builder check

* #63 add autimatic config scripts

* #63 response to PR

* #63 change the default to not upgrade

* #63 force installing of gslab_python

* #63 small change per PR

* #63 small change

* Pull request for creating size warning when git-lfs not presented (#79)

* first pass on template

* add in size warning for paper dir

* fix bugs

* add in option for git attributes

* add in paper dir

* Pull request for documenting and organizing debrief  (#81)

* #69 first pass

* implement PR

* small edits

* small edits

* Pull request for gslab_python#99: Add cache reconfiguration code (#83)

* gslab-econ/gslab_python#99 switch to scons local

* gslab-econ/gslab_python#99 delete scons local, lfs track .zip

* gslab-econ/gslab_python#99 commit scons-local-2.5.1

* gslab-econ/gslab_python#99 scons python scripts for each sub

* gslab-econ/gslab_python#99 remove top-level scons.zip

* #83 gslab-econ/gslab_python#99 update commands in readme.

* 83 gslab-econ/gslab_python#99 symlink to scons.py

* #83 gslab-econ/gslab_python#83 scons.py -> run.py, mention global scons in readme

* #83 gslab-econ/gslab_python#99 arg in main call

* Pull request for #82: Update docs for 4.1.0  (#84)

* #82 readme for 4.1.0, fix scons in paper_slides, build_anything in analysis

* #82 update data download procedure

* #82 add jupyter notebooks to .gitignore

* #82 build_anything demonstration

* #82 output

* #82 uncomment builder

* #84 #82 instructions to connect analysis and paper_slides

* #84 #82 respond to PR

* #84 #82 more PR

* change template

* Pull request for #86: Integrate make_provenance into SConstruct (#87)

* #86 incorporate external logging from gslab-econ/gslab_python#131

* #87 #86 PR

* #132 #131 delete stray file

* #87 #86 comment out some builders in analysis/SConstruct

* Pull request for #88: Grab all paths when recording inputs (#89)

* #88 allow inputs specificed as str or dict

* #88 fix sconscript after flatten input in config_global

* pep8 e713 change

* Pull request for #90: Load PYTHONPATH in SCons ENV (#91)

* #90 store pythonpath

* #90 remove stray pythonpath definition

* Fix location of sconstruct.log.

* Capitalization in readme

* Pull request for #92: Turn off default logging during SCons dry-run (#93)

* #92 turn off logging on dry run, uses gslab-econ/gslab_python#141

* #93 #92 move dry-run checking for logs into gslab_python

* Fix typo in readme

* #95 switch to scons 3 (#97)

* Update SCons local version in README

* practice task

* realized my mistake and reverting

* Pull request for #96: Complete MG points 1-5  (#98)

* #95 switch to scons 3

* .#96 update to new builders and config and rerun

* #96 drop _ before anything builder

* #98 #96 rerun with new builders

* #98 #96 typo in README

* #98 #96 better advertise package dependencies in config scripts

* #98 #96 mention reset config_user in README FAQ
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.

3 participants