Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use nox for testing #4841

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from
Draft

Use nox for testing #4841

wants to merge 28 commits into from

Conversation

justinchuby
Copy link
Contributor

@justinchuby justinchuby commented Feb 1, 2023

Description

Use nox to run tests. Only linux and macOS is using nox right now.

Additionally:

  1. Use ubuntu-latest as the linux image, upgrading from 18.02
  2. Add python 3.10 and 3.11
  3. Update pipelines to improve how we set environment variables
  4. Upload coverage results to codecov
  5. Update googletest to the latest commit

Motivation and Context

Unified and repeatable test runs.

Fixes #4791

cc @jcwchen

@justinchuby justinchuby requested a review from a team as a code owner February 1, 2023 21:10
Copy link
Member

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

@jcwchen It says I am missing pybind11 (https://dev.azure.com/onnx-pipelines/onnx/_build/results?buildId=36629&view=logs&j=51991ee2-49eb-5ba6-75d8-d9fa44c75e77&t=2ba8a090-2ce7-5eb6-17ea-8e3db6b5fc5d&l=66). Do you have a clue?

This is normal. Later on ONNX will build pybind11 from ONNX's submodule.

According the complete error message, it's more like a Protobuf issue. I thought you have removed installing Protobuf from requirements-release.txt. Using a fixed version of Protobuf (3.20.2) should help to resolve this issue.

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Copy link
Member

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

I was wrong. The issue should be export env variables are not shared in different script. Before building ONNX, you will need to manually set export CMAKE_ARGS="-DONNX_USE_PROTOBUF_SHARED_LIBS=ON" again. The error shows you are using a shared Protobuf (comes from apt-get), but the build is using ONNX_USE_PROTOBUF_SHARED_LIBS=OFF".

@justinchuby
Copy link
Contributor Author

Yeah that's what I figured too. I am trying to see how I can configure the envvars properly

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby
Copy link
Contributor Author

@justinchuby justinchuby changed the title Use nox Use nox for testing Feb 2, 2023
@justinchuby
Copy link
Contributor Author

@jcwchen pipelines are now working, but its strange that some pbs are not up to date https://dev.azure.com/onnx-pipelines/onnx/_build/results?buildId=36686&view=logs&j=08e05908-426e-595c-5fcc-fd56e5a3c39e&t=74710ad4-2707-5404-bc87-4a4ad8cca814&l=5412

Could it be caused by system image upgrade?

@jcwchen
Copy link
Member

jcwchen commented Feb 2, 2023

@jcwchen pipelines are now working, but its strange that some pbs are not up to date https://dev.azure.com/onnx-pipelines/onnx/_build/results?buildId=36686&view=logs&j=08e05908-426e-595c-5fcc-fd56e5a3c39e&t=74710ad4-2707-5404-bc87-4a4ad8cca814&l=5412

Perhaps also exclude coverage.xml for git diff?

@justinchuby
Copy link
Contributor Author

Good idea, although I don’t think it’s coverage because untracked files are not going to cause git to return non zero

“Changes not staged for commit:” has some pb files for log sum exp.

@justinchuby
Copy link
Contributor Author

Is there a way to read / visualize the files?

@jcwchen
Copy link
Member

jcwchen commented Feb 2, 2023

“Changes not staged for commit:” has some pb files for log sum exp.

Those should be excluded by ':!onnx/onnx-data.proto' ':!onnx/onnx-data.proto3' ':!*output_*.pb' ':!*input_*.pb', IIUC. So actually I don't know what happened... Will take a deeper look

@justinchuby
Copy link
Contributor Author

I see what’s happening. I reverted a condition

Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby
Copy link
Contributor Author

@jcwchen Done

onnx_ml: true
onnx_debug: false
documentation: false
maxParallel: 7
Copy link
Member

Choose a reason for hiding this comment

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

nit: since we are adding two more environments (3.10 + 3.11) in CIs, probably we can remove two existing environments (e.g., one Python 3.7 and one Python 3.8) to keep the number of CI pipelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tools/check_generated_backend_test_data.sh Show resolved Hide resolved
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby
Copy link
Contributor Author

Linux getting gmake: *** [Makefile:136: all] Error 2 (used to work) I wonder what it was...

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby
Copy link
Contributor Author

I think it is

CMake Deprecation Warning at googletest/CMakeLists.txt:49 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

@justinchuby
Copy link
Contributor Author

I think it is

CMake Deprecation Warning at googletest/CMakeLists.txt:49 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

hmm no. cmake is 3.25.1

@justinchuby
Copy link
Contributor Author

google test errors. Do we need to update googletest?

[ 50%] Building CXX object googletest/CMakeFiles/gtest.dir/src/gtest-all.cc.o
In file included from /home/vsts/work/1/s/.setuptools-cmake-build/googletest/src/googletest/googletest/src/gtest-all.cc:42:
/home/vsts/work/1/s/.setuptools-cmake-build/googletest/src/googletest/googletest/src/gtest-death-test.cc: In function ‘bool testing::internal::StackGrowsDown()’:
/home/vsts/work/1/s/.setuptools-cmake-build/googletest/src/googletest/googletest/src/gtest-death-test.cc:1224:24: error: ‘dummy’ may be used uninitialized [-Werror=maybe-uninitialized]
 1224 |   StackLowerThanAddress(&dummy, &result);
      |   ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
/home/vsts/work/1/s/.setuptools-cmake-build/googletest/src/googletest/googletest/src/gtest-death-test.cc:1214:13: note: by argument 1 of type ‘const void*’ to ‘void testing::internal::StackLowerThanAddress(const void*, bool*)’ declared here
 1214 | static void StackLowerThanAddress(const void* ptr, bool* result) {
      |             ^~~~~~~~~~~~~~~~~~~~~
/home/vsts/work/1/s/.setuptools-cmake-build/googletest/src/googletest/googletest/src/gtest-death-test.cc:1222:7: note: ‘dummy’ declared here
 1222 |   int dummy;
      |       ^~~~~
cc1plus: all warnings being treated as errors

@justinchuby
Copy link
Contributor Author

@justinchuby
Copy link
Contributor Author

This is because we are using a newer gcc with ubuntu 20.04

justinchuby and others added 4 commits February 3, 2023 04:01
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby
Copy link
Contributor Author

gmake[2]: *** No rule to make target 'googletest/src/googletest/googletest/libgtestd.a', needed by 'onnx_gtests'. Stop.

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Copy link
Member

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

I thought previously all pipelines are green? Any new things added causes googletest to fail?

.azure-pipelines/Linux-CI.yml Outdated Show resolved Hide resolved
.azure-pipelines/MacOS-CI.yml Outdated Show resolved Hide resolved
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby
Copy link
Contributor Author

@jcwchen Reverted image versions. Now only macos is failing because of the not found protobuf compiler. Any idea how I can fix that?

Copy link
Member

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

.

# in a private namespace.
! grep -R --include='*.cc' --include='*.h' 'namespace onnx' .
! grep -R --include='*.cc' --include='*.h' 'onnx::' .
pip install nox
Copy link
Member

Choose a reason for hiding this comment

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

@jcwchen Reverted image versions. Now only macos is failing because of the not found protobuf compiler. Any idea how I can fix that?

Cross scripts is sometimes a headache in AZP. I guess the export path in the first script for installing Protobuf from source was not applied in the following scripts? If yes, manually export in the this script before installing onnx like

export PATH=$INSTALL_PROTOBUF_PATH/include:$INSTALL_PROTOBUF_PATH/lib:$INSTALL_PROTOBUF_PATH/bin:$PATH
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize there was a path exported. thanks!

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
noxfile.py Fixed Show fixed Hide fixed
@justinchuby
Copy link
Contributor Author

I need to set an env var that is not PATH. Is there another var I can set?

@jcwchen
Copy link
Member

jcwchen commented Feb 4, 2023

I need to set an env var that is not PATH. Is there another var I can set?

Could you please elaborate? I thought you can directly use any variable name you want?

Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby justinchuby marked this pull request as draft February 22, 2023 21:20
@github-actions github-actions bot added stale and removed stale labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use tox or nox for running tests
3 participants