Performance tests #597

renatav · 2025-03-04T20:33:49Z

Description (e.g. "Related to ...", etc.)

Integrate benchmark tests into CI:

We now run benchmark tests after every push. Made it possible to use a JSON configuration file to set different performance thresholds when necessary. This is useful especially when new features are added that might slow down the system because they do more work. The default value is set to 10.
Every run compares the current results with the master branch's performance. This meant that some custom download code was necessary, as we want to download from a different branch.
This custom JSON file also helps us keep track of why and when we needed to change these thresholds
When changes are merged into the master branch, we generate new benchmark results and upload the corresponding artifacts.
Add scheduled builds set to run every 90 days to prevent our artifacts from being automatically deleted due to GitHub's cleanup policies.

There are probably many ways in which we can compare this whole logic, but I think this is a good starting point and we'll have some basic performance tests. I think that we can close the issue and open new ones if we think of any improvements

Closes #571

…nce-tests

n-dusan

Looks awesome. Left some minor suggestions

n-dusan · 2025-03-07T17:12:37Z

docs/developers/performance-regression-tests.md

+```json
+{
+  "feature/cool-feature": 20,
+  "bugfix/urgent-fix": 15
+}


suggestion: A sentence or two explaining what the threshold number means would be useful

I removed this file and merged everything with the testing docs that I asked Cal to write

n-dusan · 2025-03-07T17:20:58Z

benchmark_thresholds.json

@@ -0,0 +1,3 @@
+{
+    "renatav/performance-tests": 15


question: can we bring this threshold to be lower? Is 15 times slower before it errors out too much?

That's 15%. The default is 10%, this is just an example of how to overwrite the default value

n-dusan · 2025-03-07T17:25:35Z

.github/workflows/ci.yml

+
+    env:
+      ARTIFACT_NAME: benchmark-results${{ matrix.python-version }}
+      BRANCH_NAME: 'master' # needs to be changed to master before merging


Suggested change

BRANCH_NAME: 'master' # needs to be changed to master before merging

BRANCH_NAME: 'master'

n-dusan · 2025-03-07T17:28:15Z

.github/workflows/ci.yml

+          script: |
+            const artifacts = await github.rest.actions.listArtifactsForRepo({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+            });
+            const filteredArtifacts = artifacts.data.artifacts.filter(artifact => artifact.name === process.env.ARTIFACT_NAME);
+            let latestArtifact = null;
+
+            for (const artifact of filteredArtifacts) {
+              const run = await github.rest.actions.getWorkflowRun({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                run_id: artifact.workflow_run.id,
+              });
+
+              if (run.data.head_branch === process.env.BRANCH_NAME) {
+                if (!latestArtifact || new Date(artifact.created_at) > new Date(latestArtifact.created_at)) {
+                  latestArtifact = artifact;
+                }
+              }
+            }
+
+            if (latestArtifact) {
+              console.log(`Found latest artifact: ${latestArtifact.id}`);
+              core.setOutput('artifact_id', latestArtifact.id.toString());
+            } else {
+              console.log('No matching artifacts found.');
+              core.setOutput('artifact_id', '');
+            }


question: could we check in this javascript (and other) code somewhere in the .github (e.g. .github/scripts) directory and have GitHub actions require the file? The benefit is that we have syntax highlighting and would be easier to debug [1]?

[1] - https://github.com/orgs/community/discussions/26248

…load code to a separate js file

n-dusan · 2025-03-13T12:14:07Z

.github/workflows/ci.yml

-      - name: Run pre-commit and test with pytest
+      # - name: Run pre-commit and test with pytest
+      #   run: |
+      #     pre-commit run --all-files
+      #     pytest taf/tests
+


question: is running pytest taf/tests/ and pre-commit supposed to be commented out?

n-dusan · 2025-03-13T12:26:36Z

.github/workflows/ci.yml

+      # Use a step to extract the branch name from github.ref
+      - name: Extract Branch Name
+        id: extract_branch
+        run: echo "::set-output name=branch::$(echo ${GITHUB_REF#refs/heads/})"


suggestion: set-output is getting deprecated. Could we please use the new approach [1]? The deprecation notice is here.

[1] - https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

n-dusan · 2025-03-13T12:27:47Z

docs/developers/benchmark-testing.md

+## Artifact Handling
+
+- **Uploading Artifacts**: After benchmarks are run, the resulting `0001_output.json` file is uploaded as an artifact to GitHub Actions, allowing it to be used in future benchmark comparisons.


suggestion: adding link to the already uploaded artifacts (from some older GitHub Actions build as an example) would be very useful here

…d up development

CalShucha and others added 11 commits February 4, 2025 14:13

Copy over changes to ci.yml

8d46376

Temporarily disable artifact downloading for test

153067a

Disable comparison for testing

03ac0f6

Re-enable everything

9995440

Format with black

4fce15c

Re-disable comparison for testing

86cbb34

Re-re-enable everything

04eaa06

Disable parts of workflow for testing

4fa2d3e

Re-enable parts of workflow for testing

c90d312

Merge branch 'CalShucha/automated-benchmarking' into renatav/performa…

2a6b34f

…nce-tests

devops: download artifacts without using an external, unofficial script

846c842

renatav force-pushed the renatav/performance-tests branch from 14dbb31 to 846c842 Compare March 5, 2025 01:01

devops: implement a way of redefining default threshold

d7af1ff

renatav force-pushed the renatav/performance-tests branch from 9d2ccb6 to d7af1ff Compare March 5, 2025 07:00

renatav added 2 commits March 5, 2025 02:16

docs: performance testing docs

c364b94

devops: regenerate artifacts every 90 days

75bbfeb

renatav force-pushed the renatav/performance-tests branch from 1c8604d to 1845b19 Compare March 5, 2025 12:19

fix, chore: fix the scheduled build script, cleanup

a648792

renatav force-pushed the renatav/performance-tests branch from 239cf57 to a648792 Compare March 5, 2025 12:38

renatav marked this pull request as ready for review March 5, 2025 15:19

renatav requested review from sale3 and n-dusan March 5, 2025 15:19

renatav self-assigned this Mar 5, 2025

n-dusan reviewed Mar 7, 2025

View reviewed changes

devops, docs: move all docs to existing document, move fetch and down…

a7f863f

…load code to a separate js file

renatav force-pushed the renatav/performance-tests branch from a1fd500 to a7f863f Compare March 12, 2025 05:29

renatav requested a review from n-dusan March 12, 2025 05:32

renatav force-pushed the renatav/performance-tests branch from 19a4aca to a7f863f Compare March 12, 2025 06:27

Merge branch 'master' into renatav/performance-tests

622adad

n-dusan reviewed Mar 13, 2025

View reviewed changes

chore: uncomment running tests step, previously commented out to spee…

abb7d3a

…d up development

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance tests #597

Performance tests #597

renatav commented Mar 4, 2025 •

edited

Loading

n-dusan left a comment

n-dusan Mar 7, 2025

renatav Mar 12, 2025

n-dusan Mar 7, 2025

renatav Mar 12, 2025

n-dusan Mar 7, 2025

n-dusan Mar 7, 2025

n-dusan Mar 13, 2025

n-dusan Mar 13, 2025

n-dusan Mar 13, 2025

	BRANCH_NAME: 'master' # needs to be changed to master before merging
	BRANCH_NAME: 'master'

		## Artifact Handling

		- Uploading Artifacts: After benchmarks are run, the resulting `0001_output.json` file is uploaded as an artifact to GitHub Actions, allowing it to be used in future benchmark comparisons.

Performance tests #597

Are you sure you want to change the base?

Performance tests #597

Conversation

renatav commented Mar 4, 2025 • edited Loading

Description (e.g. "Related to ...", etc.)

n-dusan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatav commented Mar 4, 2025 •

edited

Loading