Re: [PR] Update benchmarks to handle pip dependencies for user [arrow-datafusion]

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #10070:
URL: https://github.com/apache/arrow-datafusion/pull/10070#discussion_r1564037926


##########
benchmarks/.gitignore:
##########
@@ -1,2 +1,3 @@
 data
-results
\ No newline at end of file
+results
+.venv

Review Comment:
   the script makes a `venv` directory, not a `.venv` directory, should this be `venv`?



##########
benchmarks/bench.sh:
##########
@@ -440,6 +440,9 @@ compare_benchmarks() {
     fi
 
     echo "Comparing ${BRANCH1} and ${BRANCH2}"
+    python3 -m venv ./${SCRIPT_DIR}/venv

Review 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
   
   ```shell
   # 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 tpch
   ```
   
   



##########
benchmarks/compare.py:
##########
@@ -24,14 +24,8 @@
 from typing import Dict, List, Any
 from pathlib import Path
 from argparse import ArgumentParser
-
-try:

Review Comment:
   I think this is still a helpful error message



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org