Skip to content

Test tools, verification and some bug fixes#66

Merged
kdvalin merged 9 commits into
mainfrom
test_tools
Feb 17, 2026
Merged

Test tools, verification and some bug fixes#66
kdvalin merged 9 commits into
mainfrom
test_tools

Conversation

@kdvalin

@kdvalin kdvalin commented Feb 12, 2026

Copy link
Copy Markdown
Member

User description

Description

This PR does a couple things

  1. Moves test_tools to $HOME/test_tools which is a well known location we are standardizing on
  2. Implements verification to ensure the resulting CSV makes sense, and exit with an appropriate return code
  3. Fixes a bug with chameleon where a recent setuptools removed a module that it depended on. If the pyperformance version is affected (<=1.11.0), then we setup the venv separately and install the newest setuptools version that still has the module (v81.0.0).

Before/After Comparison

Before

Test tools would end up at the pwd of the calling shell.

CSVs would not be verified, resulting in possible garbage output

Chameleon would not run

After

Test tools will reside at $HOME/test_tools

CSVs are verified, ensuring that garbage does not land into the output

Chameleon runs again.

Clerical Stuff

Closes #65 and #63

Relates to JIRA: RPOPC-835
Relates to JIRA: RPOPC-823

Supporting files

pyperf.log
pyperf_out_2026.02.17-16.25.31.csv


PR Type

Enhancement, Bug fix


Description

  • Standardize test_tools location to $HOME/test_tools environment variable

  • Add CSV verification with schema validation and proper exit codes

  • Fix chameleon benchmark by capping setuptools version for pyperformance <=1.11.0

  • Refactor output handling to use exec redirection instead of recursive calls


Diagram Walkthrough

flowchart LR
  A["pyperf_run script"] -->|uses TOOLS_BIN| B["$HOME/test_tools"]
  A -->|calls setup_pyperformance| C["setuptools v81.0.0 fix"]
  A -->|generates| D["pyperf.json"]
  D -->|verified by| E["verify_results"]
  E -->|returns| F["exit code rtc"]
  A -->|exits with| F
Loading

File Walkthrough

Relevant files
Enhancement, bug fix
pyperf_run
Standardize paths, add verification, fix setuptools           

pyperf/pyperf_run

  • Replace hardcoded test_tools references with $TOOLS_BIN environment
    variable pointing to $HOME/test_tools
  • Add setup_pyperformance() function to cap setuptools at v81.0.0 for
    pyperformance versions <=1.11.0
  • Implement CSV verification using csv_to_json and verify_results tools
    with schema validation
  • Refactor output redirection from recursive call with tee to exec-based
    approach
  • Propagate verification return code through rtc variable to final exit
+36/-22 
Enhancement
pyperf_schema.py
Add pydantic schema for results validation                             

pyperf_schema.py

  • Create new schema file defining Pydantic model for pyperformance
    results validation
  • Define TestNames enum with all supported benchmark names
  • Define DurationUnits enum for time measurement units
  • Define PyperfResults model with required fields: Test, Avg, Unit,
    Start_Date, End_Date
  • Add validation constraints: Avg must be positive and not infinite/NaN
+124/-0 

Caps the setuptools version on pyperformance versions <=1.11.0.  This is
due to setuptools removing `pkg_resources` which breaks the chameleon
benchmark in that pyperformance range.
Exit based off verification return code, if it's a no-op it would return
0.  This also moves the writing of /tmp/pyperf.out to an `exec` instead
of trying to figure it out via tee/shell redirection.

This removes a recursive call to the wrapper.
@github-actions

Copy link
Copy Markdown

This relates to RPOPC-835

@qodo-code-review

qodo-code-review Bot commented Feb 12, 2026

Copy link
Copy Markdown
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Supply-chain code execution

Description: The script git clones external test_tools into $HOME/test_tools (line 129-133) and then
executes/sources tools from it (e.g., source $TOOLS_BIN/general_setup at line 287 and
$TOOLS_BIN/* invocations at lines 268, 419-420), and also installs setuptools==81.0.0 from
PyPI (line 93) without integrity pinning (hash/signature), creating a potential
supply-chain path to arbitrary code execution if the repo, transport, or package source is
compromised. pyperf_run [129-421]

Referred Code
	if [ ! -d "$TOOLS_BIN" ]; then
		git clone $tools_git "$TOOLS_BIN"
		if [ $? -ne 0 ]; then
			exit_out "pulling git $tools_git failed." 1
		fi
	fi

	if [ $show_usage -eq 1 ]; then
		usage $1
	fi
}

generate_csv_file()
{
	instance=0
	float=0
	ivalue=0
	fvalue=0.0
	test_name=""
	unit=""
	reduce=0


 ... (clipped 272 lines)
Ticket Compliance
🟡
🎫 #65
🟢 Fix the chameleon subbenchmark failure caused by missing pkg_resources (setuptools
change), so the benchmark runs again across affected environments.
Ensure compatibility for affected pyperformance versions (workaround noted: install later
pyperformance).
Confirm chameleon now runs successfully in the listed environments (Ubuntu 24.04, UBI9,
AL2023, RHEL 9/10) and that the setuptools cap does not break other benchmarks.
🟡
🎫 #63
🟢 Use pyantics-based verification to validate run results and ensure the resulting
CSV/converted output “makes sense”.
Handle test_tools properly (implied: consistent/standardized tool location and
invocation).
Confirm csv_to_json + verify_results correctly validate real produced outputs and that
exit codes propagate as intended in CI/automation.
Codebase Duplication Compliance
🟢
No codebase code duplication found New Components Detected:
- TestNames
- PyperfResults
Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Unchecked command failures: Several newly added external commands (e.g., gather_data, csv_to_json, and pip install)
are invoked without validating prerequisites or handling failures with actionable context
beyond propagating $?.

Referred Code
$TOOLS_BIN/gather_data ${curdir}

exec &> >(tee /tmp/pyperf.out)

if [ -d "workloads" ]; then
	#
	# If running from zathras, workloads will be symlinked to
	# to /mnt.  Which is done due to azure having a very small
	# user space.
	#
	start_dir=`pwd`
	cd workloads
	for file in `ls ${start_dir}`; do
		if [[ ! -f $file ]] && [[ ! -d $file ]]; then
			ln -s $start_dir/$file .
		fi
	done
fi

source $TOOLS_BIN/general_setup "$@"



 ... (clipped 135 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit context: New actions like cloning tools, creating virtualenvs, and installing packages are executed
without structured audit logging that includes timestamp/user/action/outcome, making later
reconstruction difficult.

Referred Code
setup_pyperformance()
{
	major=$(echo $PYPERF_VERSION | cut -d. -f1)
	minor=$(echo $PYPERF_VERSION | cut -d. -f2)

	# Revert to setuptools v81.0.0 due to `pkg_resources` removal at v82.0.0
	# https://setuptools.pypa.io/en/stable/history.html#deprecations-and-removals
	# Skipping using pkg_tool to ensure that all relevant venv PATHs are honored
	if [[ "$major" -le 1 ]] && [[ "$minor" -le 11 ]]; then
		echo "Detected <=1.11.0, applying setuptools fix"
		$python_exec -m pyperformance venv create
		venv_path=$($python_exec -m pyperformance venv show | cut -d: -f2 | awk '{ print $1 }')
		source $venv_path/bin/activate
		python3 -m pip install setuptools==81.0.0 #Operating within venv, so $python_exec is not needed
		deactivate
	fi

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Potential PII in logs: The new flow captures full stdout/stderr to /tmp/pyperf.out and later persists it via
save_results, which may include usernames (--user $to_user) or other environment details
depending on tool output.

Referred Code
exec &> >(tee /tmp/pyperf.out)

if [ -d "workloads" ]; then
	#
	# If running from zathras, workloads will be symlinked to
	# to /mnt.  Which is done due to azure having a very small
	# user space.
	#
	start_dir=`pwd`
	cd workloads
	for file in `ls ${start_dir}`; do
		if [[ ! -f $file ]] && [[ ! -d $file ]]; then
			ln -s $start_dir/$file .
		fi
	done
fi

source $TOOLS_BIN/general_setup "$@"

ARGUMENT_LIST=(
	"pyperf_version"


 ... (clipped 148 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unvalidated external sourcing: The script executes code via source $TOOLS_BIN/general_setup "$@" from a cloned
tools directory without validating the source, integrity, or ensuring tools_git/TOOLS_BIN
cannot be influenced to run unintended code.

Referred Code
	if [ ! -d "$TOOLS_BIN" ]; then
		git clone $tools_git "$TOOLS_BIN"
		if [ $? -ne 0 ]; then
			exit_out "pulling git $tools_git failed." 1
		fi
	fi

	if [ $show_usage -eq 1 ]; then
		usage $1
	fi
}

generate_csv_file()
{
	instance=0
	float=0
	ivalue=0
	fvalue=0.0
	test_name=""
	unit=""
	reduce=0


 ... (clipped 138 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review

qodo-code-review Bot commented Feb 12, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to f676b26

CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Abort when pip install fails

Add an error check after the pip install command to ensure it was successful and
exit with an informative message if it failed.

pyperf/pyperf_run [100]

 		"$venv_path/bin/python3" -m pip install --upgrade setuptools==81.0.0 # Execute venv pip instead
+		if [ $? -ne 0 ]; then
+			exit_out "Error: Failed to install pinned setuptools into pyperformance venv" 1
+		fi
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that the script does not check the exit code of the pip install command, which could lead to running benchmarks in a broken environment; adding an explicit error check is a significant improvement for robustness.

Medium
Validate venv path robustly

Sanitize the venv_path variable by trimming whitespace and taking the first
line, and add a check to ensure it is a valid directory before proceeding.

pyperf/pyperf_run [88-90]

-	if [[ -z "$venv_path" ]]; then
-		exit_out "Error: Could not determine pyperformance venv path, exiting" 1
+	venv_path="$(printf '%s' "$venv_path" | head -n1 | xargs)"
+	if [[ -z "$venv_path" ]] || [[ ! -d "$venv_path" ]]; then
+		exit_out "Error: Could not determine valid pyperformance venv path, exiting" 1
 	fi
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential robustness issue where the venv_path variable might contain unexpected whitespace or multiple lines, and it proposes a solid fix to sanitize the path and validate it's a directory, improving script reliability.

Low
Possible issue
Ensure tools exist before use

Add a check to ensure test_tools are installed before they are used, and quote
path variables to prevent word splitting issues.

pyperf/pyperf_run [274-293]

-$TOOLS_BIN/gather_data ${curdir}
+if [[ ! -x "$TOOLS_BIN/gather_data" ]] || [[ ! -f "$TOOLS_BIN/general_setup" ]]; then
+	install_tools
+fi
+
+"$TOOLS_BIN/gather_data" "${curdir}"
 
 exec &> >(tee /tmp/pyperf.out)
 ...
-source $TOOLS_BIN/general_setup "$@"
+source "$TOOLS_BIN/general_setup" "$@"

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the script may fail if run on a fresh machine, as it attempts to use tools from $TOOLS_BIN before ensuring they are installed, thus improving the script's robustness.

Medium
Use string-based enums

Inherit from str in the TestNames and DurationUnits enums to ensure they are
handled as strings by Pydantic for stricter validation and serialization.

pyperf_schema.py [5-116]

-class TestNames(Enum):
+class TestNames(str, Enum):
     two_to_three = "2to3"
     async_generators = "async_generators"
     async_tree_none = "async_tree_none"
     ...
-class DurationUnits(Enum):
+class DurationUnits(str, Enum):
     second = "sec"
     millisecond = "ms"
     microsecond = "us"
     nanosecond = "ns"

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that using str, Enum is a best practice with Pydantic for string-based enumerations, which ensures stricter validation and consistent serialization behavior.

Low
  • More

Previous suggestions

✅ Suggestions up to commit ef093e4
CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Fix setuptools installation command
Suggestion Impact:Replaced the incorrect `python3 install setuptools==81.0.0` invocation with `python3 -m pip install --upgrade setuptools==81.0.0` inside the venv.

code diff:

-		"$venv_path/bin/python3" install setuptools==81.0.0 # Execute venv pip instead
+		"$venv_path/bin/python3" -m pip install --upgrade setuptools==81.0.0 # Execute venv pip instead

Fix the command for installing setuptools by using python -m pip install instead
of calling python install.

pyperf/pyperf_run [97]

-"$venv_path/bin/python3" install setuptools==81.0.0 # Execute venv pip instead
+"$venv_path/bin/python3" -m pip install --upgrade setuptools==81.0.0 # Execute venv pip instead

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug where the command to install setuptools is syntactically incorrect and would fail, and provides the correct command to fix it.

High
Validate virtualenv path before use
Suggestion Impact:The commit adds an explicit check for an empty venv_path and exits with an error before attempting to use $venv_path/bin/python3.

code diff:

+	if [[ -z "$venv_path" ]]; then
+		exit_out "Error: Could not determine pyperformance venv path, exiting" 1
+	fi
 	if [[ ! -x "$venv_path/bin/python3" ]]; then
 		exit_out "Error: Could not find interpreter at $venv_path/bin/python3, exiting" 1
 	fi

Add a check to ensure the venv_path variable is not empty before using it to
prevent accidentally using the system's Python interpreter.

pyperf/pyperf_run [88-90]

+if [[ -z "$venv_path" ]]; then
+	exit_out "Error: Could not determine pyperformance venv path, exiting" 1
+fi
 if [[ ! -x "$venv_path/bin/python3" ]]; then
 	exit_out "Error: Could not find interpreter at $venv_path/bin/python3, exiting" 1
 fi

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that an empty venv_path could cause the script to fall back to the system's Python interpreter, and proposes adding a check to prevent this potential bug.

Medium
Possible issue
Propagate tool failures correctly

Add an explicit exit code check after the csv_to_json command to ensure any
failures are caught and prevent the script from continuing with potentially
invalid data.

pyperf/pyperf_run [422-424]

-$TOOLS_BIN/csv_to_json $to_json_flags --csv_file ${pyresults}.csv --output_file pyperf.json
-$TOOLS_BIN/verify_results $to_verify_flags --file pyperf.json --schema_file $to_script_dir/../pyperf_schema.py --class_name PyperfResults
+"$TOOLS_BIN/csv_to_json" $to_json_flags --csv_file "${pyresults}.csv" --output_file pyperf.json
+if [ $? -ne 0 ]; then
+	exit_out "Failed: csv_to_json" 1
+fi
+
+"$TOOLS_BIN/verify_results" $to_verify_flags --file pyperf.json --schema_file "$to_script_dir/../pyperf_schema.py" --class_name PyperfResults
 rtc=$?
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug where a failure in csv_to_json is ignored, which could lead to the script reporting success based on stale data. This improves the script's robustness and correctness.

Medium
✅ Suggestions up to commit 7c193c8
CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Fix benchmark name typo
Suggestion Impact:Updated the TestNames enum entry for dask from "dasks" to "dask", fixing the typo as suggested.

code diff:

@@ -32,7 +32,7 @@
     coroutines = "coroutines"
     coverage = "coverage"
     crypto_pyaes = "crypto_pyaes"
-    dask = "dasks"
+    dask = "dask"

Correct the typo in the TestNames enum from "dasks" to "dask" to ensure correct
schema validation for the dask benchmark.

pyperf_schema.py [35]

-dask = "dasks"
+dask = "dask"

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a clear typo ("dasks" instead of "dask") in the schema definition that would cause validation to fail for valid dask benchmark results, which is a critical bug in the new verification logic.

High
Use venv Python for pip
Suggestion Impact:The commit changed the installation command from using the venv's pip3 to invoking the venv's python3 (and added a check that the venv python3 exists), aligning with the intent to rely on the venv interpreter, though it did not implement the recommended `-m pip` usage.

code diff:

+	if [[ ! -x "$venv_path/bin/python3" ]]; then
+		exit_out "Error: Could not find interpreter at $venv_path/bin/python3, exiting" 1
+	fi
+
 	# Revert to setuptools v81.0.0 due to `pkg_resources` removal at v82.0.0
 	# https://setuptools.pypa.io/en/stable/history.html#deprecations-and-removals
 	# Skipping using pkg_tool to ensure that all relevant venv PATHs are honored
 	if [[ "$major" -le 1 ]] && [[ "$minor" -le 11 ]]; then
 		echo "Detected <=1.11.0, applying setuptools fix"
-		$venv_path/bin/pip3 install setuptools==81.0.0 # Execute venv pip instead
+		"$venv_path/bin/python3" install setuptools==81.0.0 # Execute venv pip instead
 	fi

Use the virtual environment's Python interpreter to run pip (i.e., python -m
pip) to ensure the correct pip instance is used for package installation.

pyperf/pyperf_run [93]

-$venv_path/bin/pip3 install setuptools==81.0.0 # Execute venv pip instead
+"$venv_path/bin/python" -m pip install setuptools==81.0.0 # Execute venv pip instead

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: This suggestion correctly recommends using python -m pip which is the canonical and more robust way to ensure the correct pip from the virtual environment is used, preventing potential installation issues.

Medium
Validate virtualenv path extraction

Improve the robustness of virtual environment path extraction by using a more
reliable parsing method and adding a validation check to ensure the path is
valid.

pyperf/pyperf_run [85-86]

 $python_exec -m pyperformance venv create
-venv_path=$($python_exec -m pyperformance venv show | sed -e "s/Virtual environment path: //g" -e "s/ (already created)//" )
+venv_path="$($python_exec -m pyperformance venv show | awk -F': ' '/Virtual environment path:/ {print $2}' | sed -e 's/ (already created)//' -e 's/[[:space:]]*$//')"
+if [[ -z "$venv_path" || ! -d "$venv_path" ]]; then
+	exit_out "Error: Unable to determine pyperformance venv path" 1
+fi
Suggestion importance[1-10]: 6

__

Why: The suggestion improves the script's robustness by making the path extraction more reliable and adding an explicit check to ensure the virtual environment path was correctly identified, preventing potential downstream failures.

Low
Possible issue
Quote tool and directory paths

Quote usages of $TOOLS_BIN and $curdir to prevent script failures from
word-splitting or glob expansion when these paths contain whitespace.

pyperf/pyperf_run [267-286]

-$TOOLS_BIN/gather_data ${curdir}
+"$TOOLS_BIN/gather_data" "${curdir}"
 
 exec &> >(tee /tmp/pyperf.out)
 ...
-source $TOOLS_BIN/general_setup "$@"
+source "$TOOLS_BIN/general_setup" "$@"
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that unquoted variables $TOOLS_BIN and $curdir can lead to script failures if their paths contain spaces, and quoting them is a crucial robustness improvement.

Medium
Quote venv path executions
Suggestion Impact:The change added quoting around the venv_path-based executable path (now "$venv_path/bin/python3" ...) which addresses the word-splitting concern, though it did not implement the exact suggested pip3 quoting.

code diff:

-		$venv_path/bin/pip3 install setuptools==81.0.0 # Execute venv pip instead
+		"$venv_path/bin/python3" install setuptools==81.0.0 # Execute venv pip instead
 	fi

Quote the $venv_path variable when executing pip3 to prevent word-splitting
issues if the path contains spaces.

pyperf/pyperf_run [85-94]

 $python_exec -m pyperformance venv create
 venv_path=$($python_exec -m pyperformance venv show | sed -e "s/Virtual environment path: //g" -e "s/ (already created)//" )
 
 # Revert to setuptools v81.0.0 due to `pkg_resources` removal at v82.0.0
 # https://setuptools.pypa.io/en/stable/history.html#deprecations-and-removals
 # Skipping using pkg_tool to ensure that all relevant venv PATHs are honored
 if [[ "$major" -le 1 ]] && [[ "$minor" -le 11 ]]; then
 	echo "Detected <=1.11.0, applying setuptools fix"
-	$venv_path/bin/pip3 install setuptools==81.0.0 # Execute venv pip instead
+	"$venv_path/bin/pip3" install setuptools==81.0.0 # Execute venv pip instead
 fi

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the unquoted $venv_path could cause script failure if the path contains spaces, and proposes a valid fix to improve robustness.

Low
✅ Suggestions up to commit a456bd3
CategorySuggestion                                                                                                                                    Impact
High-level
Consider a more robust dependency management

Instead of manually pinning setuptools inside a shell script for the 'chameleon'
benchmark fix, use a dependency constraints file. This makes the dependency
management more declarative and easier to maintain.

Examples:

pyperf/pyperf_run [80-96]
setup_pyperformance()
{
	major=$(echo $PYPERF_VERSION | cut -d. -f1)
	minor=$(echo $PYPERF_VERSION | cut -d. -f2)

	# Revert to setuptools v81.0.0 due to `pkg_resources` removal at v82.0.0
	# https://setuptools.pypa.io/en/stable/history.html#deprecations-and-removals
	# Skipping using pkg_tool to ensure that all relevant venv PATHs are honored
	if [[ "$major" -le 1 ]] && [[ "$minor" -le 11 ]]; then
		echo "Detected <=1.11.0, applying setuptools fix"

 ... (clipped 7 lines)

Solution Walkthrough:

Before:

# pyperf/pyperf_run

setup_pyperformance() {
    if [[ "$PYPERF_VERSION" <= "1.11.0" ]]; then
        echo "Applying setuptools fix"
        $python_exec -m pyperformance venv create
        venv_path=$($python_exec -m pyperformance venv show ...)
        source $venv_path/bin/activate
        python3 -m pip install setuptools==81.0.0
        deactivate
    fi
}

...
setup_pyperformance
$python_exec -m pyperformance run ...

After:

# pyperf/pyperf_run

setup_pyperformance() {
    if [[ "$PYPERF_VERSION" <= "1.11.0" ]]; then
        # pyperformance would need to be invoked in a way
        # that respects a constraints file.
        # This is a conceptual change.
        PYPERF_PIP_INSTALL_ARGS="--constraint /path/to/constraints.txt"
    fi
}

# /path/to/constraints.txt
setuptools==81.0.0

...
setup_pyperformance
# The pyperformance tool would internally use the constraints
$python_exec -m pyperformance run ...
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the shell-based manipulation of the virtual environment in setup_pyperformance is a brittle solution and proposes a more robust, standard approach using a dependency constraints file, which would significantly improve maintainability.

Medium
Possible issue
Improve venv path parsing robustness
Suggestion Impact:The commit replaced the previous `cut ... | awk ...` parsing of `pyperformance venv show` output with a `sed`-based extraction to obtain `venv_path`, improving robustness for paths with spaces (though with a slightly different sed pattern than suggested).

code diff:

+	$python_exec -m pyperformance venv create
+	venv_path=$($python_exec -m pyperformance venv show | sed -e "s/Virtual environment path: //g" -e "s/ (already created)//" )
+
 	# Revert to setuptools v81.0.0 due to `pkg_resources` removal at v82.0.0
 	# https://setuptools.pypa.io/en/stable/history.html#deprecations-and-removals
 	# Skipping using pkg_tool to ensure that all relevant venv PATHs are honored
 	if [[ "$major" -le 1 ]] && [[ "$minor" -le 11 ]]; then
 		echo "Detected <=1.11.0, applying setuptools fix"
-		$python_exec -m pyperformance venv create
-		venv_path=$($python_exec -m pyperformance venv show | cut -d: -f2 | awk '{ print $1 }')
-		source $venv_path/bin/activate
-		python3 -m pip install setuptools==81.0.0 #Operating within venv, so $python_exec is not needed
-		deactivate
+		$venv_path/bin/pip3 install setuptools==81.0.0 # Execute venv pip instead

Replace the cut and awk pipeline with a sed command to robustly parse the
virtual environment path, ensuring it correctly handles paths that contain
spaces.

pyperf/pyperf_run [91]

-venv_path=$($python_exec -m pyperformance venv show | cut -d: -f2 | awk '{ print $1 }')
+venv_path=$($python_exec -m pyperformance venv show | sed 's/Virtual environment: //')

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential bug where paths with spaces would be parsed incorrectly and provides a more robust sed-based solution, improving script reliability.

Medium
General
Use direct path for pip install
Suggestion Impact:The commit removed `source .../activate` and `deactivate` and switched to using the venv’s own executable directly (`$venv_path/bin/pip3 install ...`) to install setuptools.

code diff:

+	$python_exec -m pyperformance venv create
+	venv_path=$($python_exec -m pyperformance venv show | sed -e "s/Virtual environment path: //g" -e "s/ (already created)//" )
+
 	# Revert to setuptools v81.0.0 due to `pkg_resources` removal at v82.0.0
 	# https://setuptools.pypa.io/en/stable/history.html#deprecations-and-removals
 	# Skipping using pkg_tool to ensure that all relevant venv PATHs are honored
 	if [[ "$major" -le 1 ]] && [[ "$minor" -le 11 ]]; then
 		echo "Detected <=1.11.0, applying setuptools fix"
-		$python_exec -m pyperformance venv create
-		venv_path=$($python_exec -m pyperformance venv show | cut -d: -f2 | awk '{ print $1 }')
-		source $venv_path/bin/activate
-		python3 -m pip install setuptools==81.0.0 #Operating within venv, so $python_exec is not needed
-		deactivate
+		$venv_path/bin/pip3 install setuptools==81.0.0 # Execute venv pip instead
 	fi

Instead of sourcing the virtual environment's activate script, call the python3
executable directly from the venv's bin directory to install setuptools.

pyperf/pyperf_run [92-94]

-source $venv_path/bin/activate
-python3 -m pip install setuptools==81.0.0 #Operating within venv, so $python_exec is not needed
-deactivate
+$venv_path/bin/python3 -m pip install setuptools==81.0.0

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that sourcing activate in a script is not ideal and proposes a more robust, self-contained method by calling the venv's python executable directly.

Medium
Quote commands and paths

Add quotes around the command path and variable expansions in the csv_to_json
call to prevent issues with word splitting or globbing.

pyperf/pyperf_run [419]

-$TOOLS_BIN/csv_to_json $to_json_flags --csv_file ${pyresults}.csv --output_file pyperf.json
+"$TOOLS_BIN/csv_to_json" $to_json_flags --csv_file "${pyresults}.csv" --output_file "pyperf.json"
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly recommends quoting variables and paths to prevent word splitting and globbing, which is a good practice for improving script robustness.

Low

Comment thread pyperf/pyperf_run Outdated
Remove venv sourcing and use the pip binary directly.  Use sed for
cleaning up the path
Comment thread pyperf/pyperf_run Outdated
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Comment thread pyperf/pyperf_run
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
@kdvalin kdvalin requested a review from a team February 12, 2026 21:51

@dvalinrh dvalinrh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@dvalinrh dvalinrh added the group_review_lgtm Indicates approval after a group review meeting label Feb 17, 2026
@kdvalin kdvalin merged commit c7834e2 into main Feb 17, 2026
4 checks passed
@kdvalin kdvalin deleted the test_tools branch February 17, 2026 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group_review_lgtm Indicates approval after a group review meeting Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chameleon Subbenchmark Fails

2 participants