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

[DOCKER] Fix broken test environment for test_gemm_acc32_vnni.py #4097

Closed
wants to merge 1 commit into from
Closed

[DOCKER] Fix broken test environment for test_gemm_acc32_vnni.py #4097

wants to merge 1 commit into from

Conversation

mshawcroft
Copy link
Contributor

The following commit:

bb82e09
Author: Jianyu Huang [email protected]
Date: Fri Sep 13 13:27:40 2019 -0700
Add AVX512VNNI support for TVM (#3388)

Introduced a test case dependency on the nose package which is
currently not installed by the docker install scripts. Add the
package.

Change-Id: I96debb44801f4a2ea98e4a72daa495342a44b7d8

The following commit:

bb82e09
Author: Jianyu Huang <[email protected]>
Date:   Fri Sep 13 13:27:40 2019 -0700
Add AVX512VNNI support for TVM (#3388)

Introduced a test case dependency on the nose package which is
currently not installed by the docker install scripts.  Add the
package.

Change-Id: I96debb44801f4a2ea98e4a72daa495342a44b7d8
@tqchen
Copy link
Member

tqchen commented Oct 10, 2019

We have moved to pytest env, it is interesting that nose was still available. A better fix would be to change that to use https://doc.pytest.org/en/latest/skipping.html#skipping-test-functions instead.

@tqchen tqchen added the status: need update need update based on feedbacks label Oct 10, 2019
@mshawcroft
Copy link
Contributor Author

@tqchen understood.

I wonder, has the nose package been dropped from docker/install/xxx.sh or whereever it was originally installed from, but the deployed CI system not updated?

These tests are clearly passing in current upstream CI which indicates that "nose" remains available in CI today, which leaves the door open for further PRs to creep in re-introducing further dependencies on nose.

I'm not going to have time rework a proper fix for this in the immediate future. Perhaps the original author of the patch @jianyuh would be able to fix their patch as you've requested above?

@u99127
Copy link
Contributor

u99127 commented Oct 11, 2019

@tqchen Are you suggesting the test is skipped because nose isn't available, consequently losing test coverage ?

regards
Ramana

@tqchen
Copy link
Member

tqchen commented Oct 11, 2019

But it has nothing to do with nose, as we started to migrate to pytest infra due to nose is no longer in the maintenance mode.

That particular testcase was marked as skipped because the lack of CI env supporting the latest skylake intel. But a better approach would be runtime detect the feature and optionally skip the test

@mshawcroft
Copy link
Contributor Author

@u99127 the test case uses the nose notest mechanism to disable the test case. So, as tqchen states, the test case is not intended to run right now.

But whether the test case is intended to execute or not is a secondary issue here, more important is that that the CI system continues to provide nose in the environment despite nose not being installed by the docker setup scripts. @tqchen why is that? and what needs to be done to make the actual CI system executing consistent with the docker setup scripts?

@tqchen
Copy link
Member

tqchen commented Oct 11, 2019

We used to rebuild the docker image when-ever the Docker script changes in. This would be more akin to what you said about keep the image up to date. However we stopped that approach mainly because the cause of growing number of images(and the cost of disk space), and potential broken dependencies which could cause CI outage

Currently we manually tag and stage the CI docker(by change the tag in the Jenkinsfile). It also enables the CI worker to use a limited amount disk resources(as they always point to a version), as well as making reproducing error easier(as we always point to the same binary tag). Given that we do not change the CI env frequently, we find it is a better approach.

The drawback is of course there could be a lag between the docker script update and CI image. Right now we do that actively whenever we update the docker script, which works fine. I believe specific instance for the CPU was due to the fact that there was an error of rust env that prevents us from updating the CPU docker, I will take a stab to see if we can sync it, but of course we will need to fix the testcase before that :)

@tqchen
Copy link
Member

tqchen commented Oct 11, 2019

@u99127
Copy link
Contributor

u99127 commented Oct 11, 2019

I see, thanks for the explanation, see #4107 for my attempt at fixing the test.

@jianyuh
Copy link
Contributor

jianyuh commented Oct 11, 2019

The nose package is introduced after #3516. @anijain2305 might want to take a look. The original introduction is to avoid some errors from #3598 and #3516.

@tqchen tqchen closed this Oct 14, 2019
@tqchen
Copy link
Member

tqchen commented Oct 14, 2019

Just a followup note for @mshawcroft and @u99127 . The best way to get consistent behavior with upstream test env is to use the same binary image as indicated in https://github.com/dmlc/tvm/blob/master/Jenkinsfile#L43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants