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

Reviewer 2 request 1 changes for improved instructions on installing GOMC package #107

Merged
merged 13 commits into from
Aug 5, 2024

Conversation

CalCraven
Copy link
Collaborator

Add installation instructions for GOMC in README and docs, as well as… fix tests for improved error messages and automated checking of PATH for GOMC installation

… fix tests for improved error messages and automated checking of PATH for GOMC installation
@CalCraven CalCraven requested a review from bc118 August 3, 2024 15:45
bc118 and others added 6 commits August 3, 2024 20:22
Changed ‘GOMC_binary_path’ from None to “” .  

Doing this to see if it changes
Removed () from “” the file that sets the GOMC binary path here
Removed “GOMC/bin” as I thing the file is in the path
Added name of binary file= “GOMC_CPU_NVT" to look for in testing in GOMC installed
@bc118
Copy link
Collaborator

bc118 commented Aug 4, 2024

I took a quick look at this and here is my recommendations, All tested in RHEL:

@CalCraven Another thought and I actually like this better . We may want to avoid building it in "opt" as many not experience users will not be able to get rid of it. Why not build it in the actual conda env directly and symlink it to the conda bin directory.

This should also eliminate having to modify their bash.rc (meaning not messing it up and likely not differences between Mac and Ubuntu and RHEL).

Note the subprocess command. Despite the GOMC binary (GOMC_CPU_NVT) being available in my terminal through the auto-path setup when activating themosdef_dihedral_fitenv, it still required python to run this subprocess command to be able to access the path auto-setup conda env path. This may be the reason for the Mac vs. Ubuntu error, but not 100% sure.

I would add these instructions directly to the mosdef_dihedral_fit docs, and then in the error if GOMC binary (GOMC_CPU_NVT) not found, reference back to these docs. We should forward them to our docs which shows the simple and minimal install. Referencing GOMC docs is fine, but we should try not to get them to build it via the GOMC docs, as it is complicated, more unneeded disk space, and likely will result in more errors from building with GPUs.

This will need changed in a few files with checks/errors if

Code and notes :

Need to add to all effected python files:
import subprocess
**In the dihedral fit code, if “gomc_binary_directory”=None (if not default, make it so). We may want to check if conda exists first or print an error if this command fails maybe try/except if statement to see if it is in the path or the local path (=“”) to get those who have it there (this will eliminate issue with the Docker build(s) also)
gomc_binary_directory=
if gomc_binary_directory=None:
gomc_binary_directory = subprocess.check_output("echo ${CONDA_PREFIX}/bin", shell=True, text=True).strip()

Setup the GOMC binary (GOMC_CPU_NVT) in the correct:

conda activate mosdef_dihedral_fit
or (from the activated env)
cd $CONDA_PREFIX (env needs to be activated so will only work when in that env, which is likely what we want)

git clone https://github.com/GOMC-WSU/GOMC.git --branch v2.75a
cd GOMC
chmod u+x metamake.sh

These options turn off all but the GOMC_CPU_NVT ensemble, which is all that is really needed and used for mosdef_dihedral_fit. Specify setup is with anaconda, which the only way "mosdef_dihedral_fit" it is currently distributed so we should be fine
./metamake.sh NVT # this only builds the 1 we need, so it will avoid excess space and GPU build errors.

Note: was tested with copy not ls , so may need to change a few Python checks or just use copy (‘cp’ instead of ‘ln -s’)
ln -s $CONDA_PREFIX/GOMC/bin/GOMC_CPU_NVT $CONDA_PREFIX/bin # create a symlink to standard/auto-sourced conda/envs/<active_env_path>

Copy link
Collaborator

@bc118 bc118 left a comment

Choose a reason for hiding this comment

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

LGTM, just the 1 comment

## Quick Installation/Setup

```bash
mamba install -c conda-forge mosdef-dihedral-fit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we mention can do 'conda install' but 'mamba' recommended like the docs?

This is the only suggestion I have, otherwise looks good! Good work!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed the conda mention. above. Approved

@bc118 bc118 merged commit 5e855d5 into GOMC-WSU:main Aug 5, 2024
6 checks passed
@bc118 bc118 mentioned this pull request Aug 20, 2024
25 tasks
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