Skip to content

Conversation

@tmi
Copy link
Contributor

@tmi tmi commented Jun 13, 2024

Which issue does this PR close?

Closes #10022

(Note: this PR follows up on #10070 which worked on the same issue but looks like got kinda stale)

Rationale for this change

Make it more convenient to run bench.sh compare

What changes are included in this PR?

Updates to bench.sh to include a new command venv, and edit the compare command to use the respective venv

Separates requirements.txt in case compare.py would ever need something else

Are these changes tested?

I tested by hand ./bench.sh compare and ./bench.sh venv, in multiple scenarios:

  • create empty venv with bench.sh, then run compare
  • have existing venv, update with bench.sh venv, then run compare
  • have no venv but install rich into system, and just run compare
    In all cases compare works correctly.

Are there any user-facing changes?

No -- if the compare worked for the user before, it will still do, regardless of whether they used venv or not.
If it didn't work, then the user would see improved error message, run ./bench.sh venv without a need to configure any env var, and the new venv would be created in a gitignored folder in the repo

@tmi tmi force-pushed the benchmarksVenv branch from 52cc167 to 2b33a97 Compare June 13, 2024 13:28
Copy link
Contributor

@alamb alamb 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 -- thank you @tmi

@alamb
Copy link
Contributor

alamb commented Jun 13, 2024

I merged up to get a clean CI run

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @tmi for the nice first contribution

@comphead comphead merged commit 38bd893 into apache:main Jun 14, 2024
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
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.

Update benchmarks README to require rich installation

3 participants