Update benchmarks to handle pip dependencies for user #10070
Conversation
alamb
left a comment
There was a problem hiding this comment.
Thank you for this contribution @JacobOgle -- I like where this is headed
| data | ||
| results | ||
| results | ||
| .venv |
There was a problem hiding this comment.
the script makes a venv directory, not a .venv directory, should this be venv?
There was a problem hiding this comment.
Good catch! I changed it last second, but will update!
| fi | ||
|
|
||
| echo "Comparing ${BRANCH1} and ${BRANCH2}" | ||
| python3 -m venv ./${SCRIPT_DIR}/venv |
There was a problem hiding this comment.
I think automatically creating / removing a virtual env each run may be surprising to people already managing their enviornment
What would you think about adding a new command that worked like bench.sh data like bench.sh venv
That might work like
# creates virtual environment in $SCRIPT_DIR/venv (alongside data)
# prints out a message about how to activate it
$ bench.sh venv
$ source venv/bin/activate.sh # do what bench.sh tells you
...
$ bench.sh run tpchThere was a problem hiding this comment.
I do like that idea a bit more than what I have implemented now. I would be open to taking that route and updating the PR. Thanks for the feedback, its much appreciated!
| from pathlib import Path | ||
| from argparse import ArgumentParser | ||
|
|
||
| try: |
There was a problem hiding this comment.
I think this is still a helpful error message
There was a problem hiding this comment.
Thats a good point and I can roll this back. Since rich isn't STL its probably a good idea to keep this.
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
|
Thanks again for the contribution -- I look forward to seeing the next iteration |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Which issue does this PR close?
Closes #10022
Rationale for this change
What changes are included in this PR?
I have updated the bash script to create a temporary virtual env for the python script to run in. This venv installs the rich dependency and upon completion removes the virtual env. This keeps the user system python installs clean. Also, the python script no longer has to try/except for the dependency.
Are these changes tested?
Yes, locally everything works well.
Are there any user-facing changes?
All users steps are the same. This process should go mostly unnoticed.