Set `LD_LIBRARY_PATH` on `mamba activate` #1496

roomrys · 2023-09-12T19:11:17Z

Description

Some Linux users have been unable to utilize the GPU. Modifying some installation instructions from tensorflow, this PR sets the LD_LIBRARY+PATH on mamba activate (if installing SLEAP via the conda package) and gives instructions for setting the LD_LIBRARY_PATH manually (once and then automated on mamba activate) for conda from source.

Add instructions for CUDA issues with new TensorFlow versions #1390

Types of changes

Does this address any currently open issues?

Failure to Start GUI #1476

Outside contributors checklist

Review the guidelines for contributing to this repository
Read and sign the CLA and add yourself to the authors list
Make sure you are making a pull request against the develop branch (not main). Also you should start your branch off develop
Add tests that prove your fix is effective or that your feature works
Add necessary documentation (if appropriate)

Thank you for contributing to SLEAP!

❤️

Summary by CodeRabbit

New Feature: Switched from conda to mamba for package management, improving the speed and reliability of environment creation and package installation.
Bug Fix: Added a shell script that sets the LD_LIBRARY_PATH for Linux users with NVIDIA GPUs, resolving issues with GPU utilization.
Documentation: Updated the installation instructions in installation.md to reflect the switch to mamba and added guidance for Linux users with NVIDIA GPUs on how to set up their environment correctly.

coderabbitai · 2023-09-12T19:11:32Z

Walkthrough

This pull request introduces changes to improve the package build instructions and environment activation for Linux and Windows users. It adopts mamba as an alternative package manager, adds scripts to set up the LD_LIBRARY_PATH for CUDA GPU utilization, and updates the documentation accordingly.

Changes

File Path	Summary
`.conda/README.md`, `.conda/build.sh`	Replaced `conda` with `mamba` for creating and activating the environment. Added code to copy activate scripts to a specific directory.
`.conda/sleap_activate.sh`	Added a shell script that sets the `LD_LIBRARY_PATH` environment variable to help CUDA find GPUs.
`docs/installation.md`	Updated instructions for Linux users with NVIDIA GPUs to resolve GPU utilization issues.

🐇💻

In the land of code where the shadows lie,

Mamba slithers in, under the binary sky.

With a hop and a skip, the rabbit's on its way,

Making GPUs sing, in CUDA's bright array.

So here's to the changes, made with care,

To make our code run, smooth and fair. 🎉

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

Mention @coderabbitai in any review comment for bot assistance.
Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

coderabbitai

Review Status

Actionable comments generated: 1

Commits

Files that changed from the base of the PR and between e4fca4f and c24b9cb commits.

Files ignored due to filter (1)

environment.yml

Files selected for processing (4)

.conda/README.md (2 hunks)
.conda/build.sh (1 hunks)
.conda/sleap_activate.sh (1 hunks)
docs/installation.md (1 hunks)

Files skipped from review due to trivial changes (3)

.conda/README.md
.conda/sleap_activate.sh
docs/installation.md

Additional comments (Suppressed): 1

.conda/build.sh (1)

13-15: The installation of SLEAP itself remains unchanged and seems to be working as expected.

coderabbitai · 2023-09-12T19:12:51Z

.conda/build.sh

+# This will allow them to be run on environment activation.
+export CHANGE=activate
+
+mkdir -p "${PREFIX}/etc/conda/${CHANGE}.d"
+ls "${RECIPE_DIR}"
+cp "${RECIPE_DIR}/${PKG_NAME}_${CHANGE}.sh" "${PREFIX}/etc/conda/${CHANGE}.d/${PKG_NAME}_${CHANGE}.sh"


This new block of code is responsible for copying the activation scripts to a specific directory. This will allow them to be run when the environment is activated. However, there's no error handling in case the copy operation fails. It would be beneficial to add some error checking here to ensure that the script has been copied successfully.

- cp "${RECIPE_DIR}/${PKG_NAME}_${CHANGE}.sh" "${PREFIX}/etc/conda/${CHANGE}.d/${PKG_NAME}_${CHANGE}.sh" + cp "${RECIPE_DIR}/${PKG_NAME}_${CHANGE}.sh" "${PREFIX}/etc/conda/${CHANGE}.d/${PKG_NAME}_${CHANGE}.sh" || { + echo "Failed to copy ${PKG_NAME}_${CHANGE}.sh"; + exit 1; + }

The build just fails and gives us an error message if these lines end in failure.

@roomrys While it's true that the build will fail and provide an error message if these lines end in failure, adding explicit error handling can still be beneficial. It allows you to control the error message that is displayed, making it more informative and specific to the operation that failed. This can help with troubleshooting and understanding exactly what went wrong.

For example, without explicit error handling, you might get a generic "No such file or directory" error. With explicit error handling, you can provide a message like "Failed to copy ${PKG_NAME}_${CHANGE}.sh", which immediately tells you that the script failed to copy.

Additionally, explicit error handling allows you to control the exit code of the script. This can be useful if other scripts or systems are relying on specific exit codes for certain types of failures.

Here's how you could modify the code to include explicit error handling:

cp "${RECIPE_DIR}/${PKG_NAME}_${CHANGE}.sh" "${PREFIX}/etc/conda/${CHANGE}.d/${PKG_NAME}_${CHANGE}.sh" || { echo "Error: Failed to copy ${PKG_NAME}_${CHANGE}.sh to ${PREFIX}/etc/conda/${CHANGE}.d/"; exit 1; }

This way, if the copy operation fails, the script will print a specific error message and then exit with a status of 1.

talmo · 2023-09-12T19:45:02Z

.conda/sleap_activate.sh

+#!/bin/sh
+
+# Help CUDA find GPUs!
+export LD_LIBRARY_PATH=$CONDA_PREFIX/lib:$LD_LIBRARY_PATH


Are you sure this will work? I thought the cuda binaries were stored in the $CONDA_PREFIX/envs/{env_name}/lib or something.

Like, one blocker for upgrading to newer tensorflow was that we couldn't dynamically set this since users would have different env names.

If it's really in $CONDA_PREFIX/lib and it's not just hardcoding the path from the computer you built in (would Jinja do this?), then that's awesome and would allow us to move up to tf 2.9 I think!

If it doesn't work, then it might be ok to just include the extra line that people have to run in the install docs?

The $CONDA_PREFIX includes the environment name:

(nb4) talmolab@talmolab-01-ubuntu:~/sleap-estimates-animal-poses/pull-requests/sleap$ echo $CONDA_PREFIX /home/talmolab/micromamba/envs/nb4

!!!

If it's really in $CONDA_PREFIX/lib and it's not just hardcoding the path from the computer you built in (would Jinja do this?), ...

I think that might be the point of using cp now that you mention it (instead of echo ... >> ...).

sleap/.conda/build.sh

Line 23 in 21339e9

cp "${RECIPE_DIR}/${PKG_NAME}_${CHANGE}.sh" "${PREFIX}/etc/conda/${CHANGE}.d/${PKG_NAME}_${CHANGE}.sh"

...then that's awesome and would allow us to move up to tf 2.9 I think!

😨

talmo · 2023-09-12T19:45:33Z

environment.yml

+# If you are on Linux, have a NVIDIA GPU, and are getting the warning:
+
+# W tensorflow/stream_executor/platform/default/dso_loader.cc:64] Could not load dynamic 
+# library 'libcudart.so.11.0'; dlerror: libcudart.so.11.0: cannot open shared object 
+# file: No such file or directory
+
+# then activate the environment and run the commands:
+
+# mkdir -p $CONDA_PREFIX/etc/conda/activate.d
+# echo '#!/bin/sh' >> $CONDA_PREFIX/etc/conda/activate.d/libcudart_activate.sh
+# echo 'export LD_LIBRARY_PATH=$CONDA_PREFIX/lib:$LD_LIBRARY_PATH' >> $CONDA_PREFIX/etc/conda/activate.d/libcudart_activate.sh
+# source $CONDA_PREFIX/etc/conda/activate.d/libcudart_activate.sh


Possibly move to docs?

This is also in the docs under GPU support. If you dislike it here I can remove?

coderabbitai

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between c24b9cb and 21339e9 commits.

Files selected for processing (1)

docs/installation.md (1 hunks)

Files skipped from review due to trivial changes (1)

docs/installation.md

* Do not try to remove item if already deleted (#1498) * Set `LD_LIBRARY_PATH` on `mamba activate` (#1496) * Add version restrictions to tensorflow for pypi (#1485) * Remove `imageio` pin (#1501) * Reset LD_LIBRARY_PATH on deactivate (#1502) * Brown bag bump to 1.3.3 (#1484)

Set LD_LIBRARY_PATH on mamba activate

c24b9cb

roomrys requested a review from talmo September 12, 2023 19:11

roomrys marked this pull request as ready for review September 12, 2023 19:11

coderabbitai bot reviewed Sep 12, 2023

View reviewed changes

talmo reviewed Sep 12, 2023

View reviewed changes

Rename libcudart_activate.sh to sleap_activate.sh in docs

21339e9

coderabbitai bot reviewed Sep 12, 2023

View reviewed changes

roomrys requested a review from talmo September 12, 2023 20:18

roomrys and others added 2 commits September 12, 2023 16:22

Remove comments from environment.yml

94efb5f

Merge branch 'develop' into liezl/set-LD_LIBRARY_PATH-on-activate

c73d465

roomrys merged commit cdf8cba into develop Sep 12, 2023

roomrys deleted the liezl/set-LD_LIBRARY_PATH-on-activate branch September 12, 2023 23:24

roomrys mentioned this pull request Sep 14, 2023

Reset LD_LIBRARY_PATH on deactivate #1502

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set `LD_LIBRARY_PATH` on `mamba activate` #1496

Set `LD_LIBRARY_PATH` on `mamba activate` #1496

roomrys commented Sep 12, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 12, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

Pause Incremental Reviews

coderabbitai bot left a comment

coderabbitai bot Sep 12, 2023 •

edited

Loading

roomrys Sep 12, 2023

coderabbitai bot Sep 12, 2023

talmo Sep 12, 2023

roomrys Sep 12, 2023 •

edited

Loading

roomrys Sep 12, 2023 •

edited

Loading

talmo Sep 12, 2023

roomrys Sep 12, 2023

roomrys Sep 12, 2023

coderabbitai bot left a comment

Set LD_LIBRARY_PATH on mamba activate #1496

Set LD_LIBRARY_PATH on mamba activate #1496

Conversation

roomrys commented Sep 12, 2023 • edited by coderabbitai bot Loading

Description

Types of changes

Does this address any currently open issues?

Outside contributors checklist

Thank you for contributing to SLEAP!

Summary by CodeRabbit

coderabbitai bot commented Sep 12, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

Pause Incremental Reviews

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

roomrys Sep 12, 2023

Choose a reason for hiding this comment

coderabbitai bot Sep 12, 2023

Choose a reason for hiding this comment

talmo Sep 12, 2023

Choose a reason for hiding this comment

roomrys Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

roomrys Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

talmo Sep 12, 2023

Choose a reason for hiding this comment

roomrys Sep 12, 2023

Choose a reason for hiding this comment

roomrys Sep 12, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

Set `LD_LIBRARY_PATH` on `mamba activate` #1496

Set `LD_LIBRARY_PATH` on `mamba activate` #1496

roomrys commented Sep 12, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 12, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

coderabbitai bot Sep 12, 2023 •

edited

Loading

roomrys Sep 12, 2023 •

edited

Loading

roomrys Sep 12, 2023 •

edited

Loading