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

Fix install-dependencies.sh for Ubuntu 18.04 #585

Merged
merged 14 commits into from
Sep 14, 2018

Conversation

jamessynge
Copy link
Contributor

  • Update for conda 4.4
  • Avoid astroscrappy build problem by requiring Python 3.6
  • Make the install process quieter by logging the output of
    the astrometry make commands, instead flooding the terminal
    with content most folks won't care about.
  • Add some more context messages to the output.
  • Create an logs/install directory specific to this install.- Move all of the command line arg handling to the start of the
    file.
  • Add comments to most functions.
  • Apply most fixes suggested by shellcheck, e.g. quoting
    variable expansions and replacing fgrep with grep -F.
  • Insert changes into the appropriate file used by the shell
    at login.

- Move all of the command line arg handling to the start of the
file.
- Add comments to most functions.
- Apply most fixes suggested by shellcheck, e.g. quoting
  variable expansions and replacing fgrep with grep -F.
- Insert changes into the appropriate file used by the shell
  at login.
- Update for conda 4.4
- Avoid astroscrappy build problem by requiring Python 3.6
- Make the install process quieter by logging the output of
  the astrometry make commands, instead flooding the terminal
  with content most folks won't care about.
- Add some more context messages to the output.
- Create an logs/install directory specific to this install.
@jamessynge jamessynge requested a review from a team September 9, 2018 02:05
@codecov
Copy link

codecov bot commented Sep 9, 2018

Codecov Report

Merging #585 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #585      +/-   ##
===========================================
- Coverage    70.45%   70.43%   -0.02%     
===========================================
  Files           64       64              
  Lines         5520     5520              
  Branches       769      769              
===========================================
- Hits          3889     3888       -1     
- Misses        1423     1425       +2     
+ Partials       208      207       -1
Impacted Files Coverage Δ
pocs/camera/camera.py 89.72% <0%> (-1.09%) ⬇️
pocs/serial_handlers/protocol_arduinosimulator.py 75.76% <0%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29d420f...18126b5. Read the comment docs.

It may often be possible to refresh the installation without
logging out and back in, but sometimes things are different
enough that that would be best (or at least the easiest way
to get a good environment).
@jamessynge
Copy link
Contributor Author

This script doesn't quite work right w.r.t. detecting the presence of conda due to conda/conda#7753. I'll workaround that by checking for the environment variables that they do export.

Use a checksum of the file listing to determine whether the conda
packages (including pip) have changed. Approximately correct.
@jamessynge
Copy link
Contributor Author

After a ton of experiments and commits, this is ready for review. @wtgee Could you handle that?

@jamessynge
Copy link
Contributor Author

Oh, note that it is currently configured to install the "latest" version of astrometry (i.e. not stable). Let me know if you want me to return it to 0.72 or to the current latest, 0.76.

Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

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

Approving with minor comments although I'd still like to test this out.

One thing we are not using is an Anaconda environment, which we could save as a single yaml file and then host on the panoptes Anaconda channel. I haven't looked into that much but might be worth exploring (for future PR).


# Want git for upgrading to the latest version. Probably already installed
# but not necessarily (i.e. it is possible to download a zip of POCS, which
# includes this file).
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I've wondered if we should skip git and just install the zip. An idea for later if we are finding we want to force atomic updates or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, an interesting idea. The downside is that it makes it harder to keep track of what is installed.

echo
echo "Creating conda environment 'panoptes-env' with Python 3.6"
# 3.6 works around a problem building astroscrappy in 3.7.
conda create -n panoptes-env --yes --quiet python=3.6
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the python version might be better stored in a variable somewhere else? It's 3.6 for now but will certainly change at some point and this is buried deep in the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will do.

# We need to know the version that is in the tar file in order to
# enter that directory.
ASTROMETRY_VERSION=""
for version in $(find astrometry.net-* -maxdepth 0 -type d | sed "s/astrometry.net-//")
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just rename the directory when we untar or wget it?

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're thinking of the name of the tar file, while this is the name of the top-level directory inside the tar file, which isn't known when we download "latest".

echo "Fetching astrometry into directory $(pwd)"

wget "${URL}"
Copy link
Member

Choose a reason for hiding this comment

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

Rename dir here with -O flag. Comment below.

function install_astrometry_indices() {
echo_bar
echo
echo "Fetching astrometry and other indices."
Copy link
Member

Choose a reason for hiding this comment

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

Don't we still somewhere need to tell the astrometry.cfg file about the data dir? Or does it default to data inside it's base dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall needing to do that.

if [[ "${DO_APT_GET}" -eq 1 ]] ; then
install_apt_packages
fi


# Install Mongodb, a document database. Used by POCS for storing environment
# readings, from which weather plots are generated.
Copy link
Member

Choose a reason for hiding this comment

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

Also stores observations metadata. Not really important but comment undersells how much we use it to some extent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

fi


# Install cfitsio, native tools for reading and writing FITS files.
Copy link
Member

Choose a reason for hiding this comment

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

Did cfitsio move to a conda package and if so does it need a separate option for installing? Would we ever not install cfitsio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now get it via apt-get, hence the DO_CFITSIO=0 at the top of the file. Ideally we'd use conda.

Copy link
Contributor Author

@jamessynge jamessynge left a comment

Choose a reason for hiding this comment

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

New commit TBD.


# Want git for upgrading to the latest version. Probably already installed
# but not necessarily (i.e. it is possible to download a zip of POCS, which
# includes this file).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, an interesting idea. The downside is that it makes it harder to keep track of what is installed.

echo
echo "Creating conda environment 'panoptes-env' with Python 3.6"
# 3.6 works around a problem building astroscrappy in 3.7.
conda create -n panoptes-env --yes --quiet python=3.6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will do.

# We need to know the version that is in the tar file in order to
# enter that directory.
ASTROMETRY_VERSION=""
for version in $(find astrometry.net-* -maxdepth 0 -type d | sed "s/astrometry.net-//")
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're thinking of the name of the tar file, while this is the name of the top-level directory inside the tar file, which isn't known when we download "latest".

function install_astrometry_indices() {
echo_bar
echo
echo "Fetching astrometry and other indices."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall needing to do that.

if [[ "${DO_APT_GET}" -eq 1 ]] ; then
install_apt_packages
fi


# Install Mongodb, a document database. Used by POCS for storing environment
# readings, from which weather plots are generated.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

fi


# Install cfitsio, native tools for reading and writing FITS files.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now get it via apt-get, hence the DO_CFITSIO=0 at the top of the file. Ideally we'd use conda.

@wtgee
Copy link
Member

wtgee commented Sep 11, 2018

What files does this depend on? Ideally I want to wget this file and run it and do nothing else but I guess it's not quite to that point.

Pulled down the file. Env variables weren't set. Pulled down the default-env-vars.sh. Running it doesn't actually set my env vars, so I have to manually do that before I can run script. I know we want to clean up env vars otherwise but seems like install script should set them up if they don't exist.

@wtgee
Copy link
Member

wtgee commented Sep 11, 2018

Because some of this runs as sudo it requires that the env variables be set up for the sudo user. Not sure if that is intended or needed.

Edit: Requires correct permissions set on $PANDIR for $PANUSER. Should be explicit.

panoptes@pan008-nuc:~/Downloads$ head install-dependencies.sh 
#!/bin/bash -e

# Run with --help to see your options. With no options, does a complete install
# of dependencies, though attempts to reuse existing installs.

THIS_DIR="$(dirname "$(readlink -f "${0}")")"
THIS_PROGRAM="$(basename "${0}")"

if [[ -z "${PANDIR}" || -z "${POCS}" || -z "${PAWS}" || -z "${PANLOG}" ||
      -z "${PANUSER}" ]] ; then

panoptes@pan008-nuc:~/Downloads$ echo $PANDIR
/var/panoptes

panoptes@pan008-nuc:~/Downloads$ echo $POCS
/var/panoptes/POCS

panoptes@pan008-nuc:~/Downloads$ echo $PAWS
/var/panoptes/PAWS

panoptes@pan008-nuc:~/Downloads$ echo $PANLOG
/var/panoptes/logs

panoptes@pan008-nuc:~/Downloads$ echo $PANUSER
panoptes

panoptes@pan008-nuc:~/Downloads$ ./install-dependencies.sh 
mkdir: cannot create directory ‘tmp’: Permission denied

panoptes@pan008-nuc:~/Downloads$ sudo ./install-dependencies.sh 
Please set the Panoptes environment variables, then re-run this script.

panoptes@pan008-nuc:~/Downloads$ 

Generate /var/panoptes/set-panoptes-env.sh, and source that
from the rc file of the user's shell (e.g. $HOME/.bashrc).

Explicitly choose astrometry version 0.76 because testing with
"latest" as the version causes lots of downloads.

Stop suggesting that the user logout and log back in, and instead
recommend sourcing the generated shell setup file.

Not yet done with conda related updates. For one thing, if --no-conda
is specified, the remaining conda updates may fail. Sigh.
I may want to change what --no-conda means.
We're now using the version we can get from apt-get,
so removing this support for the sake of reducing
complexity.
Ensure that conda.sh is sourced where needed, repeating if necessary.
Should have escaped the $ in $PATH so that the literal
string "$PATH" was added, not the current value of PATH.
the setup script is executed, not the value when the
install script is executed.
If we have the user "run python setup.py install", without
appending "; pytest", then once they run the first command
they'll have lost sight of the second command to run.
@jamessynge
Copy link
Contributor Author

Our Intel NUC Setup doc describes how to run this, including how to setup the environment variables.

My intent though is to eventually have a "setup computer" script, that is used right after creating the first user (e.g. "james") on the computer, and takes care of creating the "panoptes" user, sets up the cron jobs, etc., leaving you with a system that is ready to go... though we'd need some second action that actually enables POCS to run automatically. This new script would certainly be derived from this one.

@wtgee
Copy link
Member

wtgee commented Sep 13, 2018

Our Intel NUC Setup doc describes how to run this, including how to setup the environment variables.

And I apologize, I should have followed the docs while doing this. Let's consider my testing to be that of someone who thinks they know better so doesn't have to read the instructions so tries anyway. :)

I'm going to test out newest version on NUC later today and will follow instructions.

Agree that the fully automated one will be in future.

@wtgee
Copy link
Member

wtgee commented Sep 14, 2018

Tried latest version on clean install while following docs and worked well.

One note, the install script gives instructions to follow at the end, which I did. This is slightly different than what the docs say. I made a comment in the docs.

@jamessynge jamessynge merged commit d39379a into panoptes:develop Sep 14, 2018
@jamessynge jamessynge deleted the tidy-install-dependencies branch September 14, 2018 15:09
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.

2 participants