Skip to content

Conversation

@ajyu
Copy link
Contributor

@ajyu ajyu commented Sep 6, 2018

Followup to the serialized test framework

Round 1 for refactoring tests, starting alphabetically. I added some functionality, so I wanted to send out some of these initial changes sooner.

I'm skipping all tests that don't explicitly call assertReferenceChecks. Some tests directly call np.allclose, and others are simply TestCase (rather than HypothesisTestCase).

  1. Start alphabetically producing serialized outputs for test functions, annotating those we want to include with @serialized_test_util.given. So far I've only added one test per operator, but this already does seem to add quite a few tests.
  2. Add functionality to allow us to generate outputs using pytest by adding pytest argument options. This allows us to skip adding a __main__ function to quite a few tests.
  3. Catch any exceptions generating the gradient operator and skip serializing/reading it, since certain operators don't have gradients.
  4. Add functionality to better handle jagged array inputs, which numpy doesn't handle very well. We simply explicitly do the conversion to dtype=object.
  5. Make only one file per test function, rather than 4, to reduce the number of files in the github repo.

I also noticed that there is some hypothesis handling that makes @serialized_test_util.given not compatible with adding more hypothesis decorators on top. For example, there are tests that do

@settings(...)
@given(...)
def test_my_stuff(...)

But there is a hypothesis handler that explicitly checks that @given is called below @settings, so we cannot refactor this to @serialized_test_util.given. I've just avoided decorating these kinds of tests for now, I hope that's alright.

@ajyu
Copy link
Contributor Author

ajyu commented Sep 6, 2018

@houseroad @ilia-cher @BIT-silence @apaszke for review

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

ajyu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Can we generate a table in the md doc which stats the test cases by the script?

return (param_out, mom_out)

@given(inputs=hu.tensors(n=3),
@serial.given(inputs=hu.tensors(n=3),

This comment was marked as off-topic.

This comment was marked as off-topic.

2. Change the `@given` decorator to `@serialized_test_util.given`. This runs a seeded hypothesis test instance which will generate outputs if desired in addition to the unseeded hypothesis tests normally run.
3. [Optional] Add (or change a call of `unittest.main()` to) `testWithArgs` in `__main__`. This allows you to generate outputs using `python caffe2/python/operator_test/my_test.py -G`.
4. Run your test `python -m pytest caffe2/python/operator_test/my_test.py -G` to generate serialized outputs. They will live in `caffe2/python/serialized_test/data/operator_test`, one npz file per test function. Use `-O` to change the output directory.
5. Thereafter, runs of the test without the flag will load serialized outputs and gradient operators for comparison against the seeded run. The comparison is done as long as you have a call to assertReferenceChecks. If for any reason the seeded run's inputs are different (this can happen with different hypothesis versions or different setups), then we'll run the serialized inputs through the serialized operator to get a runtime output for comparison.

This comment was marked as off-topic.

This comment was marked as off-topic.

ajyu added a commit to ajyu/pytorch that referenced this pull request Sep 18, 2018
Summary: Followup to the [first
refactor](pytorch#11350). Increase
coverage of tests

Test Plan: Run test with `python -m pytest
caffe2/python/operator_test/my_test.py`

Reviewers:

Subscribers:

Tasks:

Tags:
facebook-github-bot pushed a commit that referenced this pull request Sep 19, 2018
Summary:
Followup to the [first refactor](#11350). Increase coverage of tests
Pull Request resolved: #11811

Reviewed By: houseroad

Differential Revision: D9923074

Pulled By: ajyu

fbshipit-source-id: 0f899bb9e9a75bf7ed939e06cc9b028daa7f6bd9
@ajyu ajyu deleted the less-files branch September 20, 2018 21:54
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants