Add args for black/isort/flake in the Makefile

[eldarkurtic]

Is your feature request related to a problem? Please describe. It could be useful to have an option to specify some arguments to black/isort/flake8 when running make quality and make style. For example --diff to check which changes are going to be applied to files.

Describe the solution you'd like I guess one solution could be to add $(BLACK_ARGS), $(ISORT_ARGS) and $(FLAKE8_ARGS) when calling them in the Makefile. Then one could run make style BLACK_ARGS="--diff".

Note: given that they have some common flags (for example --diff), there could be an additional $(COMMON_ARGS) to avoid having to specify the same arg for all of them separately.

Let me know what you think. If some of these seem reasonable, I can send a PR for it.

[markurtz]

Hey @eldarkurtic, thanks for the suggestion. I'd be worried about supplying args for anything related to the automated syntax correction. By leaving args out, we can ideally guarantee that what users run locally matches what GitHub actions is running as well.

For the use case given, make quality would ideally achieves this and print out what the changes would be. Is there anything that quality isn't properly surfacing right now?

[eldarkurtic]

@markurtz good point about GitHub actions, haven't thought about that one. Then probably better to leave this as it is, and let the users modify their own makefiles if they need it.

On a side note, make quality would print something like this:

Running copyright checks
python utils/copyright.py quality 'integrations/**/*.py' 'src/**/*.py' 'tests/**/*.py' 'utils/**/*.py' setup.py 'docs/**/*.md' 'docs/**/*.rst' 'integrations/**/*.md' CODE_OF_CONDUCT.md CONTRIBUTING.md DEVELOPING.md README.md
606 files have copyrights
Running python quality checks
black --check integrations src tests utils setup.py;
would reformat src/sparseml/pytorch/sparsification/pruning/modifier_pruning_mfac.py
Oh no! 💥 💔 💥
1 file would be reformatted, 484 files would be left unchanged.
make: *** [Makefile:50: quality] Error 1

so it's not really clear what would be the change nor where in the file it's located. Whereas by providing the --diff flag to the black command (others have this flag too), the output is a bit more informative:

Running copyright checks
python utils/copyright.py quality 'integrations/**/*.py' 'src/**/*.py' 'tests/**/*.py' 'utils/**/*.py' setup.py 'docs/**/*.md' 'docs/**/*.rst' 'integrations/**/*.md' CODE_OF_CONDUCT.md CONTRIBUTING.md DEVELOPING.md README.md
606 files have copyrights
Running python quality checks
black --check integrations src tests utils setup.py --diff;
--- src/sparseml/pytorch/sparsification/pruning/modifier_pruning_mfac.py	2022-05-10 15:27:52.141757 +0000
+++ src/sparseml/pytorch/sparsification/pruning/modifier_pruning_mfac.py	2022-05-10 15:34:20.252849 +0000
@@ -329,11 +329,13 @@
         if not isinstance(grad_sampler, GradSampler):
             raise ValueError(
                 "One-shot MFAC pruning requires a GradSampler object given by the "
                 f"grad_sampler kwarg. Given an object of type {type(grad_sampler)}"
             )
-        num_grads = _get_num_grads_for_sparsity( self._num_grads, self._applied_sparsity or 0.0)
+        num_grads = _get_num_grads_for_sparsity(
+            self._num_grads, self._applied_sparsity or 0.0
+        )

         is_training = module.training
         _LOGGER.debug("Setting the model in the eval mode")
         module.eval()

would reformat src/sparseml/pytorch/sparsification/pruning/modifier_pruning_mfac.py
Oh no! 💥 💔 💥
1 file would be reformatted, 484 files would be left unchanged.
make: *** [Makefile:50: quality] Error 1

i.e. it's clear what changes will happen in the code.