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

Tests runnable on default CI agent #1507

Closed
laserprec opened this issue Aug 23, 2021 · 3 comments · Fixed by #1538
Closed

Tests runnable on default CI agent #1507

laserprec opened this issue Aug 23, 2021 · 3 comments · Fixed by #1538
Assignees
Labels
enhancement New feature or request

Comments

@laserprec
Copy link
Contributor

laserprec commented Aug 23, 2021

Description

At the moment, some of our unit & integration test suites are not fit to run on the default CI vms which has relatively small memory (7GB). These are usually tests that run our example notebook end-to-end with a large dataset (ranging from 100k to 20million rows). As a result, we can only run them on self-hosted VM with larger memory. Does the team think that there is a need to redesign these tests/notebooks so they runnable on GitHub-hosted VMs? @miguelgfierro, @gramhagen?

Link to integration test failures on Github-hosted VM
Link to unit test failures on Github-hosted VM

I think there are some solutions we can go after, besides optimizing the content in the notebook:

  1. Incomplete training using a smaller dataset, and relax the assertion checks.
    • assert results["rmse"] == pytest.approx(0.8621, rel=TOL, abs=ABS_TOL)
      for example, instead of checking if rsme is within some error range within a target value, we can check that our notebook runs successfully with a smaller dataset, and rsme "improves" or is not none.
  2. Avoid running the full notebook multiple times for different data-size.

Expected behavior with the suggested feature

Tests should meet the resource constrains in a Standard_DS2_v2 SKU so they are runnable in such machine.

This has the following benefits:

  1. CI is less dependent on self-hosted resource so we can scale up better when we support more python versions
  2. Reduce build time significantly
  3. Faster iteration

The devil's advocate:

  1. Do we care how long our nightly build would take to run?
  2. Are we comfortable with relaxing some of the assertions in the tests and risk notebook failing for users?
  3. Is it even possible to run some of the notebooks e2e under 7GB memory (minus whatever it is needed to keep the OS running)

Other Comments

@laserprec laserprec added the enhancement New feature or request label Aug 23, 2021
@laserprec laserprec mentioned this issue Aug 23, 2021
4 tasks
@miguelgfierro
Copy link
Collaborator

The idea of having the current test configuration comes from a publication of Lenskit from University of Minnesota https://buildingrecommenders.wordpress.com/2016/02/04/testing-recommenders/ and it is explained here: https://miguelgfierro.com/blog/2018/a-beginners-guide-to-python-testing/

This method is a more complete way of testing ML pipelines than just checking that the code works, we also want to make sure that the ML models provide metrics that are reasonable. That's why we wanted to use a relatively big dataset in the tests.

I hope this clarifies

@gramhagen
Copy link
Collaborator

the test configuration is very thorough, but i think we can restrict the smoke and integration tests to running less frequently (nightly, weekly?).

also, the notebook tests are not really unit tests since they involve multiple components interacting with each other.
i think we should be aiming for unit tests that exercise individual components and don't use external data this will make them very fast and less likely to fail if a website goes down. all the other types of tests can be moved to smoke/integration and run nightly.

@miguelgfierro
Copy link
Collaborator

@gramhagen both ideas are really good, would you like them to discuss them in the weekly meeting?

@laserprec laserprec self-assigned this Aug 26, 2021
@laserprec laserprec linked a pull request Sep 24, 2021 that will close this issue
4 tasks
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.

3 participants