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

[UnitTest] Fuzz based on seed rather than random value. #10515

Merged

Conversation

Lunderberg
Copy link
Contributor

Some extensions to run tests in parallel (e.g. pytest-xdist) require that test collection be deterministic. Using the random seed as the test parameter instead of the random value makes the test collection be deterministic.

Some extensions to run tests in parallel (e.g. `pytest-xdist`) require
that test collection be deterministic.  Using the random seed as the
test parameter instead of the random value makes the test collection
be deterministic.
Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

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

LGTM generally, but the test is not really random anymore right. You might as well just use fixed inputs?

Furthermore is this a general problem we have in the codebase? Do we want to be able to use xdist?

@Lunderberg
Copy link
Contributor Author

That's correct. I initially added this test to validate copying across a few orders of magnitude, and a fixed input size would work just as well. If we want to have the test be randomized per run, we could remove the call to np.random.seed. I'm fine with it either way, or switching to a fixed array.

Most of the other unit tests don't run into the xdist incompatibility, because they didn't change the name of the test being run. It only became an issue here because the seed value was included in the parametrized test name. I've found it really useful to be able to use xdist to speed up local test times. Other than some false positives from running out of GPU memory, most of the other tests are xdist-friendly.

Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

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

Sounds good to me, maybe we should consider xdist on CI then? @driazati @areusch ?

@AndrewZhaoLuo AndrewZhaoLuo merged commit d37c1d2 into apache:main Mar 23, 2022
@Lunderberg Lunderberg deleted the deterministic_pytest_collection branch March 23, 2022 15:18
@Lunderberg
Copy link
Contributor Author

I think there was some discussion of using xdist, and it has been part of the docker image since #6736. If I recall correctly, the main limitations are the lower resource CI machines, and false positives from multiple simultaneous tests that each use lots of GPU memory.

@driazati
Copy link
Member

we want to add it since we have a lot of CPU headroom we're not using for tests in most cases, but there are a bunch of pitfalls like this waiting around that someone has to wade through and we haven't gotten around to it yet

pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022
Some extensions to run tests in parallel (e.g. `pytest-xdist`) require
that test collection be deterministic.  Using the random seed as the
test parameter instead of the random value makes the test collection
be deterministic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants