Skip to content

Conversation

rohit211-s
Copy link

@rohit211-s rohit211-s commented Sep 11, 2025

Fixes #465

Changes

  • Added --columns command-line argument to knnPerfTest.py
  • Updated gradle runKnnPerfTest task to support -Pcolumns property
  • Maintains backward compatibility - existing behavior unchanged when no columns specified

@rohit211-s rohit211-s changed the title Add --columns parameter to knnPerfTest.py for custom output selection Add --columns parameter to knnPerfTest.py for custom output selection Fixes #465 Sep 11, 2025
@rohit211-s rohit211-s changed the title Add --columns parameter to knnPerfTest.py for custom output selection Fixes #465 Add --columns parameter to knnPerfTest.py for custom output selection Sep 11, 2025
@dsmiley
Copy link

dsmiley commented Sep 11, 2025

Thanks for contributing during the CoC hackathon! You have the honors of the first PR contribution :-)

Copy link
Collaborator

@msokolov msokolov left a comment

Choose a reason for hiding this comment

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

This looks cool. I am concerned about relying on gradle though -- I think typical usage is to run by python3 src/main/knnPerfTest.py . if we do that, columns will be None by default I think? Also, it would be nice to have a docstring to that --help would tell me what the columns are that I can select. I haven't really followed the work to add a gradle runner and / or use argparse, but I think argparse generally provides docs like that

@rohit211-s
Copy link
Author

You're absolutely right about typical usage being direct Python execution Mike. The implementation works correctly for both approaches:

Direct Python usage (primary):
bash
python3 src/python/knnPerfTest.py --columns="recall,latency(ms)"

Gradle usage (optional convenience):
bash
./gradlew runKnnPerfTest -Pcolumns="recall,latency(ms)"

The core functionality is in the Python script - the gradle changes just pass through the parameter for users who prefer the gradle workflow (I generally use ./gradlew). When --columns is not provided, args.columns is None and the code falls back to default behavior, so direct Python usage works perfectly.

Would you prefer I remove the gradle changes and keep only the Python implementation?

@msokolov
Copy link
Collaborator

Would you prefer I remove the gradle changes and keep only the Python implementation?

No, I was just confused how this works at first. I must have missed some of the changes that I see now, not sure how ...

@msokolov
Copy link
Collaborator

can you show the output of knnPerfTest.py --help after this change?

@msokolov
Copy link
Collaborator

also, it looks like there's a failing test - maybe indentation or something?

@mikemccand
Copy link
Owner

also, it looks like there's a failing test - maybe indentation or something?

Try make tidy and see if it fixes the failure?

@rohit211-s
Copy link
Author

can you show the output of knnPerfTest.py --help after this change?

Yeah sure mike

image

@msokolov
Copy link
Collaborator

still fails with:

# validate sources with ruff linter
/home/runner/work/luceneutil/luceneutil/.venv/bin/ruff check src/python
src/python/knnPerfTest.py:143:18: C401 Unnecessary generator (rewrite as a set comprehension)
    |
141 |     return set()
142 |
143 |   selected_set = set(col.strip() for col in selected_columns.split(","))
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C401
144 |   all_headers = set(OUTPUT_HEADERS)
145 |   return all_headers - selected_set
    |
    = help: Rewrite as a set comprehension

Is there a way to run such tests locally? Maybe make does it?

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.

Enable option to select output parameters in knnPerfTest.py
4 participants