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

Improvements to ANN Benchmark Python scripts and docs #1734

Merged
merged 7 commits into from
Aug 11, 2023

Conversation

divyegala
Copy link
Member

No description provided.

@divyegala divyegala added improvement Improvement / enhancement to an existing function breaking Breaking change labels Aug 10, 2023
@divyegala divyegala self-assigned this Aug 10, 2023
@divyegala divyegala requested a review from a team as a code owner August 10, 2023 18:01
@cjnolet cjnolet changed the title Improvements to ANN Benchmark Python scripts Improvements to ANN Benchmark Python scripts and docs Aug 10, 2023
@divyegala divyegala requested a review from a team as a code owner August 10, 2023 22:48
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.

Couple minor things but these are huge improvements. Thanks!

if args.configuration:
conf_filepath = args.configuration
else:
conf_filepath = os.path.join(scripts_path, "conf", f"{args.dataset}.json")
Copy link
Member

Choose a reason for hiding this comment

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

Can we enforce the constraint here that either the dataset or the configuration argument needs to be specified (one of them is required but both can't be specified).

Copy link
Member Author

Choose a reason for hiding this comment

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

Since dataset has a default, that enforces that one of them is specified. Should I remove the default and throw an error instead if both are unspecified?

docs/source/raft_ann_benchmarks.md Show resolved Hide resolved
docs/source/raft_ann_benchmarks.md Outdated Show resolved Hide resolved
@@ -34,6 +34,7 @@ dependencies:
- nccl>=2.9.9
- ninja
- nlohmann_json>=3.11.2
- pyyaml
Copy link
Member

Choose a reason for hiding this comment

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

Matplotlib doesn't seem to be getting installed as a dependency of the libraft-ann-bench conda package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's part of the bench-ann environment file. It should not be a dependency of the libraft-ann-bench conda package because that package is just the executables

@cjnolet
Copy link
Member

cjnolet commented Aug 11, 2023

/merge

@rapids-bot rapids-bot bot merged commit 25b6916 into rapidsai:branch-23.10 Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function
Projects
Development

Successfully merging this pull request may close these issues.

3 participants