Skip to content

Run tests under the appropriate 'extra' dependencies#1517

Merged
laserprec merged 9 commits intostagingfrom
laserprec/run-test-under-dep-subset
Sep 14, 2021
Merged

Run tests under the appropriate 'extra' dependencies#1517
laserprec merged 9 commits intostagingfrom
laserprec/run-test-under-dep-subset

Conversation

@laserprec
Copy link
Contributor

@laserprec laserprec commented Sep 1, 2021

Description

To improve developer experience working with this repo, we will begin migrating our DevOps pipelines into Github Action, leveraging popular DevOps tools like tox and flake8. This will be a sequence of updates to our DevOps infrastructures:

  1. Propose and draft the CI pipeline in Github Action
  2. Setup self-hosted machines to run our GPU workloads
  3. Create feature parity with the existing CI pipelines (pr-gates & nightly-build)
  4. Run tests on the appropriate dependency subsets <---- (Current PR)
  5. Optimize build time for unit tests
  6. Enforce flake8 (coding style checks) on the build and clean up coding styles to pass flake8 checks
  7. Deprecate CI pipelines in ADO and switch to Github Actions

In this PR:

Problem

Our recommender package has several extra dependencies to address the different compute/development environments: recommender[spark,gpu,example,dev]. We should run our tests against the specific extra dependency (currently, we always install recommenders[all] to run any ANY test markers)

For example:

A user pip install recommenders[gpu,examples,dev] should be able to run recommender utilities tested by pytest -m "gpu and notebooks and not spark

Solution

New tox-env is introduced to the pipeline. One can map the extra dependencies with the tests type running the following command:

tox -e {TOX_ENV} -- {PYTEST_PARAM}

where

- `TOX_ENV` can be `cpu|gpu|spark|all`, each env maps to the "extra" dependency, for example recommenders[gpu], and recommenders[spark].
- `PYTEST_PARAM` are any standard parameters to supply to `pytest` cli executing particular tests.

For example:

  1. tox -e cpu -- tests/unit -m "not notebook and not spark and not gpu (runs the unit tests without extra dependency)
  2. tox -e gpu -- tests/unit -m "gpu and notebook" (runs the gpu notebook tests)
  3. tox -e spark -- tests/unit -m "spark and notebook" (runs the spark notebook tests)
  4. tox -e all -- tests/unit (to run all of the unit tests)

For more details, please see the changes in tox.ini.

Related Issues

#1517

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • This PR is being made to staging branch and not to main branch.

@laserprec laserprec changed the title Laserprec/run test under dep subset Laserprec/run-test-under-dep-subset Sep 1, 2021
@laserprec laserprec changed the title Laserprec/run-test-under-dep-subset Run tests under the appropriate 'extra' dependencies Sep 1, 2021
@laserprec laserprec self-assigned this Sep 1, 2021
@laserprec laserprec linked an issue Sep 1, 2021 that may be closed by this pull request
Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

This is really good

tests/README.md Outdated
pytest tests/unit/test_notebooks_python.py::test_sar_single_node_runs
```

### Test execution with tox
Copy link
Collaborator

Choose a reason for hiding this comment

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

One question, when we deprecate the ADO pipeline, are we going to use tox as the main test tool instead of pytest?
If that's the case, above in the text we have content showing the behavior with pytest:
pytest tests/smoke -m "smoke and not spark and gpu" --durations 0 will we still need this?

Copy link
Contributor Author

@laserprec laserprec Sep 2, 2021

Choose a reason for hiding this comment

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

I was hoping people can use tox as another option to run tests (especially when they need to debug a build issue in the pipeline), I want to keep the option here for people to use either pytest or tox. I myself runs pytest most of the time due to habit 😄. So I think we can keep these nice documentations on pytest.

Copy link
Collaborator

@loomlike loomlike left a comment

Choose a reason for hiding this comment

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

lgtm!

@codecov-commenter
Copy link

Codecov Report

Merging #1517 (09400ea) into staging (c5029af) will decrease coverage by 12.10%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           staging    #1517       +/-   ##
============================================
- Coverage    74.23%   62.12%   -12.11%     
============================================
  Files           84       84               
  Lines         8369     8397       +28     
============================================
- Hits          6213     5217      -996     
- Misses        2156     3180     +1024     
Impacted Files Coverage Δ
...ecommenders/models/newsrec/io/mind_all_iterator.py 12.21% <0.00%> (-86.65%) ⬇️
recommenders/models/newsrec/io/mind_iterator.py 15.67% <0.00%> (-82.71%) ⬇️
...ommenders/models/deeprec/io/sequential_iterator.py 15.85% <0.00%> (-81.94%) ⬇️
recommenders/models/newsrec/models/base_model.py 30.90% <0.00%> (-59.40%) ⬇️
...deeprec/models/sequential/sequential_base_model.py 46.97% <0.00%> (-47.66%) ⬇️
recommenders/models/geoimc/geoimc_data.py 41.66% <0.00%> (-44.80%) ⬇️
...enders/models/deeprec/io/dkn_item2item_iterator.py 45.61% <0.00%> (-42.11%) ⬇️
...menders/models/deeprec/models/graphrec/lightgcn.py 51.47% <0.00%> (-40.24%) ⬇️
recommenders/datasets/mind.py 0.00% <0.00%> (-30.85%) ⬇️
recommenders/models/newsrec/newsrec_utils.py 70.00% <0.00%> (-13.75%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5029af...09400ea. Read the comment docs.

@laserprec laserprec merged commit 3387aa4 into staging Sep 14, 2021
@laserprec laserprec deleted the laserprec/run-test-under-dep-subset branch September 14, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run tests in the appropriate extra dependencies.

5 participants