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

[Testing] Implementation of an updated testing framework #2187

Merged
merged 18 commits into from
Mar 29, 2024

Conversation

dsikka
Copy link
Contributor

@dsikka dsikka commented Mar 19, 2024

Summary:

  1. Update an existing unit test and an existing integration test, both for export such that they are using the unittest.TestCase base class and parameterized module
  2. The integration tests are also updated to use config files to define the different combination of inputs that are meant to be tested for a particular integration test. Each config must include a cadence field, which dictates how frequently the test is run (set through an environment variables) and the test_type (sanity, regression, or smoke) which for now is a config variable but we can easily swap to just use decorators, as being done with the sample smoke test added
  3. Introduce a base CustomIntegrationTest class which other integration tests can inherit and use to test custom scripts and custom test classes.
  4. Add examples of decorators to mark unit , custom, and integration tests. Also introduce decorators to mark tests with certain package or compute environments requirements (e.g requires_torch, requires_gpu). These are described in a new testing_utils.py file

For a summary of the changes, see tests/docs/testing_framework.md

Testing

  • An example of a class which uses the CustomIntegrationTest class is the TestGenerationExportIntegrationCustom and it can be found in test_generation_export.py. This test class was tested using sample config/python files added under tests/sparseml/export/transformers/generation_configs
  • An existing integration test was updated to use config files (as mentioned above) and was tested using the newly added
    tests/sparseml/export/transformers/generation_configs/tiny_stories.yaml

Using unittest

  • The existing tests have been updated to use unittest package. We can definitely keep pytest for the existing tests if there are strong feelings although I do find it easier to read and understand and we can easily use the package parameterized to get the same functionality (or even expanded functionality for classes, compared to pytest fixtures).
  • Custom testing should keep the unittest framework as it makes testing custom scripts/tests fairly easy/straightforward

tests/custom_test.py Show resolved Hide resolved
tests/custom_test.py Outdated Show resolved Hide resolved
tests/data.py Outdated Show resolved Hide resolved
@@ -0,0 +1,97 @@
# An Updated Testing Framework
Copy link
Contributor

Choose a reason for hiding this comment

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

is this meant to be a migration doc, or long standing demonstrating the new framework? if the latter probably don't need as much framing around what changed

Copy link
Contributor Author

@dsikka dsikka Mar 26, 2024

Choose a reason for hiding this comment

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

More so a summary of the new framework and what we want to establish going forward in terms of testing structure. Maybe makes more sense in internal docs as the target are MLE?

from tests.testing_utils import parse_params


CONFIGS_DIRECTORY = "tests/sparseml/export/transformers/generation_configs"
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason we wouldn't want to use relative path here? hardcoding this would require us to always run from sparseml/ not that this is not always the case though...

Copy link
Contributor Author

@dsikka dsikka Mar 26, 2024

Choose a reason for hiding this comment

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

yah I kept the path from sparseml as that is the directory used to run all gha tests

@dsikka dsikka requested a review from bfineran March 26, 2024 19:38
tests/testing_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@rahul-tuli rahul-tuli left a comment

Choose a reason for hiding this comment

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

I'm cool with using a mix of unit test and pytest, specially cause pytest can pick up unittest style tests. Good work on the documentation, really appreciate it.

tests/docs/testing_framework.md Outdated Show resolved Hide resolved
rahul-tuli
rahul-tuli previously approved these changes Mar 27, 2024
@bfineran bfineran merged commit 8693b27 into main Mar 29, 2024
9 of 15 checks passed
@bfineran bfineran deleted the sparseml_testing branch March 29, 2024 14:35
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.

4 participants