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

User local installation documentation and bash script #418

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

emprzy
Copy link
Collaborator

@emprzy emprzy commented Dec 9, 2024

Describe your changes.

This pull request creates a new section of documentation (in a file called local-installation.md) that delineates more up-to-date installation instructions for new flepiMoP users. Within these instructions, users are instructed to run a bash script (also created in this PR: local_install_or_update.sh). This script:

  • automatically sets the environment variable FLEPI_PATH to be equal to the current working directory and tests that this is the right directory (exits on fail with message)
  • Verifies that the user's conda environment is set up correctly and aligns with demands of flepiMoP (exits on fail with message)
  • installs gempyor + dependencies and all R packages + dependencies

Important note: The existence local-installation.md documentation page renders some information on other documentation pages redundant or outdated, so I will need to go back and adjust accordingly after I get a thumbs up on the general structure of these two files! @TimothyWillard

Does this pull request make any user interface changes? If so please describe.

The user interface changes are slightly altered local installation instructions for flepiMoP.
Those are reflected in updates to the documentation in local-installation.md.

What does your pull request address? Tag relevant issues.

This pull request addresses GH #401 .

Created two files (`local-installation.md` and `local_install_or_update.sh`) that will likely make it easier for new flepiMoP users to install flepiMoP onto their machine.
Copy link
Contributor

@TimothyWillard TimothyWillard left a comment

Choose a reason for hiding this comment

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

Great start! I left comments mostly on the docs rather than the script since I think the script will be easier to tackle after docs are clear and can run the script after fixing the first major bug. My big picture comments are:

  1. Let's merge this int dev instead of main to be consistent with the new development workflow.
  2. We need to be clear that this install script is for Mac/Linux users and we will need to add a windows equivalent or steps for windows users but that's not your responsibility @emprzy. We'll find and work with a windows user who can test that part.
  3. I think it's worth pointing out somewhere why we recommend conda and use that as our default installation. I think there is resistance to conda since it adds an extra layer for users and we want to make clear why it's worth jumping that hurdle.

documentation/gitbook/how-to-run/local-installation.md Outdated Show resolved Hide resolved
documentation/gitbook/how-to-run/local-installation.md Outdated Show resolved Hide resolved
documentation/gitbook/how-to-run/local-installation.md Outdated Show resolved Hide resolved
documentation/gitbook/how-to-run/local-installation.md Outdated Show resolved Hide resolved
documentation/gitbook/how-to-run/local-installation.md Outdated Show resolved Hide resolved
documentation/gitbook/how-to-run/local-installation.md Outdated Show resolved Hide resolved
documentation/gitbook/how-to-run/local-installation.md Outdated Show resolved Hide resolved
build/local_install_or_update.sh Outdated Show resolved Hide resolved
build/local_install_or_update.sh Outdated Show resolved Hide resolved
@TimothyWillard TimothyWillard added documentation Relating to ReadMEs / gitbook / vignettes / etc. high priority High priority. installation Relating to installation / upgrade / migration. next release Marks a PR as a target to include in the next release. labels Dec 12, 2024
@emprzy emprzy changed the base branch from main to dev December 16, 2024 18:32
@emprzy emprzy marked this pull request as ready for review December 18, 2024 15:02
Copy link
Contributor

@TimothyWillard TimothyWillard left a comment

Choose a reason for hiding this comment

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

I get this error:

twillard@epid-iss-MacBook-Pro ~/D/G/H/flepiMoP ((cbecd91c))> ./build/local_install_or_update
Using '/Users/twillard/Desktop/GitHub/HopkinsIDD/flepiMoP' for $FLEPI_PATH.
Using 'flepimop-env' for $FLEPI_CONDA.

CondaError: Run 'conda init' before 'conda activate'

I think due to the wrong ordering for conda setup mentioned inline.

Comment on lines 22 to 45
# Create the conda environment if it doesn't exist
if ! conda env list | grep -q "$FLEPI_CONDA"; then
conda env create --name flepimop-env --file $FLEPI_PATH/environment.yml
fi

# Ensure that conda environment is named flepimop-env
if [ -z "${FLEPI_CONDA}" ]; then
export FLEPI_CONDA="flepimop-env"
echo "Using '$FLEPI_CONDA' for \$FLEPI_CONDA."
fi

# Activate the conda environment
conda activate $FLEPI_CONDA

# Check that the name of the FLEPI_CONDA variable matches the environment that has been set up, exit if not
FLEPI_CONDA_ENV_MATCHES=$( conda info --envs | awk '{print $1}' | grep -x "$FLEPI_CONDA" | wc -l )
if [ "$FLEPI_CONDA_ENV_MATCHES" -eq 0 ]; then
echo "Issue with conda environment: ensure you have set up your conda environment and it has the correct name."
exit 1
fi

# Load the conda environment
eval "$(conda shell.bash hook)"
conda activate $FLEPI_CONDA
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be in the wrong order/have the wrong logic. The

    conda env create --name flepimop-env --file $FLEPI_PATH/environment.yml

should be in side of the

if [ "$FLEPI_CONDA_ENV_MATCHES" -eq 0 ]; then
    .....
fi

block? And then there shouldn't be two calls to conda activate right? I think we want the created env to have the name pulled from $FLEPI_CONDA right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay I re-downloaded conda then tested the script (with changes) locally. Everything seems to work. I think there were issues with my conda environment but it should be working correctly now. Ready again for a test on your end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Relating to ReadMEs / gitbook / vignettes / etc. high priority High priority. installation Relating to installation / upgrade / migration. next release Marks a PR as a target to include in the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants