Skip to content

Conversation

suo
Copy link
Member

@suo suo commented Sep 24, 2020

Stack from ghstack:

Context for why we are porting to gtest in: #45018.

This PR completes the process of porting and removes unused files/macros.

Differential Revision: D23901392

[ghstack-poisoned]
suo added a commit that referenced this pull request Sep 24, 2020
ghstack-source-id: 2314ab5
Pull Request resolved: #45264
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 24, 2020
@dr-ci
Copy link

dr-ci bot commented Sep 24, 2020

💊 CI failures summary and remediations

As of commit 6575c3a (more details on the Dr. CI page):


  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_linux_xenial_py3_clang5_asan_test2 (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

Sep 25 07:57:40 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /var/lib/jenkins/workspace/aten/src/ATen/Utils.cpp:11:3 in
Sep 25 07:57:40     #7 0x556e5e61d70b in PyEval_EvalCode /tmp/build/80754af9/python_1599604603603/work/Python/ceval.c:731 
Sep 25 07:57:40     #8 0x556e5e69d573 in run_mod /tmp/build/80754af9/python_1599604603603/work/Python/pythonrun.c:1025 
Sep 25 07:57:40     #9 0x556e5e69d60c in PyRun_StringFlags /tmp/build/80754af9/python_1599604603603/work/Python/pythonrun.c:949 
Sep 25 07:57:40     #10 0x556e5e69d66e in PyRun_SimpleStringFlags /tmp/build/80754af9/python_1599604603603/work/Python/pythonrun.c:445 
Sep 25 07:57:40     #11 0x556e5e6a1472 in run_command /tmp/build/80754af9/python_1599604603603/work/Modules/main.c:301 
Sep 25 07:57:40     #12 0x556e5e6a1472 in Py_Main /tmp/build/80754af9/python_1599604603603/work/Modules/main.c:749 
Sep 25 07:57:40     #13 0x556e5e56b43d in main /tmp/build/80754af9/python_1599604603603/work/Programs/python.c:69 
Sep 25 07:57:40     #14 0x7fdaa3ee683f in __libc_start_main /build/glibc-e6zv40/glibc-2.23/csu/../csu/libc-start.c:291 
Sep 25 07:57:40     #15 0x556e5e64ad0a in _start /home/rdonnelly/mc/conda-bld/compilers_linux-64_1534865402226/work/.build/src/glibc-2.12.2/csu/../sysdeps/x86_64/elf/start.S:103 
Sep 25 07:57:40  
Sep 25 07:57:40 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /var/lib/jenkins/workspace/aten/src/ATen/Utils.cpp:11:3 in  
Sep 25 07:57:40 + retcode=1 
Sep 25 07:57:40 + set -e 
Sep 25 07:57:40 + return 1 
Sep 25 07:57:40 + [[ pytorch-linux-xenial-py3-clang5-asan-test2 == *-NO_AVX-* ]] 
Sep 25 07:57:40 + [[ pytorch-linux-xenial-py3-clang5-asan-test2 == *-NO_AVX2-* ]] 
Sep 25 07:57:40 + '[' -n https://github.com/pytorch/pytorch/pull/45264 ']' 
Sep 25 07:57:40 + [[ pytorch-linux-xenial-py3-clang5-asan-test2 != *coverage* ]] 
Sep 25 07:57:40 ++ mktemp 
Sep 25 07:57:40 + DETERMINE_FROM=/tmp/tmp.iIYwnB01H6 
Sep 25 07:57:40 + file_diff_from_base /tmp/tmp.iIYwnB01H6 

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 16 times.

@suo
Copy link
Member Author

suo commented Sep 24, 2020

cc @csarofeen: heads up that this PR changes the GPU fuser tests to be registered through gtest only. So running cpp tests through the test_jit.py will no longer work. Let me know that is an issue!

Context for why we are porting to gtest in: #45018.

This PR completes the process of porting and removes unused files/macros.

Differential Revision: [D23901392](https://our.internmc.facebook.com/intern/diff/D23901392)

[ghstack-poisoned]
Context for why we are porting to gtest in: #45018.

This PR completes the process of porting and removes unused files/macros.

Differential Revision: [D23901392](https://our.internmc.facebook.com/intern/diff/D23901392)

[ghstack-poisoned]
suo added a commit that referenced this pull request Sep 24, 2020
ghstack-source-id: 0df18a6
Pull Request resolved: #45264
@suo suo requested a review from ZolotukhinM September 24, 2020 19:09
@jjsjann123
Copy link
Collaborator

So running cpp tests through the test_jit.py will no longer work. Let me know that is an issue!

IIUC, CI would still run cpp tests, but jut not via the python script?
I don't fully understand how our own CI works, but I suspect it's relying on test_jit.py to cover our cpp tests. Can you point me to where cpp tests are run in upstream CI? so I can mimic that in our own :)

@suo
Copy link
Member Author

suo commented Sep 24, 2020

IIUC, CI would still run cpp tests, but jut not via the python script?

Correct, our upstream CI runs the JIT tests here: https://github.com/pytorch/pytorch/blob/master/.jenkins/pytorch/test.sh#L200. Adding a similar line in your CI should be sufficient.

Copy link

@ZolotukhinM ZolotukhinM left a comment

Choose a reason for hiding this comment

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

Mostly looks good, my only question is about thread_init_test.cpp (see below).

#include <ATen/ATen.h>
#include <ATen/Parallel.h>
#include <test/cpp/jit/test_base.h>
#include <test/cpp/tensorexpr/test_base.h>

Choose a reason for hiding this comment

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

This looks kinda awkward. Why does this depend on something in tensorexpr now? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This test depends on some of the gtest polyfills in test_base.h. Since I am deleting the JIT's version, I included the tensorexpr copy to avoid editing this test too much. I think it wouldn't be too hard to either 1) include gtest or 2) just define the polyfills here, but I didn't want to make this PR bigger in non-JIT places.

ASSERT_FALSE(stack[2].toBool());
}

// TODO: These tests weren't doing anything.

Choose a reason for hiding this comment

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

Do these tests pass if we enable them now?

Copy link
Member Author

Choose a reason for hiding this comment

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

There were a few tests that weren't running, because they were defined in a test file but not registered in tests.h. If they are commented out, it's because they failed when I re-enabled them :(

Context for why we are porting to gtest in: #45018.

This PR completes the process of porting and removes unused files/macros.

Differential Revision: [D23901392](https://our.internmc.facebook.com/intern/diff/D23901392)

[ghstack-poisoned]
suo added a commit that referenced this pull request Sep 25, 2020
ghstack-source-id: fc3999f
Pull Request resolved: #45264
@csarofeen
Copy link
Contributor

@suo you mentioned a conflict, can we give you a hand? CC @jjsjann123

@facebook-github-bot
Copy link
Contributor

@suo merged this pull request in 22401b8.

@suo
Copy link
Member Author

suo commented Sep 25, 2020

@suo you mentioned a conflict, can we give you a hand? CC @jjsjann123

no worries, I just rebased and fixed the conflicts manually. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants