Enable Virtual Environments in testing #2191

ahsan-ca · 2023-09-15T15:38:24Z

This sets up the virtual environments for running python onnx tests.
ctest's fixtures are used to ensure that the virtual environment is
set up before the tests are run.

ahsan-ca

@pfultz2 This is a draft version, needs to be cleaned up and improved.
I have asked some questions for clarification/feedback.
Please feel free to leave more comments/feedback that you may have.
Thanks!

ahsan-ca · 2023-09-15T15:49:14Z

test/py/CMakeLists.txt

+    add_test (NAME SETUP_VENV COMMAND "${Python3_EXECUTABLE}" -m pip install -r ${REQUIREMENTS_FILE})
+    set_tests_properties(SETUP_VENV PROPERTIES FIXTURES_REQUIRED INIT_VENV)
+    set_tests_properties(SETUP_VENV PROPERTIES FIXTURES_SETUP ${FIXTURE_NAME})
+endfunction()

 function(add_py_test NAME SCRIPT)


This function iterates over all the available python versions on the system from the ones specified in PythonModules: (PYTHON_SEARCH_VERSIONS 2.7 3.5 3.6 3.7 3.8 3.9 3.10).
For enabling virtual environments, do we need to consider python 2.7 too? Current implementation of virtual environment is assuming we only work with Python 3.

For posterity; we discussed this and disabled python 2.7.

ahsan-ca · 2023-09-15T15:50:25Z

tools/install_prereqs.sh

@@ -51,6 +51,7 @@ else
    openmp-extras \
    python3-dev \
    python3-pip \
+    python3.8-venv \


I think we would need to add it for sles as well.

sles supports a different level of python (3.7?), Ubuntu 22.04 supports python3.10. I think you'll need to just install the default python3-venv and not be explicit to 3.8

ahsan-ca · 2023-09-15T15:51:53Z

test/py/CMakeLists.txt

@@ -44,6 +66,9 @@ endfunction()
 add_dependencies(tests migraphx_py)
 add_dependencies(check migraphx_py)

+add_py_venv_fixture(
+    ${FIX_NAME} ${VENV_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/REQUIREMENTS.TXT)


The REQUIREMENTS.TXT file lives under test/py, is there a better place for having this file reside?

It should be lower case: requirements.txt.

Done. I have also added a requirements-sles.txt for package requirements on sles.

ahsan-ca · 2023-09-15T15:57:41Z

test/py/CMakeLists.txt

+        set (ENV{VIRTUAL_ENV} ${VENV_DIR})
+        set (Python3_FIND_VIRTUALENV ONLY)
+        unset (Python3_EXECUTABLE)
+        find_package (Python3 COMPONENTS Interpreter Development)


Should these lines be moved to a function since we also have these same lines in add_py_venv_fixture function (35-38)?

Dont use find_package to find python. Instead do set(PYTHON_EXECUTABLE ${VENV_DIR}/${PYTHON_VERSION}/bin/python).

ahsan-ca · 2023-09-15T15:58:07Z

test/py/REQUIREMENTS.TXT

+onnx==1.10.2
+numpy==1.21.6
+typing==3.7.4
+pytest==6.0.1
+packaging==23.0
+protobuf==3.20.0


Any other requirements that we should add here?

codecov · 2023-09-15T17:07:47Z

Codecov Report

Merging #2191 (9847907) into develop (1a1c1b4) will not change coverage.
The diff coverage is n/a.

❗ Current head 9847907 differs from pull request most recent head a5e163e. Consider uploading reports for the commit a5e163e to get more accurate results

@@           Coverage Diff            @@
##           develop    #2191   +/-   ##
========================================
  Coverage    91.45%   91.45%           
========================================
  Files          433      433           
  Lines        16174    16174           
========================================
  Hits         14792    14792           
  Misses        1382     1382

migraphx-bot · 2023-09-16T01:08:09Z

Test	Batch	Rate new a5e163	Rate old 34b68e	Diff	Compare
torchvision-resnet50	64	2,323.95	2,324.38	-0.02%	✅
torchvision-resnet50_fp16	64	5,338.00	5,353.05	-0.28%	✅
torchvision-densenet121	32	1,838.90	1,849.92	-0.60%	✅
torchvision-densenet121_fp16	32	3,418.16	3,413.07	0.15%	✅
torchvision-inceptionv3	32	1,294.31	1,298.13	-0.29%	✅
torchvision-inceptionv3_fp16	32	2,536.42	2,540.25	-0.15%	✅
cadene-inceptionv4	16	620.61	620.61	-0.00%	✅
cadene-resnext64x4	16	589.19	589.41	-0.04%	✅
slim-mobilenet	64	7,212.70	7,211.38	0.02%	✅
slim-nasnetalarge	64	236.58	236.59	-0.00%	✅
slim-resnet50v2	64	2,557.36	2,558.46	-0.04%	✅
bert-mrpc-onnx	8	825.30	824.85	0.06%	✅
bert-mrpc-tf	1	388.66	389.94	-0.33%	✅
pytorch-examples-wlang-gru	1	302.68	303.38	-0.23%	✅
pytorch-examples-wlang-lstm	1	312.06	318.45	-2.01%	✅
torchvision-resnet50_1	1	550.22	545.40	0.88%	✅
torchvision-inceptionv3_1	1	304.15	303.75	0.13%	✅
cadene-dpn92_1	1	356.36	356.96	-0.17%	✅
cadene-resnext101_1	1	220.83	219.20	0.75%	✅
slim-vgg16_1	1	223.94	223.90	0.02%	✅
slim-mobilenet_1	1	1,530.32	1,503.43	1.79%	✅
slim-inceptionv4_1	1	218.43	215.89	1.18%	✅
onnx-taau-downsample	1	306.63	305.90	0.24%	✅
dlrm-criteoterabyte	1	21.68	21.69	-0.04%	✅
dlrm-criteoterabyte_fp16	1	40.75	40.75	0.01%	✅
agentmodel	1	5,835.12	5,818.66	0.28%	✅
unet_fp16	2	55.26	55.23	0.05%	✅
resnet50v1_fp16	1	764.90	761.25	0.48%	✅
bert_base_cased_fp16	64	971.19	970.88	0.03%	✅
bert_large_uncased_fp16	32	305.22	305.17	0.02%	✅
bert_large_fp16	1	167.35	166.96	0.23%	✅
distilgpt2_fp16	16	1,280.35	1,278.95	0.11%	✅

This build is OK for merge ✅

migraphx-bot · 2023-09-16T01:08:11Z

:white_check_mark:bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

:white_check_mark:bert-mrpc-tf: PASSED: MIGraphX meets tolerance

:white_check_mark:pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

:white_check_mark:pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

:white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

:white_check_mark:torchvision-inceptionv3_1: PASSED: MIGraphX meets tolerance

:white_check_mark:cadene-dpn92_1: PASSED: MIGraphX meets tolerance

:white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-inceptionv4_1: PASSED: MIGraphX meets tolerance

:white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

:white_check_mark:agentmodel: PASSED: MIGraphX meets tolerance

:white_check_mark:unet: PASSED: MIGraphX meets tolerance

:white_check_mark:resnet50v1: PASSED: MIGraphX meets tolerance

🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

:white_check_mark:bert_large: PASSED: MIGraphX meets tolerance

🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

pfultz2 · 2023-09-19T15:59:43Z

test/py/CMakeLists.txt

+
+function(add_py_venv_fixture FIXTURE_NAME VIRTUAL_ENV_DIR REQUIREMENTS_FILE)
+    message("Attempting to create virtual environment: ${VIRTUAL_ENV_DIR}")
+    find_package (Python3 COMPONENTS Interpreter)


Dont call find_package(Python3) as this could get a different python version. Instead this should take the PYTHON_EXECUTABLE as an input parameter.

pfultz2 · 2023-09-19T16:10:42Z

test/py/CMakeLists.txt

+    set (Python3_FIND_VIRTUALENV ONLY)
+    unset (Python3_EXECUTABLE)
+    find_package (Python3 COMPONENTS Interpreter Development)
+    add_test (NAME SETUP_VENV COMMAND "${Python3_EXECUTABLE}" -m pip install -r ${REQUIREMENTS_FILE})


This should use ${VIRTUAL_ENV_DIR}/bin/python.

pfultz2 · 2023-09-19T16:11:25Z

test/py/CMakeLists.txt

+    add_test (NAME SETUP_VENV COMMAND "${Python3_EXECUTABLE}" -m pip install -r ${REQUIREMENTS_FILE})
+    set_tests_properties(SETUP_VENV PROPERTIES FIXTURES_REQUIRED INIT_VENV)
+    set_tests_properties(SETUP_VENV PROPERTIES FIXTURES_SETUP ${FIXTURE_NAME})
+endfunction()

 function(add_py_test NAME SCRIPT)


This should take VENV as input parameter.

pfultz2 · 2023-09-19T16:19:34Z

test/py/CMakeLists.txt

+function(add_py_venv_fixture FIXTURE_NAME VIRTUAL_ENV_DIR REQUIREMENTS_FILE)
+    message("Attempting to create virtual environment: ${VIRTUAL_ENV_DIR}")
+    find_package (Python3 COMPONENTS Interpreter)
+    add_test (NAME INITIALIZE_VENV COMMAND "${Python3_EXECUTABLE}" -m venv ${VIRTUAL_ENV_DIR})


The name of the test should be ${FIXTURE_NAME}_initialize_env to avoid conflicts with different python versions or fixtures.

pfultz2 · 2023-09-19T16:20:52Z

test/py/CMakeLists.txt

+    message("Attempting to create virtual environment: ${VIRTUAL_ENV_DIR}")
+    find_package (Python3 COMPONENTS Interpreter)
+    add_test (NAME INITIALIZE_VENV COMMAND "${Python3_EXECUTABLE}" -m venv ${VIRTUAL_ENV_DIR})
+    set_tests_properties(INITIALIZE_VENV PROPERTIES FIXTURES_SETUP INIT_VENV)


The fixture setup name should ${FIXTURE_NAME}_INIT_VENV.

pfultz2 · 2023-09-19T16:21:30Z

test/py/CMakeLists.txt

@@ -44,6 +66,9 @@ endfunction()
 add_dependencies(tests migraphx_py)
 add_dependencies(check migraphx_py)

+add_py_venv_fixture(


This should be called inside add_py_test.

I made the change to call it inside add_py_test but I was hitting issues with the unique naming of tests. The reason is that add_py_test is called multiple times for different tests (ref, save_load, op etc) and the naming we were using for the test name was based on python version only so cmake configure complained that we are having duplicate test names for add_test. I tried checking for the existence of the venv directory to introduce a condition to only add tests if the venv for a particular python version does not exist, but the issue was that the venv directory is created during build step and cmake would fail (complain about duplicate names for add_test) at the configure step when it would not see the directory.
To fix this, I moved add_py_venv_fixture to outside of add_py_test. Inside add_py_venv_fixture, I iterate over all available Python versions. This might not be the most elegant solution, so I am open to suggestions for a better solution.

Discussed this on Friday with Paul, and he suggested to check if a test already exists before adding tests for init/setup venv to resolve the issue with duplication of test names. I have now made that change and add_py_venv_fixture is being called inside add_py_test.

ahsan-ca · 2023-09-22T15:04:41Z

test/py/CMakeLists.txt

+
+        add_test(NAME py_${PYTHON_VERSION}_initialize_env COMMAND ${PYTHON_EXECUTABLE} -m venv ${VIRTUAL_ENV_DIR} --clear)
+        set_tests_properties(py_${PYTHON_VERSION}_initialize_env PROPERTIES FIXTURES_SETUP ${PYTHON_VERSION}_INIT_VENV)
+        set(ENV{VIRTUAL_ENV} ${VIRTUAL_ENV_DIR})


Since we are using the path to python in the virtual environment, this might not be needed. Should I remove this?
Edit: set(ENV{VIRTUAL_ENV} ${VIRTUAL_ENV_DIR}) is similar to activating the virtual env.

This env variable wont be applied when running the test, so this can be removed.

TedThemistokleous

LGTM. Unless @pfultz2 has anything else

Perf errors are from another issue I think we've already resolved on develop.

ahsan-ca · 2023-10-03T19:00:36Z

@pfultz2 I have made changes to the requirement files for python version specific requirement files. Can you please take a look to see if this looks good?
I have not disabled backend tests for 3.6 as they can be run with an older onnx version provided we have the models downloaded (which I believe is how QA does testing).

pfultz2 · 2023-10-04T23:09:07Z

test/py/base-requirements.txt

+# THE SOFTWARE.
+#####################################################################################
+
+pytest==6.0.1


I dont think we need pytest, but we do need numpy for our tests.

I have removed pytest, it was needed for onnx.
Tests pass without numpy. So I have kept this file empty, I can add numpy if you feel it should be here.

pfultz2 · 2023-10-04T23:09:29Z

test/py/requirements-python-3.6.txt

+onnx==1.10.2
+protobuf==3.19.6
+numpy==1.19.5
+packaging==21.3


Remove this file, we should just disable the onnx tests when using python 3.6.

pfultz2 · 2023-10-04T23:09:54Z

test/py/requirements-python-all.txt

+onnx==1.14.1
+protobuf==3.20.2
+numpy==1.21.6
+packaging==23.0


This should be named requirements-onnx.txt.

pfultz2 · 2023-10-04T23:11:37Z

test/py/CMakeLists.txt

+add_py_test(gpu_offload test_gpu_offload.py ${VENV} WORKING_DIRECTORY ${TEST_ONNX_DIR})
+add_py_test(gpu test_gpu.py ${VENV} WORKING_DIRECTORY ${TEST_ONNX_DIR})
+add_py_test(array test_array.py ${VENV} WORKING_DIRECTORY ${TEST_ONNX_DIR})
+add_py_test(backend onnx_backend_test.py ${VENV} WORKING_DIRECTORY ${TEST_ONNX_DIR})


Onnx backend should be using a different virtual environment, and this should be disabled on python 3.6.

I have disabled the onnx backend tests on python 3.6. I am open to a more elegant solution if you think we can have an alternate way to disable this.

pfultz2 · 2023-10-04T23:15:35Z

test/py/CMakeLists.txt

+        if(${PYTHON_VERSION} STREQUAL "3.6")
+            add_py_venv_fixture(${PYTHON_VERSION} ${PYTHON_EXECUTABLE} ${VENV_DIR}/${PYTHON_VERSION} ${REQUIREMENTS_PYTHON_36})
+        else()
+            add_py_venv_fixture(${PYTHON_VERSION} ${PYTHON_EXECUTABLE} ${VENV_DIR}/${PYTHON_VERSION} ${REQUIREMENTS_PYTHON_ALL})


The add_py_env_fixture probably needs to be called outside of the function since tests will use different virtual environments.

I have taken the function out of add_py_test.

pfultz2 · 2023-10-04T23:16:05Z

test/py/CMakeLists.txt

    foreach(PYTHON_VERSION ${PYTHON_VERSIONS})
        set (ENV_COMMAND ${CMAKE_COMMAND} -E env 
            "PYTHONPATH=$<TARGET_FILE_DIR:migraphx_pybind_${PYTHON_VERSION}>"
            "PYTHONMALLOC=debug"
            "MALLOC_CHECK_=3"
        )
        set(PYTHON_EXECUTABLE ${PYTHON_${PYTHON_VERSION}_EXECUTABLE})
+        if(${PYTHON_VERSION} STREQUAL "3.6")


Remove this, we shouldn't have python specific versions of the requirements.

ahsan-ca · 2023-10-06T02:38:50Z

@pfultz2 Thanks for taking a look and providing feedback. I have tried to address the comments. Can you please take a look to see if I have addressed the comments correctly? I am not a 100% sure if the current implementation is how you wanted it.

Side note: The backend test seems to be timing out, I am investigating that but thought to put this up in the meanwhile to get your feedback on the implementation approach.
Thanks!

ahsan-ca · 2023-10-06T20:55:28Z

Side note: The backend test seems to be timing out, I am investigating that but thought to put this up in the meanwhile to get your feedback on the implementation approach. Thanks!

The timing out of backend test was system related. A reboot of the system helped fix the issue.

test/py/requirements.txt

ahsan-ca · 2023-10-11T16:40:24Z

Looks like we have also have try/except clause in another test test_numpy.py as well: https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/a50cb30242d9b328a05f17e34a225310d1210134/test/py/test_numpy.py#L26

Should we also remove this try except clause or leave it as is? @pfultz2

pfultz2 · 2023-10-11T16:41:30Z

That should be removed as well.

ahsan-ca · 2023-10-11T16:49:35Z

That should be removed as well.

Thanks! Removed through #2316.

ahsan-ca · 2023-10-11T20:26:32Z

Updated with develop branch to pull fix of cppcheck issue that has been fixed by df5f8c9.

pfultz2

This looks good, but as a followup we should add a flag to disable the virtual environments so we arent installing these packages everytime we are running tests in the CI.

ahsan-ca · 2023-10-12T19:17:21Z

add a flag to disable the virtual environments so we arent installing these packages everytime we are running tests in the CI.

@pfultz2 Thanks for approving the PR and for all your suggestions/feedback during the course of it. I have opened an issue to add a flag to disable virtual environments: #2324

ahsan-ca self-assigned this Sep 15, 2023

ahsan-ca requested a review from pfultz2 September 15, 2023 15:38

ahsan-ca linked an issue Sep 15, 2023 that may be closed by this pull request

Enable Virtual Environments in testing #1322

Closed

ahsan-ca commented Sep 15, 2023

View reviewed changes

ahsan-ca mentioned this pull request Sep 15, 2023

Enable Virtual Environments in testing #1322

Closed

TedThemistokleous self-requested a review September 15, 2023 16:02

TedThemistokleous added enhancement New feature or request Continous Integration Pull request updates parts of continous integration pipeline labels Sep 15, 2023

pfultz2 reviewed Sep 19, 2023

View reviewed changes

ahsan-ca requested a review from pfultz2 September 22, 2023 15:03

ahsan-ca commented Sep 22, 2023

View reviewed changes

ahsan-ca force-pushed the enable-virtual-env branch from 00c01be to a39bb79 Compare September 25, 2023 16:08

ahsan-ca marked this pull request as ready for review September 25, 2023 16:11

TedThemistokleous added the skip bot checks Skips the Performance and Accuracy CI tests label Sep 28, 2023

TedThemistokleous approved these changes Sep 28, 2023

View reviewed changes

ahsan-ca requested a review from causten October 3, 2023 18:58

pfultz2 reviewed Oct 4, 2023

View reviewed changes

ahsan-ca requested a review from pfultz2 October 6, 2023 02:33

causten removed the skip bot checks Skips the Performance and Accuracy CI tests label Oct 6, 2023

pfultz2 reviewed Oct 6, 2023

View reviewed changes

test/py/requirements.txt Show resolved Hide resolved

ahsan-ca force-pushed the enable-virtual-env branch from 115b84e to afe6260 Compare October 11, 2023 13:54

ahsan-ca added 8 commits October 11, 2023 14:05

setup virtual env for python

c434206

Update prereqs to use python3-venv

8435321

Change package name

46c0690

Address comments to create virtual env for all python versions

cb36a2c

Remove python3-virtualenv, not needed for sles

1e38c4f

Add python version specific requirement files

5a7f072

Address review comments to create separate venv for onnx backend tests

6e64f73

Added numpy to requirements

e6f9f5e

ahsan-ca force-pushed the enable-virtual-env branch from afe6260 to e6f9f5e Compare October 11, 2023 14:05

ahsan-ca requested a review from pfultz2 October 11, 2023 16:09

Merge branch 'develop' into enable-virtual-env

a484e20

pfultz2 approved these changes Oct 12, 2023

View reviewed changes

Merge branch 'develop' into enable-virtual-env

a5e163e

causten merged commit 63b7bd1 into develop Oct 13, 2023
15 checks passed

causten deleted the enable-virtual-env branch October 13, 2023 13:18

Enable Virtual Environments in testing #2191

Enable Virtual Environments in testing #2191

Conversation

ahsan-ca commented Sep 15, 2023

ahsan-ca 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

ahsan-ca Sep 15, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 15, 2023 • edited Loading

Codecov Report

migraphx-bot commented Sep 16, 2023 • edited Loading

migraphx-bot commented Sep 16, 2023 • edited Loading

pfultz2 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

pfultz2 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfultz2 Sep 19, 2023 • 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

Choose a reason for hiding this comment

ahsan-ca Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedThemistokleous left a comment

Choose a reason for hiding this comment

ahsan-ca commented Oct 3, 2023 • 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

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

ahsan-ca commented Oct 6, 2023 • edited Loading

ahsan-ca commented Oct 6, 2023

ahsan-ca commented Oct 11, 2023

pfultz2 commented Oct 11, 2023

ahsan-ca commented Oct 11, 2023

ahsan-ca commented Oct 11, 2023

pfultz2 left a comment

Choose a reason for hiding this comment

ahsan-ca commented Oct 12, 2023

ahsan-ca Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 15, 2023 •

edited

Loading

migraphx-bot commented Sep 16, 2023 •

edited

Loading

migraphx-bot commented Sep 16, 2023 •

edited

Loading

pfultz2 Sep 19, 2023 •

edited

Loading

pfultz2 Sep 19, 2023 •

edited

Loading

pfultz2 Sep 19, 2023 •

edited

Loading

ahsan-ca Sep 22, 2023 •

edited

Loading

ahsan-ca commented Oct 3, 2023 •

edited

Loading

ahsan-ca commented Oct 6, 2023 •

edited

Loading