Prechecks for asv #2107

grusev · 2025-01-08T08:15:55Z

Reference Issues/PRs

What does this implement or fix?

There are couple of checks that need to be done before making sure asv benchmark tests are ok to be merged:

Check that code of tests is ok and can run

asv check --python=same

Checks that versions of benchmark tests are up to date in asv,conf.json

asv run --bench just-discover --python=same

this PR will prepare a script which can be run by anyone before making PR for review as well as github action that will execute it to confirm all is ok

A new python utility is added to do the required checks. Usage:

python python/utils/asv_checks.py

This tool now can be used in a github action to do automatic check

Successfull check of Job: https://github.com/man-group/ArcticDB/actions/runs/12713349006/job/35441035657

A job failed because benchmark.json not up to date: https://github.com/man-group/ArcticDB/actions/runs/12711972129/job/35436505808

NOTE:

The most efficien way to do ASV check is on your machine wither by executing the both above mentioned commands or the script python python/utils/asv_checks.py (better)
On github currently the workflow is not efficient as it does approx 35min build and the actual check is just 5 secs afterwards. Why it is not efficient?

it needs build. and as there is no build repository of previous jobs like build job the build has to be repeated here
this cannot be combines with ASV tests. The asv tests will do specific build for their own purpose and run the tests. The build AND the test are one atomic process thus we cannot plugin there and even if we plugin the inefficiency remains in terms of time

Any other comments?

Checklist

Checklist for code changes...

Have you updated the relevant docstrings, documentation and copyright notice?
Is this contribution tested against all ArcticDB's features?
Do all exceptions introduced raise appropriate error messages?
Are API changes highlighted in the PR description?
Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

IvoDD

Left some minor comments. Apart from that I see the point in not running the asv checks on every build since it takes 35 minutes.

However couldn't we run it in parallel with the asv benchmarks and make it block merging? This shouldn't increase time to run the tests, since benchmarks take way longer. However I'm not sure if it's worth the extra CPU (will leave up to you or maybe @G-D-Petrov who knows more about our CI pipelines)

IvoDD · 2025-01-14T14:33:39Z

python/utils/asv_checks.py

+        if not ok_errors_list is None:
+            for ok_error in ok_errors_list:
+                err_output.replace(ok_error, "")
+                err_output = re.sub(r'\s+', '', err_output)


This removes all whitespace, correct? Wouldn't this make the case where we have some leftover errors harder to read?

E.g. if we have 2 errors:

Expected error which should be removed Unexpected error which should persist and be displayed

In the error message bellow the unexpected error will have removed whitespaces.

Also this could be a problem if the ok_error_list contains more than one error and one of them has a whitespace.
E.g. ok_errors_list = ["Expected 1", "Expected 2"]
And we have the logs which contain just the two expected errors:

Expected 1 Expected 2

After the first iteration the error would become:

Expected2

Which wouldn't match the second expected error and we would end up with an "Unkown error" even though we only have 2 exoected errors.

Probably doesn't matter too much as you only ever use a single expected error. Still it would be better to instead of remove all whitespace after each error to check err_output.strip()=="" in the end.

IvoDD · 2025-01-14T15:03:27Z

python/utils/asv_checks.py

+
+def get_project_root():
+    file_location = os.path.abspath(__file__)
+    return file_location.split("/python/")[0]


This would theoretically fail if our working directory has a python lib in the path. E.g. imagine you install arcticdb in:
/home/grusev/code/python/ArcticDB/python/python_code. We could do something like this

changed to increase confidence :-)

IvoDD · 2025-01-14T15:07:38Z

python/utils/asv_checks.py

+    sys.path.insert(0,f"{path}/python")
+    sys.path.insert(0,f"{path}/python/tests")
+
+    bencmark_config = f"{path}/python/.asv/results/benchmarks.json"


Typo: bencHmark

IvoDD · 2025-01-14T15:12:08Z

python/utils/asv_checks.py

+            they would need to be in order for completion of current PR""")
+    print("_" * 80)
+
+    print("\n\nCheck 1: Executing check for python cod of asv tests")


G-D-Petrov · 2025-01-15T08:55:22Z

.github/workflows/asv_checks.yml

+        default: true
+
+jobs:
+  run-asv-check-script:


I agree with @IvoDD,we don't need this to be a separate flow, just a separate job in the analysis_flow.
This way it will be easier for people, as they would have to check only 1 flow.
Let's move this job to the analysis_flow.yml, similarly to the code_coverage job there

agree makes lots of sence

poodlewars · 2025-01-15T09:28:32Z

.github/workflows/asv_checks.yml

@@ -0,0 +1,79 @@
+name: Run ASV Tests Check Python Script


I think if you call this "ASV Linting" or something it will be more obvious to people that it doesn't actually run the benchmarks

ok, but note that this does something additional as check of versions of benchmark tests ... see bellow

poodlewars · 2025-01-15T09:29:16Z

.github/workflows/asv_checks.yml

+      VCPKG_NUGET_USER: ${{secrets.VCPKG_NUGET_USER || github.repository_owner}}
+      VCPKG_NUGET_TOKEN: ${{secrets.VCPKG_NUGET_TOKEN || secrets.GITHUB_TOKEN}}
+      CMAKE_C_COMPILER_LAUNCHER: sccache
+      CMAKE_CXX_COMPILER_LAUNCHER: sccache


Don't get why we need these compiler settings given that the whole point is we don't need to build the wheel to run these linting checks

to do the checks arcticdb library needs to be installed ... And installing released arcticdb does not help either as in benchmarks module we use libs from tests package (tested already as initally we wanted to be part of asv main worflow action) ... thus I need to invoke "pip install -ve ." which would do builds hence I coppied all things that would be needed for that from another workflow

If there is a way to achieve that without doing full CPP build I am ok to try this

as discussed after trasitionning this as per GP's comment this is not relevant anymore

poodlewars · 2025-01-15T09:29:44Z

python/utils/asv_checks.py

+from typing import List
+
+
+def error(mes):


Use logging not print statements in all PRs please

will start using it primarily

poodlewars · 2025-01-15T09:30:02Z

python/utils/asv_checks.py

+        if error_code == 0:
+            print("ABOVE ERRORS DOES NOT AFFECT FINAL ERROR CODE = 0")
+
+    if not output is None:


if output is not None is more idiomatic (same applies elsewhere)

poodlewars · 2025-01-15T09:36:04Z

python/utils/asv_checks.py

+    if not err_output is None:
+        error(err_output)
+        if error_code == 0:
+            print("ABOVE ERRORS DOES NOT AFFECT FINAL ERROR CODE = 0")


"DO NOT" not "DOES NOT"

poodlewars · 2025-01-15T09:37:08Z

python/utils/asv_checks.py

+    orig_hash = compute_file_hash(benchmark_config)
+
+    print("_" * 80)
+    print("""IMPORTANT: The tool checks CURRENT versions of asv tests along with asv.conf.json")


I don't understand this

Changed to this:

print("""IMPORTANT: The tool checks CURRENT ACTUAL versions of asv benchmark tests along with the one in benchmarks.json file. That means that if there are files that are not submitted yet (tests and benchmark.json), they would need to be in order for completion of current PR. benchmarks.json is updated with a version number calculated as a hash of the python test method. Thus any change of this method triggers different version. Hence you would need to update json file also. It happens automatically if you run following commandline: > asv run --bench just-discover --python=same """)

poodlewars

Georgi Rusev added 21 commits January 8, 2025 10:11

initial work

b9d2a22

check in working state

f6fb5a9

add step in benchmarks workflow

629bad4

add to python path

8e85ce8

install arcticdb release

2bf9a15

new workflow

bf6c8e8

roll back benchmark workflow

ebcfd50

YAMML fixes

dea7e07

yaml fixes

612db9d

yaml fixes

12dae48

check trigger

650bec2

added build project action

9c0aafa

check trigger 2

76ea676

more speed

3d70224

lets try manually triggering

23d6277

proper manual trigger

97b83ce

do nothing

9b9bf60

check trigger 3

b57c7e3

env vars

f3af121

test

c771b0a

remove redundant line

f23a441

grusev force-pushed the asv_slow branch 2 times, most recently from b00e8e9 to f23a441 Compare January 9, 2025 16:01

fix

3553540

grusev force-pushed the asv_slow branch 2 times, most recently from ca077d4 to 3553540 Compare January 10, 2025 09:21

asv machine --yes

762e09a

grusev force-pushed the asv_slow branch from 59ed321 to 762e09a Compare January 10, 2025 15:30

Georgi Rusev and others added 2 commits January 10, 2025 18:28

fix versions in benhcmarks.json

9b68459

Merge branch 'master' into asv_slow

b7e6e48

grusev marked this pull request as ready for review January 10, 2025 18:10

grusev requested review from alexowens90, willdealtry and poodlewars as code owners January 10, 2025 18:10

IvoDD reviewed Jan 14, 2025

View reviewed changes

grusev and others added 2 commits January 14, 2025 18:11

Merge branch 'master' into asv_slow

7eb0fde

fixes for code review

110adac

G-D-Petrov requested changes Jan 15, 2025

View reviewed changes

poodlewars reviewed Jan 15, 2025

View reviewed changes

Georgi Rusev added 2 commits January 15, 2025 14:03

more fixes on comments

40c49e3

proper refs for checkout

235760e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prechecks for asv #2107

Prechecks for asv #2107

grusev commented Jan 8, 2025 •

edited

Loading

IvoDD left a comment

IvoDD Jan 14, 2025

IvoDD Jan 14, 2025

grusev Jan 14, 2025

IvoDD Jan 14, 2025

IvoDD Jan 14, 2025

G-D-Petrov Jan 15, 2025

grusev Jan 15, 2025

poodlewars Jan 15, 2025

grusev Jan 15, 2025

poodlewars Jan 15, 2025

grusev Jan 15, 2025 •

edited

Loading

grusev Jan 15, 2025

poodlewars Jan 15, 2025

grusev Jan 15, 2025

poodlewars Jan 15, 2025 •

edited

Loading

poodlewars Jan 15, 2025

poodlewars Jan 15, 2025

grusev Jan 15, 2025

poodlewars left a comment

Prechecks for asv #2107

Are you sure you want to change the base?

Prechecks for asv #2107

Conversation

grusev commented Jan 8, 2025 • edited Loading

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

IvoDD 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

Choose a reason for hiding this comment

grusev Jan 15, 2025 • edited Loading

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

poodlewars Jan 15, 2025 • edited Loading

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

poodlewars left a comment

Choose a reason for hiding this comment

grusev commented Jan 8, 2025 •

edited

Loading

grusev Jan 15, 2025 •

edited

Loading

poodlewars Jan 15, 2025 •

edited

Loading