RfC: Enforce consistent formatting on CI #1957
Draft
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
romanc
force-pushed
the
romanc/linting
branch
2 times, most recently
from
1444d24 to
576fc1f
Compare
This PR is to support the discussion on enforcing consistent formatting on the CI [1]. [1] spcl#1804
romanc
force-pushed
the
romanc/linting
branch
from
576fc1f to
bc44c5e
Compare
romanc
commented
Feb 28, 2025
dace/frontend/python/newast.py
Outdated
Comment on lines
454
to
458
| ind_entry, ind_exit = graph.add_map('indirection', { | ||
| '__i%d' % i: '%s:%s+1:%s' % (s, e, t) | ||
| for i, (s, e, t) in enumerate(mapped_rng) | ||
| }, | ||
| debuginfo=pvisitor.current_lineinfo) |
There was a problem hiding this comment.
This is an example where we might want to put manual guards around the old version.
dace/frontend/python/replacements.py
Outdated
Comment on lines
1467
to
1465
| name, _ = sdfg.add_temp_transient(arr1.shape, restype, arr1.storage) | ||
| state.add_mapped_tasklet("_%s_" % opname, {'__i%d' % i: '0:%s' % s | ||
| for i, s in enumerate(arr1.shape)}, | ||
| {'__in1': Memlet.simple(op1, ','.join(['__i%d' % i for i in range(len(arr1.shape))]))}, | ||
| state.add_mapped_tasklet("_%s_" % opname, { | ||
| '__i%d' % i: '0:%s' % s | ||
| for i, s in enumerate(arr1.shape) | ||
| }, {'__in1': Memlet.simple(op1, ','.join(['__i%d' % i for i in range(len(arr1.shape))]))}, | ||
| '__out = %s __in1' % opcode, | ||
| {'__out': Memlet.simple(name, ','.join(['__i%d' % i for i in range(len(arr1.shape))]))}, | ||
| external_edges=True) |
There was a problem hiding this comment.
From what I see: adding mapped tasklets works best if the first argument is already on a new line (at least in the current configuration of yapf)
Comment on lines
17
to
30
| s[i, j, k] = ( | ||
| 0.5000 * r[2 * i, 2 * j, 2 * k] + | ||
| 0.2500 * (r[2 * i - 1, 2 * j, 2 * k] + r[2 * i + 1, 2 * j, 2 * k] + r[2 * i, 2 * j - 1, 2 * k] + | ||
| r[2 * i, 2 * j + 1, 2 * k] + r[2 * i, 2 * j, 2 * k - 1] + r[2 * i, 2 * j, 2 * k + 1]) + | ||
| 0.1250 * (r[2 * i - 1, 2 * j - 1, 2 * k] + r[2 * i - 1, 2 * j + 1, 2 * k] + | ||
| r[2 * i + 1, 2 * j - 1, 2 * k] + r[2 * i + 1, 2 * j + 1, 2 * k] + | ||
| r[2 * i - 1, 2 * j, 2 * k - 1] + r[2 * i - 1, 2 * j, 2 * k + 1] + | ||
| r[2 * i + 1, 2 * j, 2 * k - 1] + r[2 * i + 1, 2 * j, 2 * k + 1] + | ||
| r[2 * i, 2 * j - 1, 2 * k - 1] + r[2 * i, 2 * j - 1, 2 * k + 1] + | ||
| r[2 * i, 2 * j + 1, 2 * k - 1] + r[2 * i, 2 * j + 1, 2 * k + 1]) + | ||
| 0.0625 * (r[2 * i - 1, 2 * j - 1, 2 * k - 1] + r[2 * i - 1, 2 * j - 1, 2 * k + 1] + | ||
| r[2 * i - 1, 2 * j + 1, 2 * k - 1] + r[2 * i - 1, 2 * j + 1, 2 * k + 1] + | ||
| r[2 * i + 1, 2 * j - 1, 2 * k - 1] + r[2 * i + 1, 2 * j - 1, 2 * k + 1] + | ||
| r[2 * i + 1, 2 * j + 1, 2 * k - 1] + r[2 * i + 1, 2 * j + 1, 2 * k + 1])) |
There was a problem hiding this comment.
This one is a perfect example for manual guards
tests/schedule_tree/nesting_test.py
Outdated
Comment on lines
66
to
68
| assert [type(n) | ||
| for n in stree.preorder_traversal()][1:] == [tn.MapScope, tn.MapScope, tn.GeneralLoopScope, | ||
| tn.TaskletNode] |
There was a problem hiding this comment.
that might also warrant a manual guard
tests/sdfg/loop_region_test.py
Outdated
Comment on lines
252
to
255
| assert stree.as_string() == (f'{tn.INDENTATION}assign i = 0\n' + | ||
| f'{tn.INDENTATION}while (i < 10):\n' + | ||
| f'{2 * tn.INDENTATION}A[i] = tasklet()\n' + | ||
| f'{2 * tn.INDENTATION}assign i = (i + 1)') |
There was a problem hiding this comment.
More candidates for "assert guards" in this file
romanc
changed the title
romanc
force-pushed
the
romanc/linting
branch
from
bc44c5e to
4c3bc67
Compare
|
@romanc I am ok with this PR. Let's discuss it on Thursday to finalize the decision. |
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Description
Since there was no pushback on the discussion page1, let's go one step further and start discussing how this could look like in practice.
This PR contains two commits
To see the automated linting infrastructure in action, pull this branch, install the linting extras and run
Most changes you'll see will be whitespace changes. If you don't like the formatting,
yapfhas a couple of knobs we could use for fine-tuning. And there's always the option to manually put guards in select places where auto-formatting isn't good enough.Footnotes
https://github.com/spcl/dace/discussions/1804 ↩