Regression cli tests #772
Merged
Regression cli tests #772
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The documentation is not available anymore as the PR was closed or merged. |
muellerzr
commented
Oct 18, 2022
| @@ -489,7 +489,7 @@ def multi_gpu_launcher(args): | |||
|
|
|||
| current_env = os.environ.copy() | |||
| gpu_ids = getattr(args, "gpu_ids") | |||
| if gpu_ids != "all": | |||
| if gpu_ids != "all" and args.gpu_ids is not None: | |||
There was a problem hiding this comment.
This addition is needed for the test case of CUDA_VISIBLE_DEVICES=""
sgugger
approved these changes
Oct 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
accelerate launchtestsWhat does this add?
This PR introduces a subset of tests for checking
accelerate launchand in turnaccelerate config, and solves a bug I found during testingWhy is it needed?
Users have pointed out that we're not quite doing a good enough job with backwards compatibility checks on config yamls. To ensure these can be caught new tests are added to run a simple script using
accelerate launchwith no configuration file or specific configuration filesWhat parts of the API does this impact?
User-facing:
Nothing
Internal structure:
tests/test_configsshould contain (at least) the last 2 major versions of Accelerate plus the latest version, which are direct outputs fromaccelerate config. Since these tests take so fast to execute, I think it's fine if this just contains the last 5 or 6 configs until we are comfortable.Anticipated maintenance burden? (What will happen in say, 3 months if something changes)
As part of PR review, maintainers should be aware of any changes to the config file or CLI arguments and ensure that a new config is added to the folder for proper testing.