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

add GPU option for KNN classifier, update requirements.txt #8

Merged
merged 3 commits into from Apr 18, 2023

Conversation

chaudatascience
Copy link
Collaborator

Some small updates on the evaluation part:

  • Add enviroment.yml for setting up Conda Environment for the project
  • Add GPU option for KNN classifier
  • Minor format updates.

@Zitong-Chen-16
Copy link
Collaborator

Thank you for catching the typos and including the gpu options! It looks good to me. My only question is for the environment setting. Juan and I discussed earlier that we might want to use pip instead of conda which is why I included the requirement.txt file, but of course conda is also a viable option. Should we just use conda instead @jccaicedo ?

@chaudatascience
Copy link
Collaborator Author

chaudatascience commented Apr 14, 2023

Good point! Using pip may be a better choice here if someone only wants to check their model's performance (on their current env) and doesn't want to switch to a new env to just evaluate it. I'll revert back to the requirements.txt.

@chaudatascience chaudatascience changed the title add enviroment.yml, add GPU option for KNN classifier add GPU option for KNN classifier, update requirements.txt Apr 14, 2023
@chaudatascience
Copy link
Collaborator Author

I have reverted back to using pip and have tested the installation on my machine.

Copy link
Collaborator

@jccaicedo jccaicedo left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jccaicedo
Copy link
Collaborator

Thank you for the PR, @chaudatascience ! @Zitong-Chen-16 can you review and test on your env? After it works, feel free to merge!

@Zitong-Chen-16
Copy link
Collaborator

Hey @chaudatascience! I tried running the updated code but ran into an issue. The code ran successfully without the gpu though. But I ran the code with the gpu option (i.e. with --use_gpu), I received the following error. Were you able to run it on your machine with the gpu option?

Faiss assertion 'err == CUBLAS_STATUS_SUCCESS' failed in void faiss::gpu::runMatrixMult(faiss::gpu::Tensor<float, 2, true>&, bool, faiss::gpu::Tensor<T, 2, true>&, bool, faiss::gpu::Tensor<IndexType, 2, true>&, bool, float, float, cublasHandle_t, cudaStream_t) [with AT = float; BT = float; cublasHandle_t = cublasContext*; cudaStream_t = CUstream_st*] at /project/faiss/faiss/gpu/utils/MatrixMult-inl.cuh:265; details: cublas failed (13): (512, 1536) x (31060, 1536)' = (512, 31060) gemm params m 31060 n 512 k 1536 trA T trB N lda 1536 ldb 1536 ldc 31060

@chaudatascience
Copy link
Collaborator Author

Yes. I was able to run it with GPU.
I tested by creating a new conda environment and installing the packages listed in the requirements.txt as follows

conda create -n test_env python=3.9
conda activate test_env
cd MorphEm
pip install -r requirements.txt
python run_benchmark.py --use_gpu

Can you try again to see if the error still persists?

hmm, it seems that some people have encountered a similar problem, as reported link1 and link2.
The author recommends using Conda conda install -c pytorch faiss-gpu to install it. If the one above doesn't work, maybe using conda to install can help.

@Zitong-Chen-16
Copy link
Collaborator

That solved the problem! I did the exact same thing to create the environment and pip install via the file earlier and ran into the issue, but conda install faiss-gpu and pytorch seemed to fix it.

@Zitong-Chen-16
Copy link
Collaborator

Can I merge the pull request now then @chaudatascience ? Maybe we could update the readme to include the conda install faiss-gpu after

@chaudatascience
Copy link
Collaborator Author

Yeah, I think we can update it later.

@Zitong-Chen-16 Zitong-Chen-16 merged commit 726e8ed into broadinstitute:main Apr 18, 2023
@Zitong-Chen-16
Copy link
Collaborator

Sounds good! Thank you!!

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.

None yet

3 participants