Fix Python formatting #3962

klecki · 2022-06-06T16:55:54Z

Category: Other

Description:

Conformance to pep8 was achieved by running
yapf and adjusting by hand the rest of issues
reported by flake8.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: N/A

klecki · 2022-06-06T16:56:15Z

!build

dali-automaton · 2022-06-06T17:00:08Z

CI MESSAGE: [5025731]: BUILD STARTED

dali-automaton · 2022-06-06T18:06:59Z

CI MESSAGE: [5025731]: BUILD FAILED

szalpal

Please consider wrapping the function parameters differently. I've posted handful of suggestions, at some point I stopped posting them though ;)
What I'm referring to is such example:

class PreemphasisPythonPipeline(Pipeline):
    def __init__(self,
                 device,
                 batch_size,
                 iterator,
                 border='clamp',
                 preemph_coeff=0.97,
                 per_sample_coeff=False,
                 num_threads=4,
                 device_id=0):
        super(PreemphasisPythonPipeline, self).__init__(batch_size,
                                                        num_threads,
                                                        device_id,
                                                        seed=SEED,
                                                        exec_async=False,
                                                        exec_pipelined=False)

->

class PreemphasisPythonPipeline(Pipeline):
    def __init__(self, device, batch_size, iterator, border='clamp', preemph_coeff=0.97,
                 per_sample_coeff=False, num_threads=4, device_id=0):
        super().__init__(batch_size, num_threads, device_id, seed=SEED, exec_async=False,
                         exec_pipelined=False)

or

def check_transform_shear_op(shear=None,
                             angles=None,
                             center=None,
                             has_input=False,
                             reverse_order=False,
                             batch_size=1,
                             num_threads=4,
                             device_id=0):

->

def check_transform_shear_op(shear=None, angles=None, center=None, has_input=False,
                             reverse_order=False, batch_size=1, num_threads=4, device_id=0):

Anyway, that's your call. I don't see any blockers. I've put some suggestions, feel free to reject them, if you feel these aren't necessary.

szalpal · 2022-06-06T21:12:29Z

dali/test/python/test_operator_preemph.py

+    def __init__(self,
+                 device,
+                 batch_size,
+                 iterator,
+                 border='clamp',
+                 preemph_coeff=0.97,
+                 per_sample_coeff=False,
+                 num_threads=4,
+                 device_id=0):
+        super(PreemphasisPythonPipeline, self).__init__(batch_size,
+                                                        num_threads,
+                                                        device_id,
+                                                        seed=SEED,
+                                                        exec_async=False,
+                                                        exec_pipelined=False)


How about?

class PreemphasisPythonPipeline(Pipeline): def __init__(self, device, batch_size, iterator, border='clamp', preemph_coeff=0.97, per_sample_coeff=False, num_threads=4, device_id=0): super().__init__(batch_size, num_threads, device_id, seed=SEED, exec_async=False, exec_pipelined=False)

Turned a knob in yapf so the formatting is more condensed. I also reverted the changes where not needed as apparently we don't want to have that.

szalpal · 2022-06-06T21:14:27Z

dali/test/python/test_operator_roi_random_crop.py

+
+        def shape_gen_fn():
+            return random_shape(in_shape_min, in_shape_max, ndim)
+
+        def data_gen_f():
+            return batch_gen(max_batch_size, shape_gen_fn)
+


In the named lambdas case (we have handful of that) I like what @jantonguirao proposed:

def shape_gen_fn(): return random_shape(in_shape_min, in_shape_max, ndim) def data_gen_f(): return batch_gen(max_batch_size, shape_gen_fn)

I would be in favor of keeping regular function line breaks. We are just defining them, not passing something inline

dali/test/python/test_operator_rotate.py

szalpal · 2022-06-07T07:31:55Z

dali/test/python/test_operator_sequence_rearrange.py

+            yield raises(
+                RuntimeError,
+                glob=error_msg)(check_fail_sequence_rearrange), 2, shape, new_order, per_sample, dev


Leaving aside, that yield raises(...)(...) looks really peculiar, how about this kind of formatting?

yield raises(RuntimeError, glob=error_msg)(check_fail_sequence_rearrange), \ 2, shape, new_order, per_sample, dev

I made it consistent with the one below that is so long, that it had to be broken similar way.

szalpal · 2022-06-07T07:49:35Z

dali/test/python/test_operator_arithmetic_ops.py

+        np.testing.assert_array_equal(out,
+                                      op(l_np, r_np),
+                                      err_msg="{} op\n{} =\n{}".format(l_np, r_np, out))


How about using modern f notation? This way this can be squashed into one line:

Suggested change

np.testing.assert_array_equal(out,

op(l_np, r_np),

err_msg="{} op\n{} =\n{}".format(l_np, r_np, out))

np.testing.assert_array_equal(out, op(l_np, r_np), err_msg=f"{l_np} op\n{r_np} =\n{out}")

szalpal · 2022-06-07T07:51:53Z

dali/test/python/test_operator_arithmetic_ops.py

+    pipe = ExprOpPipeline(kinds,
+                          types,
+                          iterator, (lambda x, y: x // y),
+                          batch_size=batch_size,
+                          num_threads=2,
+                          device_id=0)


I believe this looks odd. I'm OK with putting every argument in a separate line, but if so, I'd vote for not putting two args in one line by exception, i.e.

pipe = ExprOpPipeline(kinds, types, iterator, (lambda x, y: x // y), <--- here batch_size=batch_size, num_threads=2, device_id=0)

Hmm, I will try some knobs to see why it happened.

szalpal · 2022-06-07T07:53:25Z

dali/test/python/test_operator_arithmetic_ops.py

 def check_raises_te(kinds, types, op, shape, _):
    check_raises(kinds, types, op, shape)


 # Arithmetic operations between booleans that are not allowed
 bool_disallowed = [((lambda x, y: x + y), "+"), ((lambda x, y: x - y), "-"),
                   ((lambda x, y: x / y), "/"), ((lambda x, y: x / y), "//"),
-                   ((lambda x, y: x ** y), "**")]
+                   ((lambda x, y: x**y), "**")]


TIL that pow operator formatting in Python w.r.t. whitespace is inconsistent with other ops :D

dali/test/python/test_operator_multipaste.py

szalpal · 2022-06-07T08:00:26Z

dali/test/python/test_operator_readers_webdataset_requirements.py

+import os
+from glob import glob
+import math
+import dali


Suggested change

import dali

import nvidia.dali as dali

mzient · 2022-06-07T14:51:17Z

dali/test/python/test_external_source_parallel_large_sample.py

+                assert val == expected_val, (
+                    f"Unexpected value in batch: got {val}, expected {expected_val}, "
+                    f"for batch {batch_idx}, sample {idx_in_batch}")


Is this parenthesis required for anything?

Suggested change

assert val == expected_val, (

f"Unexpected value in batch: got {val}, expected {expected_val}, "

f"for batch {batch_idx}, sample {idx_in_batch}")

assert val == expected_val, \

f"Unexpected value in batch: got {val}, expected {expected_val}, " \

f"for batch {batch_idx}, sample {idx_in_batch}"

To not use the \ I'd say it's preferred.

PEP8 says so, I guess, but it still looks no less weird than a backslash (and perhaps more so).

dali/test/python/test_operator_affine_transforms.py

mzient · 2022-06-07T15:17:03Z

dali/test/python/test_operator_arithmetic_ops.py

+integer_types = [
+    np.bool_, np.int8, np.int16, np.int32, np.int64, np.uint8, np.uint16, np.uint32, np.uint64
+]


and the nice grouping went to Hell... - I think the original formatting was actually correct under PEP8?

mzient · 2022-06-07T15:19:20Z

dali/test/python/test_operator_arithmetic_ops.py

    pipe = ExprOpPipeline(kind, type, iterator, op, batch_size=batch_size, num_threads=2,
-            device_id=0)
+                          device_id=0)


Suggested change

pipe = ExprOpPipeline(kind, type, iterator, op, batch_size=batch_size, num_threads=2,

device_id=0)

device_id=0)

pipe = ExprOpPipeline(kind, type, iterator, op,

batch_size=batch_size, num_threads=2, device_id=0)

I'd like this file compatible with full auto-format and this doesn't work.

Suggested change

pipe = ExprOpPipeline(kind, type, iterator, op, batch_size=batch_size, num_threads=2,

device_id=0)

device_id=0)

pipe = ExprOpPipeline(

kind, type, iterator, op, batch_size=batch_size, num_threads=2, device_id=0)

Out of curiosity: would this work?

klecki · 2022-06-07T15:20:29Z

!build

mzient · 2022-06-07T15:20:36Z

dali/test/python/test_operator_arithmetic_ops.py

            for types_in in [(np.int32, np.int32, np.int32), (np.float32, np.int32, np.int32),
-                             (np.uint8, np.float32, np.float32), (np.int32, np.float32, np.float32)]:
+                             (np.uint8, np.float32, np.float32),
+                             (np.int32, np.float32, np.float32)]:


Suggested change

for types_in in [(np.int32, np.int32, np.int32), (np.float32, np.int32, np.int32),

(np.uint8, np.float32, np.float32), (np.int32, np.float32, np.float32)]:

(np.uint8, np.float32, np.float32),

(np.int32, np.float32, np.float32)]:

for types_in in [(np.int32, np.int32, np.int32),

(np.float32, np.int32, np.int32),

(np.uint8, np.float32, np.float32),

(np.int32, np.float32, np.float32)]:

mzient · 2022-06-07T15:21:02Z

dali/test/python/test_operator_arithmetic_ops.py

    pipe = ExprOpPipeline(kinds, types, iterator, op, batch_size=batch_size, num_threads=2,
-            device_id=0)
+                          device_id=0)


Suggested change

pipe = ExprOpPipeline(kinds, types, iterator, op, batch_size=batch_size, num_threads=2,

device_id=0)

device_id=0)

pipe = ExprOpPipeline(kinds, types, iterator, op,

batch_size=batch_size, num_threads=2, device_id=0)

mzient · 2022-06-07T15:21:27Z

dali/test/python/test_operator_arithmetic_ops.py

+    pipe = ExprOpPipeline(kinds, types, iterator, dali_op, batch_size=batch_size, num_threads=2,
+                          device_id=0)


Suggested change

pipe = ExprOpPipeline(kinds, types, iterator, dali_op, batch_size=batch_size, num_threads=2,

device_id=0)

pipe = ExprOpPipeline(kinds, types, iterator, dali_op,

batch_size=batch_size, num_threads=2, device_id=0)

mzient · 2022-06-07T15:21:49Z

dali/test/python/test_operator_arithmetic_ops.py

+    pipe = ExprOpPipeline(kinds, types, iterator, (lambda x, y: x // y), batch_size=batch_size,
+                          num_threads=2, device_id=0)


Suggested change

pipe = ExprOpPipeline(kinds, types, iterator, (lambda x, y: x // y), batch_size=batch_size,

num_threads=2, device_id=0)

pipe = ExprOpPipeline(kinds, types, iterator, (lambda x, y: x // y),

batch_size=batch_size, num_threads=2, device_id=0)

mzient · 2022-06-07T15:23:02Z

dali/test/python/test_operator_arithmetic_ops.py

+            yield check_raises_re, kinds, (np.bool_, np.bool_,
+                                           np.bool_), op, shape_small, op_desc, error_msg


Please.... this is just a coincidence - don't abuse it.

Suggested change

yield check_raises_re, kinds, (np.bool_, np.bool_,

np.bool_), op, shape_small, op_desc, error_msg

yield check_raises_re, kinds, (np.bool_, np.bool_, np.bool_), \

op, shape_small, op_desc, error_msg

or

Suggested change

yield check_raises_re, kinds, (np.bool_, np.bool_,

np.bool_), op, shape_small, op_desc, error_msg

yield (check_raises_re, kinds, (np.bool_, np.bool_, np.bool_),

op, shape_small, op_desc, error_msg)

dali-automaton · 2022-06-07T15:25:02Z

CI MESSAGE: [5034722]: BUILD STARTED

dali/test/python/test_operator_brightness_contrast.py

dali/test/python/test_operator_multipaste.py

mzient · 2022-06-07T15:43:20Z

dali/test/python/test_operator_readers_webdataset_requirements.py

-            ),
-        )
-    )
+            lambda s: int(s[s.rfind("/") + 1:s.rfind(".")]) < 2500,


This is less readable; the colon disappeared.

Suggested change

lambda s: int(s[s.rfind("/") + 1:s.rfind(".")]) < 2500,

lambda s: int(s[s.rfind("/") + 1 : s.rfind(".")]) < 2500,

According to PEP8, the colon in array slice can have spaces on both sides or none.

According to my flake8 it's not allowed, just tried it, reverting.

It's a known bug in flake8.... known since 2015.... to make it even "funnier", black adds those spaces and doesn't have an option not to 🙃
I think we should disable E203 entirely in flake8 - can we do it?

Related issues:
PyCQA/flake8#257
PyCQA/pycodestyle#373

Dunno. Sadly, it's not possible to use black with our codebase, so this is what we get with flake8 and other formatters, which is still pep8 compliant (as it has uniform spacing).

Then:

Suggested change

lambda s: int(s[s.rfind("/") + 1:s.rfind(".")]) < 2500,

lambda s: int(s[s.rfind("/") + 1 : s.rfind(".")]) < 2500, # noqa: 203

?

I don't know if that's much better with the noqa, but done.

dali/test/python/test_operator_readers_webdataset_requirements.py

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Co-authored-by: Michał Zientkiewicz <mzient@gmail.com>

This reverts commit a6fda0d.

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2022-06-08T17:24:54Z

!build

dali-automaton · 2022-06-08T17:29:55Z

CI MESSAGE: [5045456]: BUILD FAILED

klecki · 2022-06-08T17:51:07Z

!build

dali-automaton · 2022-06-08T17:55:01Z

CI MESSAGE: [5045665]: BUILD STARTED

dali-automaton · 2022-06-08T19:03:08Z

CI MESSAGE: [5045665]: BUILD FAILED

dali-automaton · 2022-06-13T11:06:44Z

CI MESSAGE: [5045665]: BUILD PASSED

klecki assigned szalpal Jun 6, 2022

szalpal approved these changes Jun 7, 2022

View reviewed changes

jantonguirao assigned mzient and banasraf Jun 7, 2022

klecki unassigned mzient Jun 7, 2022