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

Marking fast tests #92

Closed
michalk8 opened this issue Jun 30, 2022 · 10 comments · Fixed by #101
Closed

Marking fast tests #92

michalk8 opened this issue Jun 30, 2022 · 10 comments · Fixed by #101
Assignees
Labels
enhancement New feature or request

Comments

@michalk8
Copy link
Collaborator

Currently, running all tests takes about 1h, we should mark some of them as fast and add a new CI job for them.

@michalk8 michalk8 self-assigned this Jun 30, 2022
@marcocuturi
Copy link
Contributor

that works, in effect this would make it easier to flag issues in PRs earlier? the fast tests would be targeting more fundamental forward aspects (e.g. Sinkhorn, SinkhornLR) and less so backward?

@michalk8 michalk8 added enhancement New feature or request labels Jun 30, 2022
@michalk8
Copy link
Collaborator Author

that works, in effect this would make it easier to flag issues in PRs earlier? the fast tests would be targeting more fundamental forward aspects (e.g. Sinkhorn, SinkhornLR) and less so backward?

Yes on both points.

@marcocuturi
Copy link
Contributor

I think this is a very good idea. Do you envision this as success on fast tests triggers slower tests? or both in parallel?

@michalk8
Copy link
Collaborator Author

Do you envision this as success on fast tests triggers slower tests? or both in parallel?

Actually no, would create 1 new CI job for only fast tests that runs in parallel with job with all tests.

@marcocuturi
Copy link
Contributor

LGTM!

@michalk8
Copy link
Collaborator Author

michalk8 commented Jul 4, 2022

Run this locally, am attaching a file which contains the runtime of all test. Will define a fast subset of them that covers the most of the functionality + see if we can further reduce the runtime of existing ones.
tests.txt

@michalk8
Copy link
Collaborator Author

michalk8 commented Jul 4, 2022

@LaetitiaPapaxanthos @marcocuturi I wasn't able to find a way to do this without getting rid of absl-py altogether and going for pytest based solution. If there aren't any objections, I'd switch to it, compatibility-wise, there isn't any different to how the tests are run from command line.

This boils down to:

  • changing Barynceter(paramemtrized.TestCase) -> TestBarycenter
  • for fast tests, using my custom mark (see below)
  • changing absl-py's asserts to normal ones
# new with
@pytest.mark.fast.with_args(
    lse_mode=[False, True],
    epsilon=[1e-1, 5e-1],
    jit=[False, True],
    only_fast={  # selects only this test when `-m fast` is passed
        "lse_mode": True,
        "epsilon": 1e-1,
        "jit": False
    }
)
def test_bures_barycenter(self, lse_mode, epsilon, jit):
  pass

# old
@parameterized.product(
    rank=[-1, 6],
    epsilon=[1e-1, 1e-2],
    jit=[True, False],
    init_random=[True, False]
)
def test_bures_barycenter(self, lse_mode, epsilon, jit):
  pass

In the future, I'd also refactor the setup/teardown class methods (see here) in favor of fixtures, but not necessary right now.

@JTT94
Copy link
Collaborator

JTT94 commented Jul 5, 2022

Can speed up tests in parallel
pip install pytest-xdist
pytest -n <num cpus> tests

@LaetitiaPapaxanthos
Copy link
Contributor

It is fine with me. If there is no need to select parameters of interest for the fast tests, I am wondering if it is possible to use pytest only as a decorator to mark the fast tests. With the selection of parameters, I don't know if there is a better solution indeed.

@michalk8
Copy link
Collaborator Author

michalk8 commented Jul 6, 2022

Can speed up tests in parallel

Think we had an issue with this in the past, but will try again, though there are only 2 cores for Linux jobs on GA.

If there is no need to select parameters of interest for the fast tests, I am wondering if it is possible to use pytest only as a decorator to mark the fast tests.

Possible, but they have to be pulled out the test class completely, because the fixture is not recognized at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants