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

Add CHAINER_TEST_PAIRWISE_PARAMETERIZATION and enable it only in Travis CI #8211

Merged
merged 3 commits into from Oct 16, 2019

Conversation

niboshi
Copy link
Member

@niboshi niboshi commented Oct 1, 2019

See #8210

Adding CHAINER_TEST_PAIRWISE_PARAMETERIZATION environment variable and only set it to always in Travis CI.

@niboshi niboshi changed the title [RFC] Add CHAINER_TEST_PAIRWISE_PARAMETERIZE and enable it only in Travis CI [RFC] Add CHAINER_TEST_PAIRWISE_PARAMETERIZATION and enable it only in Travis CI Oct 1, 2019
Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

Looks good for introducing the env variable.

On moving the code to tests/conftest.py,

  • I confirmed that PyTest (4.1.1) reads multiple conftest.py files.
  • Could you fix the TODO comment on _product_dict_orig in chainer/testing/parameterized.py? ("is patched by tests/conftest.py")
  • I'd like to hear ChainerMN team's comment on the pairwise testing.

@niboshi
Copy link
Member Author

niboshi commented Oct 2, 2019

  • Could you fix the TODO comment on _product_dict_orig in chainer/testing/parameterized.py? ("is patched by tests/conftest.py")

Fixed in ef3c7e2.

  • I'd like to hear ChainerMN team's comment on the pairwise testing.

I don't think we should enable pairwise testing in chainermn_tests.
It only takes 1 minute in Travis CI.

@niboshi niboshi changed the title [RFC] Add CHAINER_TEST_PAIRWISE_PARAMETERIZATION and enable it only in Travis CI Add CHAINER_TEST_PAIRWISE_PARAMETERIZATION and enable it only in Travis CI Oct 2, 2019
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@emcastillo
Copy link
Member

Jenkins, test this please

@emcastillo emcastillo added the cat:test Test or CI related. label Oct 2, 2019
@emcastillo emcastillo added this to the v7.0.0rc1 milestone Oct 2, 2019
@emcastillo emcastillo added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Oct 2, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit ef3c7e2, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member

Jenkins error might be related

@toslunar
Copy link
Member

toslunar commented Oct 2, 2019

  • I'd like to hear ChainerMN team's comment on the pairwise testing.

I don't think we should enable pairwise testing in chainermn_tests.
It only takes 1 minute in Travis CI.

I don't think it's needed. I meant that tests/conftest.py will enable it.

@testing.parameterize(*testing.product({
'shape_x': [[(4, 5), (3, 2)], [(3, 2)], [()]],
'shape_delegate': [(3, 2), ()],
'dtype': [numpy.float16, numpy.float32, numpy.float64, chainer.mixed16],
}))

@niboshi
Copy link
Member Author

niboshi commented Oct 2, 2019

@toslunar
No, it's disabled by default.

@niboshi
Copy link
Member Author

niboshi commented Oct 3, 2019

@emcastillo
The error should be fixed by #8217.

@toslunar
Copy link
Member

toslunar commented Oct 3, 2019

@toslunar
No, it's disabled by default.

I see. Could you fix step_chainer_tests, too?

@niboshi
Copy link
Member Author

niboshi commented Oct 3, 2019

@toslunar

I see. Could you fix step_chainer_tests, too?

Hm, do we need that?
Currently (only chainerx_tests) Travis takes 44 minutes, which has enough margin of 6 minutes.

Also if it were applied to chainer_tests, we wouldn't be able to test #8216.

@toslunar
Copy link
Member

toslunar commented Oct 3, 2019

48 min 41 sec with macOS https://travis-ci.org/chainer/chainer/builds/592878722 where pairwise testing is applied to chainer and chainerx. How about fixing #8073 first?

@niboshi
Copy link
Member Author

niboshi commented Oct 3, 2019

@toslunar I see. That sounds good.

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as st:test-and-merge, but there were no activities for the last 3 days. Could you check?

2 similar comments
@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as st:test-and-merge, but there were no activities for the last 3 days. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as st:test-and-merge, but there were no activities for the last 3 days. Could you check?

@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit f5db68b, target branch master) succeeded!

@mergify mergify bot merged commit 12d4ca1 into chainer:master Oct 16, 2019
@niboshi niboshi deleted the pairwise-selective branch October 17, 2019 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:test Test or CI related. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants