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

Run tests under the appropriate 'extra' dependencies #1517

Merged
merged 9 commits into from
Sep 14, 2021

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

.github/workflows/pr-gate.yml Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated
@@ -185,6 +185,29 @@ For executing this test, first make sure you are in the correct environment as d
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.

tests/README.md Outdated Show resolved Hide resolved
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