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

update v2 installation instructions page in docs #831

Merged

Conversation

kevingreenman
Copy link
Member

@kevingreenman kevingreenman commented Apr 23, 2024

Description

Copy link
Member

@JacksonBurns JacksonBurns left a comment

Choose a reason for hiding this comment

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

We should comment on Python versions somewhere. This is currently suggesting 3.11 as the only real option (which it kinda is to get all the features), but we should explain why that is and mention what 3.12 can do.

docs/source/installation.rst Outdated Show resolved Hide resolved
@kevingreenman
Copy link
Member Author

We should comment on Python versions somewhere. This is currently suggesting 3.11 as the only real option (which it kinda is to get all the features), but we should explain why that is and mention what 3.12 can do.

good point, can you remind me what things one can't do in 3.12?

@JacksonBurns
Copy link
Member

3.12 can't install ray for hpopt, but that might he it... cc @hwpang was there anything else?

@hwpang
Copy link
Contributor

hwpang commented Apr 23, 2024

Yeah as far as I know it's just ray tune doesn't support 3.12 yet

kevingreenman and others added 2 commits April 23, 2024 17:11
Co-authored-by: Shih-Cheng Li <scli@mit.edu>
@shihchengli
Copy link
Contributor

shihchengli commented Apr 23, 2024

  1. The torch-scatter is not a dependency, so the note in option 4 should be modified.
  2. For Build Image Locally, maybe it would be great to keep the steps written in v1.
  3. Idk if it is desirable to provide a GPU version Dockerfile.
  4. I think instead of saying adding the --gpus all to docker run can allow Chemprop to run on GPU within the container, we can say the --gpus command line option is used to specify which GPU devices gonna be used.

@JacksonBurns
Copy link
Member

Gpus in docker is non trivial. Requires extra system level software, manual configuration, etc... we have documented where people can start looking, but they have to do it themselves.

@kevingreenman
Copy link
Member Author

  1. The torch-scatter is not a dependency, so the note in option 4 should be modified.
  2. For Build Image Locally, maybe it would be great to keep the steps written in v1.
  3. Idk if it is desirable to provide a GPU version Dockerfile.
  4. I think instead of saying adding the --gpus all to docker run can allow Chemprop to run on GPU within the container, we can say the --gpus command line option is used to specify which GPU devices gonna be used.

@shihchengli thanks for your feedback! I've addressed 1, 2, and 4. And I agree with @JacksonBurns about 3 - this would be nice to have, but I think it's too complicated for now.

Co-authored-by: Shih-Cheng Li <scli@mit.edu>
@JacksonBurns
Copy link
Member

@kevingreenman can you update the instructions for building locally that are in the dockerfile as well?

chemprop/Dockerfile

Lines 5 to 10 in 7ab28fe

# Build this image with:
# docker build .
#
# Run the built image with:
# docker run --name chemprop_container -it <IMAGE_ID>
# where <IMAGE_ID> is shown from the output of the build command.

@kevingreenman kevingreenman merged commit d336467 into chemprop:main Apr 24, 2024
12 checks passed
@kevingreenman kevingreenman deleted the fix-installation-instructions-docs branch April 24, 2024 15:21
@kevingreenman kevingreenman linked an issue Apr 24, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants