Skip to content

Conversation

ravi-mosaicml
Copy link
Contributor

@ravi-mosaicml ravi-mosaicml commented May 17, 2022

This PR refactors the algorithms and tests as will be required by AutoYAHP. It does not depend on AutoYAHP itself (a future PR will remove the underlying hparam classes).

  • Refactored algorithm tests to not depend on hparams
  • Reformatted the factorize and selective backprop docstrings so they would be correctly parsed by auto-yahp
  • Refactor algorithm_settings.py to not depend on hparams and to return a list of pytest.param objects for a pytest.mark.parametrize. This change makes it more re-usable since it now includes information about markers required for each algorithm.
  • Moved the TestTrainerAlgorithms into tests/algorithms/test_algorithms_train.py, since it tests the individual algorithms, not the trainer, and thus should live in tests/algorithms.
  • Add helper methods for scanning a module to discover subclass implementations, check that the registry contains an entry, and test that a class is constructible from yaml

- Treat all python warnings as errors in tests. Existing tests that were throwing warnings were either fixed (yay!) or modified with `@pytest.mark.filterwarnings`
- In BlurPool, throwing an error if both `replace_maxpools` and `replace_convs` are False, as that results in the algorithm being a no-op.
-  Made the default optimizer warning in the trainer less verbose.
- Converted the bert yaml to have duration specified in terms of samples, to fix an issue where the warmup period, combine with max duration, was a noop.
- Moved `TestMetricSetter` to `tests/common` and renamed as `MetricSetterCallback`. Tests should not be importing from other test files (tests/common is OK)
- Removed TestWandBLogger, since the trainer tests do the same thing
Fix progressive resizing
This PR refactors the algorithms and tests as will be required by AutoYAHP. It does not depend on AutoYAHP itself (a future PR will remove the underlying hparam classes).

- Refactored algorithm tests to not depend on hparams
- Reformatted the factorize and selective backprop docstrings so they would be correctly parsed by auto-yahp
- Refactor `algorithm_settings.py` to not depend on hparams and to return a list of `pytest.param` objects for a `pytest.mark.parametrize`. This change makes it more re-usable since it now includes information about markers required for each algorithm.
- Moved the `TestTrainerAlgorithms` into `tests/algorithms/test_algorithms_train.py`, since it tests the individual algorithms, not the trainer, and thus should live in `tests/algorithms`.
- Add helper methods for scanning a module to discover subclass implementations, check that the registry contains an entry, and test that a class is constructable from yaml
@ravi-mosaicml ravi-mosaicml changed the title Remove hparams from tests Refactor Algorithms for AutoYAHP May 17, 2022
@ravi-mosaicml ravi-mosaicml changed the title Refactor Algorithms for AutoYAHP AutoYAHP Part 1: Cleanup the Algorithms May 20, 2022
@ravi-mosaicml ravi-mosaicml marked this pull request as ready for review May 20, 2022 06:39
@ravi-mosaicml ravi-mosaicml changed the title AutoYAHP Part 1: Cleanup the Algorithms AutoYAHP Part 1: Cleanup the Algorithms for AutoYAHP May 20, 2022
Copy link
Contributor

@hanlint hanlint left a comment

Choose a reason for hiding this comment

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

LGTM, one comment on the algorithm_settings pattern, but that is not blocking and could be cleaned up later.

@ravi-mosaicml ravi-mosaicml enabled auto-merge (squash) May 25, 2022 01:08
@ravi-mosaicml ravi-mosaicml requested a review from a team as a code owner May 25, 2022 04:30
@ravi-mosaicml ravi-mosaicml disabled auto-merge May 25, 2022 05:03
@ravi-mosaicml ravi-mosaicml merged commit f1862b8 into mosaicml:dev May 25, 2022
ravi-mosaicml added a commit that referenced this pull request May 26, 2022
Similar to #1056, this PR cleans up the callbacks and loggers for AutoYAHP. It does not depend on AutoYAHP itself (a future PR will remove the underlying hparam classes).

- Refactored callback and logger tests to not depend on hparams
- Reformatted the docstrings so they would be correctly parsed by auto-yahp
- Added `callback_settings.py`, similar to `algorithm_settings.py`, to return a list of pytest param objects for parameterization across callback tests. These param objects include appropriate markers (e.g. conditional skipping for wandb and mlperf; requiring that the memory monitor runs on GPU, ...)
- Moved the `TestTrainerAssets` into `tests/callbacks/test_callbacks.py`, since it tests the individual callbacks and loggers, not the trainer, and thus should live in `tests/callbacks`.
- Cleaned up the `MemoryMonitor` warnings when cuda is not available. Now, it warns when the model is not on cuda, to catch the edge case where one does CPU training when GPUs are available.
@ravi-mosaicml ravi-mosaicml deleted the remove_hparams_from_tests branch July 13, 2022 15:14
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.

2 participants