Improved integration tests #575

VukW · 2024-04-09T15:47:51Z

now config.yaml location can be defined with custom value (env var)
integration tests use a new /tmp/ folder storage with every run
After every command db is backed up to /tmp/%dir%/snapshots/ for easy debug
Every command is printed before execution

NB: despite of it simplifies tests rerun, one still need to reset db storage manually before tests run: ./reset_db.sh && python seed.py

now config.yaml location can be defined with custom value (env var) integration tests use a new /tmp/ folder storage with every run After every command db is backed up to /tmp/%dir%/snapshots/ for easy debug Every command is printed before execution

github-actions · 2024-04-09T15:48:08Z

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

VukW · 2024-04-09T15:48:54Z

cli/cli_chestxray_tutorial_test.sh

 echo "=========================================="
 echo "Creating test profiles for each user"
 echo "=========================================="
-medperf profile activate local
+ev medperf profile activate local


ev is a custom function defined in tests_setup: it echoes command before execution

Could we have a more descriptive name for this? Or an easy way to identify where this function is defined and what is it used for? I can see myself getting confused by this in the future. Maybe eval or print_eval. Might be longer but I'd prefer readability.

Could we have a more descriptive name for this? Maybe eval or print_eval. Might be longer but I'd prefer readability.

Sure:) IMO ev is more readable though. I pushed the fix (renamed ev to print_eval). Plz look and let me know - if it is better for you, let's keep it longish.

VukW · 2024-04-09T15:50:38Z

cli/cli_chestxray_tutorial_test.sh

+echo "We have to return the content back manually, so new tmp folder is empty"
+echo "and all the existing data is kept on the old place."
+mkdir -p ~/.medperf
+ev mv -f $MEDPERF_STORAGE/.medperf/* ~/.medperf/


a crutch: without it
(1) new tmp storage is spoiled with existing data
(2) it might be deleted after test run, so deleting any files you might want to keep

If the intention is to keep test files in case of debugging or similar, I'd prefer to instead specify the location to store them as an argument instead of merging it with the default medperf storage. This is because there's a chance the test files would collide with other files someone might be working on (e.g. I have to do a demonstration of medperf and I'm using the local profile and server for that).

Alternatively, have a flag that disables deleting the tmp folder, and not letting tests run if that folder isn't empty.

I personally prefer tests artifacts to be isolated from the real medperf storage/server if possible.

Disregard my message above, I misunderstood this section given the snippet of code commented and the accompanying message. This is only executed when moving the storage failed for some reason right? And it's just to restore the moved files to their original locations. Correct?

No:)
medperf storage move makes two essential actions:

updates all paths in config to point to a new folder

actually moves the content of ~/.medperf/ to /tmp/medperf_tests_XXXYYY/storage/.medperf/

While (1) is what we want for tests (remember, we have a separate temporary config file while running tests), (2) is eww. Because really we want to work with a new clean empty storage folder while running tests (and, and, in particular, to know it's can be safely deleted at the end).

So, this crutch reverts (2) and moves storage content back to ~/.medperf/.

VukW · 2024-04-09T15:51:54Z

cli/tests_setup.sh

-  medperf profile activate default
-  medperf profile delete testbenchmark
-  medperf profile delete testmodel
-  medperf profile delete testdata


no need to fix list of profiles as whole test root would be deleted

aristizabal95

Hey Slava, this is looking great!

I just left a few suggestions and questions. Other than that I think it's looking good!

aristizabal95 · 2024-04-09T19:33:40Z

cli/cli_chestxray_tutorial_test.sh

 echo "=========================================="
 echo "Creating test profiles for each user"
 echo "=========================================="
-medperf profile activate local
+ev medperf profile activate local


Could we have a more descriptive name for this? Or an easy way to identify where this function is defined and what is it used for? I can see myself getting confused by this in the future. Maybe eval or print_eval. Might be longer but I'd prefer readability.

aristizabal95 · 2024-04-09T19:37:28Z

cli/medperf/config.py

@@ -44,7 +45,7 @@
 # Storage config
 config_storage = Path.home().resolve() / ".medperf_config"
 logs_storage = Path.home().resolve() / ".medperf_logs"
-config_path = str(config_storage / "config.yaml")
+config_path = getenv("MEDPERF_CONFIG_PATH", str(config_storage / "config.yaml"))


Defining this env var is now required for all medperf installations?

No, if env is not defined, default value is used (str(config_storage / "config.yaml")). getenv works similar to dict.get() method

aristizabal95 · 2024-04-09T19:40:15Z

cli/tests_setup.sh

+ev() {
+    local timestamp=$(date +%m%d%H%M%S)
+    local formatted_cmd=$(echo "$@" | sed 's/[^a-zA-Z0-9]\+/_/g' | cut -c 1-50)
+    LAST_SNAPSHOT_PATH="$SNAPSHOTS_FOLDER/${timestamp}_${formatted_cmd}.sqlite3"


Could we end up clutteraing this folder indefinitely if we're not cleaning it consistently? Would there be a way of defining a max amount of snapshots that can exist at a given time?

Not exactly

tmp/ |-- medperf_tests_20240409140527/ |--|-- snapshots/ # here all the snapshots for that run are stored |--|--|--.... |--|-- storage/ |--|--|-- .medperf/ |--|--|--|-- .... |--|-- config.yaml

So, this snapshots folder belongs to a specific tests run, not mixing with snapshots from other runs. No need to manage especially snapshots. If you don't need this test results anymore (or you use -c/CLEAN flag), you delete a whole medperf_tests_XXXYYY.

However by default -c is set up to False. So, we are cluttering /tmp/ folder with a bunch of medperf_tests_XXXYYY.

What do you think guys if we ultimately remove test root folder if tests were passed successfully? And oldish crap after failed tests we can remove manually.

Or I can implement some kind of logic "on every run delete medperf_tests_XXXYYY that are older than a month", though it looks like overengineering to me

hasan7n

@VukW man, that's great! this makes lives a lot easier :)
one small suggestion is to move the Changing storage to tmp location bash code into setup since it is common, but that's not necessary!
Thanks for this!

…e-debug

VukW · 2024-05-06T14:55:24Z

Hi @hasan7n thanks, you are right, moved config&storage init to tests_setup.sh.

Also updated "storage moving": instead of real moving files back and forth (that may be error-prone if broken on the mid) we explicitly point config pathes to a newly created empty tmp folder. That is safer IMO

VukW · 2024-05-07T14:47:30Z

@hasan7n fixed, ready for re-review:)

hasan7n

Looks great man!

Improved tests

e78f379

now config.yaml location can be defined with custom value (env var) integration tests use a new /tmp/ folder storage with every run After every command db is backed up to /tmp/%dir%/snapshots/ for easy debug Every command is printed before execution

VukW requested a review from a team as a code owner April 9, 2024 15:47

VukW had a problem deploying to testing-external-code April 9, 2024 15:47 — with GitHub Actions Failure

VukW commented Apr 9, 2024

View reviewed changes

debug

10d4452

VukW had a problem deploying to testing-external-code April 9, 2024 16:24 — with GitHub Actions Failure

Debug

b0b1c8f

VukW had a problem deploying to testing-external-code April 9, 2024 16:31 — with GitHub Actions Failure

debug

fed9395

VukW had a problem deploying to testing-external-code April 9, 2024 16:47 — with GitHub Actions Failure

debug

79139c9

VukW had a problem deploying to testing-external-code April 9, 2024 18:02 — with GitHub Actions Failure

debug

e7df3b9

VukW had a problem deploying to testing-external-code April 9, 2024 18:19 — with GitHub Actions Failure

Fix: reodered folder creation and clean on -f

0f813b9

VukW had a problem deploying to testing-external-code April 9, 2024 18:37 — with GitHub Actions Failure

import os breaks pytest

7fc2750

VukW had a problem deploying to testing-external-code April 9, 2024 19:04 — with GitHub Actions Failure

aristizabal95 reviewed Apr 9, 2024

View reviewed changes

Renamed ev to print_eval

1f144ba

VukW had a problem deploying to testing-external-code April 9, 2024 20:16 — with GitHub Actions Failure

hasan7n self-requested a review May 3, 2024 15:02

hasan7n previously approved these changes May 3, 2024

View reviewed changes

Instead of real storage moving just set up config properly

3903beb

VukW dismissed hasan7n’s stale review via 3903beb May 6, 2024 14:52

VukW requested a deployment to testing-external-code May 6, 2024 14:53 — with GitHub Actions Waiting

Merge remote-tracking branch 'origin/main' into integration-tests-eas…

631c910

…e-debug

VukW requested a deployment to testing-external-code May 6, 2024 14:55 — with GitHub Actions Waiting

bugfix

42a01ff

VukW requested a deployment to testing-external-code May 6, 2024 15:06 — with GitHub Actions Waiting

bugfix

783f1cd

VukW requested a deployment to testing-external-code May 7, 2024 13:05 — with GitHub Actions Waiting

bugfix

6f12b1e

VukW requested a deployment to testing-external-code May 7, 2024 13:06 — with GitHub Actions Waiting

VukW added 2 commits May 7, 2024 16:45

bugfix

eda3d93

Covered multisource tests also

8ea2c40

VukW requested a deployment to testing-external-code May 7, 2024 13:48 — with GitHub Actions Waiting

Merge branch 'main' into integration-tests-ease-debug

8426e7a

VukW requested a review from a team as a code owner May 15, 2024 14:45

hasan7n deployed to testing-external-code May 15, 2024 14:45 — with GitHub Actions Active

hasan7n approved these changes May 15, 2024

View reviewed changes

VukW merged commit 9bb807b into mlcommons:main May 20, 2024
8 checks passed

github-actions bot locked and limited conversation to collaborators May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved integration tests #575

Improved integration tests #575

VukW commented Apr 9, 2024 •

edited

github-actions bot commented Apr 9, 2024 •

edited

VukW Apr 9, 2024

aristizabal95 Apr 9, 2024

VukW Apr 9, 2024

VukW Apr 9, 2024

aristizabal95 Apr 9, 2024 •

edited

aristizabal95 Apr 9, 2024

VukW Apr 9, 2024

VukW Apr 9, 2024

aristizabal95 left a comment

aristizabal95 Apr 9, 2024

aristizabal95 Apr 9, 2024

VukW Apr 9, 2024

aristizabal95 Apr 9, 2024

VukW Apr 9, 2024 •

edited

hasan7n left a comment

VukW commented May 6, 2024

VukW commented May 7, 2024

hasan7n left a comment

Improved integration tests #575

Improved integration tests #575

Conversation

VukW commented Apr 9, 2024 • edited

github-actions bot commented Apr 9, 2024 • edited

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

aristizabal95 Apr 9, 2024 • edited

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

aristizabal95 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

VukW Apr 9, 2024 • edited

Choose a reason for hiding this comment

hasan7n left a comment

Choose a reason for hiding this comment

VukW commented May 6, 2024

VukW commented May 7, 2024

hasan7n left a comment

Choose a reason for hiding this comment

VukW commented Apr 9, 2024 •

edited

github-actions bot commented Apr 9, 2024 •

edited

aristizabal95 Apr 9, 2024 •

edited

VukW Apr 9, 2024 •

edited