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

Fix For T3 Docker Build Issues #101

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Fix For T3 Docker Build Issues #101

merged 2 commits into from
Apr 27, 2023

Conversation

sourcesync
Copy link
Collaborator

@sourcesync sourcesync commented Apr 3, 2023

Changes:

  • The changes in this PR appear to fix the T3 docker build issues discovered recently by "gary88christie"
  • Testing was limited - only tested on a GPU test machine which ran Ubuntu 20.04 using the T3/README instructions (using the "xs" dataset)
  • Note that the FAISS baseline was built and tested with 1.7.1 (the original version) but there may be new versions
  • It was built/tested on a system with CUDA11 and I know there are new versions of CUDA

@sourcesync
Copy link
Collaborator Author

sourcesync commented Apr 3, 2023

@harsha-simhadri Hey are any of the CICD automated tests working for you? If I'm viewing my PR/branch CICD errors correctly, it says something about python3.6 not being supported (?)

@harsha-simhadri
Copy link
Owner

@harsha-simhadri Hey are any of the CICD automated tests working for you? If I'm viewing my PR/branch CICD errors correctly, it says something about python3.6 not being supported (?)

I think we need to bump up the python version number CI/CD. Looks like 3.6 is gone.
https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python#excluding-a-version

@harsha-simhadri
Copy link
Owner

I raised #102 to bump up python version to 3.10. First let's fold all 2021 PRs to main, stamp it v0.3, then raise python version, and then run these tests to merge?

@sourcesync
Copy link
Collaborator Author

I raised #102 to bump up python version to 3.10. First let's fold all 2021 PRs to main, stamp it v0.3, then raise python version, and then run these tests to merge?

OK @harsha-simhadri I approved and merged it. CI testing works again! :) Let's wait a little while for PR63 and P64 in order to have the PR author verify them. Otherwise, once CI tests pass on PR101 and PR103, you can approve and merge and we should arrive PR63 and PR64 since PR103 is my best attempt to reconcile those.

@sourcesync
Copy link
Collaborator Author

@harsha-simhadri Hey i'm trying to resolve the latest CI failing on this branch. I've merged the most recent master but getting "Nothing to run" on ngt-t1, ngt-t2, kst_ann_t1, and kst_ann_t1. I think I'm missing something obvious but not sure what it is...

@maumueller
Copy link
Collaborator

@sourcesync It seems that these builds fail because the installation of the library fails, e.g., the ngt implementation complains

Step 5/11 : RUN git clone -b neurips21 https://github.com/yahoojapan/NGT.git
 ---> Running in 1ca0823ddf9b
Cloning into 'NGT'...
fatal: Remote branch neurips21 not found in upstream origin

I'm not sure how to handle this without involving the original authors.

@sourcesync
Copy link
Collaborator Author

I'm not sure how to handle this without involving the original authors.

@maumueller Ah cool. Thanks for looking into this.

Maybe it's best to default disable those for now so that CICD passes for upcoming submissions branched off master.

I'm looking at past emails and it looks like NGT are T1 and T2 (?)

PR algo name new index inde submitted status bigann deep msspacev msturing ssnpp text2image
58 team11 Y Y bigann download failed ?? 0.64955   0.712211    
60 puck-t1 Y Y cant test            
66 ngt-t1 Y N waiting on index          
69 kst_ann_t1 Y Y memory overflow 0.71219 0.71219 0.764542 0.756419    
71 buddy-t1 Y Y bigann 0.62765          
  baseline       0.63451 0.65028 0.728861 0.703611 0.75378 0.069275
                     
62 kota-t2 N Y   0.950859   0.904001 0.939817 ERROR  
66 ngt-t2 Y N waiting on index          
70 bbann Y Y       0.7602   0.88573 0.487202
  baseline       0.94913 0.93706 0.90095 0.93564 l2 0.48854

@maumueller
Copy link
Collaborator

Right, we never got some results from ngt. kst_ann_t1 on the other hand was part of the evaluation and it seems that the repo just disappeared on their site.

Adding @NJU-yasuo to this discussion: Was there a reason to remove this?

@NJU-yasuo
Copy link
Contributor

@maumueller
Copy link
Collaborator

@NJU-yasuo Is it correct that all we need is this single file https://github.com/NJU-yasuo/big-ann-benchmarks/blob/f2a7f1f0374361deb366dc1ea16260c6049e8e42/benchmark/algorithms/opt_faiss.py and it works on top of faiss?

@maumueller maumueller merged commit 8aafc95 into main Apr 27, 2023
@maumueller
Copy link
Collaborator

I'm merging this and will fix the CI runs in a different PR.

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.

4 participants