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

Made taxbrain install script more usable #817

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

codekansas
Copy link
Contributor

I had a lot of difficulty hard-resetting my environment (partly being on a slow internet connection) and I figured it would be good to make some changes to the install script. Mainly:

  1. Making it more interactive, so that the user can decide what should be run and what shouldn't. To get the previous behavior, use yes | ./install_taxbrain_server.sh
  2. Making it so that it always fails if something goes wrong, using set -e option - this avoids a bunch of the || return 1 stuff from the previous script and is probably a more sane way to do things
  3. Batched conda / pip installs (this was previously messing with me because of the aforementioned poor internet connection)
  4. Added some documentation on what is doing what

I also noticed that there were a lot of redundant source activate aei_dropq && source webapp_env.sh so I added the first part to the webapp_env.sh script, so you only have to do source webapp_env.sh. Maybe there was a reason for keeping them separate? I'm not sure. @hdoupe

@hdoupe
Copy link
Collaborator

hdoupe commented Feb 1, 2018

Thanks for re-working the install_taxbrain_server.sh script. It's much faster now. I've always taken an "ain't broke, don't fix it" approach with this since I didn't fully understand what was going on.

I'm going to to need some time to go over this and make sure everything is still working as expected.

@@ -67,7 +67,7 @@ if prompt_user "Install conda requirements?"; then
fi
done
# Places these packages last, to install taxcalc, etc. at the end.
PACKAGES+=('btax' 'ogusa')
PACKAGES+=('btax' 'ogusa' 'taxcalc')
echo "conda install $CHANNEL ${PACKAGES[@]}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you still specify which version of these packages that we want to use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could do that after the script runs, or by changing the script. I guess right now it just gets the most recent one available.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to be able to specify the version in the script. It's pretty common to have to pin to an older version. Each parameter change and API change requires a change on the PolicyBrain side. These tend to happen a lot. And on occasion they break something on the PolicyBrain side that we don't have time to solve at the moment.

.travis.yml Outdated
@@ -8,6 +8,7 @@ install:
- wget http://repo.continuum.io/miniconda/Miniconda-latest-Linux-x86_64.sh -O miniconda.sh
- bash miniconda.sh -b -p $HOME/miniconda
- export PATH="$HOME/miniconda/bin:$PATH"
- python -c 'import os,sys,fcntl; flags = fcntl.fcntl(sys.stdout, fcntl.F_GETFL); fcntl.fcntl(sys.stdout, fcntl.F_SETFL, flags&~os.O_NONBLOCK);'
- pushd deploy
Copy link
Collaborator

Choose a reason for hiding this comment

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

That was a fun one. See #799

@hdoupe
Copy link
Collaborator

hdoupe commented Feb 2, 2018

This PR looks good. Once you add back the capability to set the taxcalc, btax, and ogusa versions, this should be good to go.

@GoFroggyRun can you take a look at this PR?

@GoFroggyRun
Copy link
Contributor

@codekansas thanks for simplifying the installation script as well as providing those documentation.

I don't have much technical output regarding the script, but was wondering whether it‘d make sense to update README.md (Local Deployment Setup in particular) within this PR?

@codekansas
Copy link
Contributor Author

@hdoupe I believe it just uses the versions of those that are in the various requirements files. I'd thought there was something important about the install order (which there seemed to be from the previous script) but it doesn't seem to be the case.

@GoFroggyRun It shouldn't change anything, as it just gives the user more options on what they want to do and makes things download faster.

@hdoupe
Copy link
Collaborator

hdoupe commented Feb 8, 2018

@Kpinkelman would you mind taking a look at this script? Here are a few things you can check:

  1. If django=1.9 is specified in requirements.txt, will this script install django 1.9?
  2. If taxcalc=0.10.2 is specified in conda-requirements.txt, will this script install taxcalc=0.10.2? Repeat for different versions of btax and ogusa.
  3. Compare the time required to setup the environment with the current install_taxbrain_server.sh script and this proposed version. (You should be able to do this via time ./install_taxbrain_server.sh --all)

Run these tests with ./install_taxbrain_server.sh --all.

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