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

Documentation for raft ANN benchmark containers. #1833

Merged
merged 20 commits into from
Oct 9, 2023

Conversation

dantegd
Copy link
Member

@dantegd dantegd commented Sep 19, 2023

Adding instructions to how to use the ANN benchmark containers, as well as general benchmark doc improvements.

@dantegd dantegd added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 22, 2023
@dantegd dantegd marked this pull request as ready for review September 25, 2023 18:16
@dantegd dantegd requested a review from a team as a code owner September 25, 2023 18:16
@dantegd dantegd requested a review from cjnolet September 25, 2023 18:16
@dantegd dantegd changed the title Documentation for raft ANN benchmark containers as well as conf/other updates to python package. Documentation for raft ANN benchmark containers. Sep 25, 2023
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Went through the docs and did some tiny touchups/fixes to the wording. overall I think the content is there.

Small nitpick: Do you mind also adding the cuda-version to the conda install command example in README.md since it's a one-liner?

@@ -36,7 +36,9 @@ def positive_int(input_str: str) -> int:

def validate_algorithm(algos_conf, algo, gpu_present):
algos_conf_keys = set(algos_conf.keys())
print("algo", algo)
Copy link
Member

Choose a reason for hiding this comment

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

Should these be removed? I'm on board w/ printing something when a specific algorithm is being asked for but it can't be executed (such as when a gpu algorithm is being requested in cpu-only).

docs/source/raft_ann_benchmarks.md Outdated Show resolved Hide resolved
docs/source/raft_ann_benchmarks.md Outdated Show resolved Hide resolved
docs/source/raft_ann_benchmarks.md Outdated Show resolved Hide resolved
docs/source/raft_ann_benchmarks.md Outdated Show resolved Hide resolved

**Note:** The user inside the containers is `root`, to workaround this the scripts in the containers fix the user of the output files after the benchmarks are run. If the benchmarks are interrupted, the owner of the datasets/results produced by the container will be wrong, and can be fixed by the user.

2. **Using the preinstalled `raft_ann_benchmarks` python package**: The docker containers are built using the conda packages described in the following section, so they can be used directly as if they were installed manually following the instructions in the next section. This is the option that allows the full flexibility of the benchmarking scripts, and recommended for advanced users. This allows using the full flexibility of the scripts. To use the python scripts directly, an easy way is to use the following command:
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to explain the conda install first and then the docker install? That way instead of saying "in the next section" where users haven't yet been exposed to the content, we can say "as outlined in the previous section" .

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, changed the order of the sections

docs/source/raft_ann_benchmarks.md Outdated Show resolved Hide resolved
docs/source/raft_ann_benchmarks.md Outdated Show resolved Hide resolved
docs/source/raft_ann_benchmarks.md Outdated Show resolved Hide resolved
docs/source/raft_ann_benchmarks.md Show resolved Hide resolved
@dantegd dantegd changed the base branch from branch-23.10 to branch-23.12 September 27, 2023 21:26
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM!

@cjnolet
Copy link
Member

cjnolet commented Oct 9, 2023

/merge

@rapids-bot rapids-bot bot merged commit f979607 into rapidsai:branch-23.12 Oct 9, 2023
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
Development

Successfully merging this pull request may close these issues.

2 participants