compiler: Misc compiler fixes and improvements -- part II #2138

FabioLuporini · 2023-05-31T09:02:58Z

No description provided.

review-notebook-app · 2023-05-31T09:03:03Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2023-05-31T09:10:45Z

Codecov Report

Merging #2138 (9636366) into master (0678627) will decrease coverage by 0.02%.
The diff coverage is 92.01%.

@@            Coverage Diff             @@
##           master    #2138      +/-   ##
==========================================
- Coverage   87.09%   87.08%   -0.02%     
==========================================
  Files         223      223              
  Lines       39689    39830     +141     
  Branches     5147     5168      +21     
==========================================
+ Hits        34567    34685     +118     
- Misses       4544     4567      +23     
  Partials      578      578

Impacted Files	Coverage Δ
devito/arch/compiler.py	`39.34% <0.00%> (ø)`
devito/ir/iet/algorithms.py	`83.87% <0.00%> (ø)`
tests/test_gpu_common.py	`1.46% <3.03%> (+0.08%)`	⬆️
devito/types/utils.py	`94.11% <50.00%> (-5.89%)`	⬇️
devito/ir/stree/algorithms.py	`90.68% <56.25%> (-2.57%)`	⬇️
devito/types/basic.py	`93.35% <80.00%> (-0.43%)`	⬇️
devito/mpi/routines.py	`93.18% <84.61%> (-0.52%)`	⬇️
devito/mpi/halo_scheme.py	`93.68% <88.88%> (-0.51%)`	⬇️
tests/test_pickle.py	`99.62% <99.61%> (+0.02%)`	⬆️
devito/ir/clusters/algorithms.py	`95.33% <100.00%> (+0.01%)`	⬆️
... and 13 more

... and 3 files with indirect coverage changes

georgebisbas

What was the problem with u.dx2? Is there an issue somewhere about this?

FabioLuporini · 2023-06-01T10:52:39Z

What was the problem with u.dx2? Is there an issue somewhere about this?

These were PRO issues, and all new tests therefore a in PRO

That said, I can assure you that there's no new feature here in OSS territory that hasn't been equipped with at least one test. Most changes here are compilation-related generalisations and improvements which are stress-tested by what de-facto are higher-order regression tests in PRO

mloubout · 2023-05-31T13:00:49Z

devito/ir/stree/algorithms.py

+            # it doesn't need to wrap any subtree. Thus, a WaitLock acts like
+            # a barrier to what follows inside `d`
+            NodeSync(syncs, tip)
+            return tip


nitpicking, Since this doesn't really return anything it'd me slightly more consistent to have

tip = NodeSync(syncs, tip) return

improved and fixed 👍

mloubout · 2023-06-05T12:37:00Z

devito/passes/iet/parpragma.py

@@ -527,7 +527,9 @@ def function(self):
    def imask(self):
        return self._imask

-    @cached_property
+    # TODO: cached_property here will break our pickling tests for reasons that


out of curiosity which test?

PRO tests landing with the fix-udx2-MPI branch in PRO. For future reference: they are test_layered_funcs.py::TestMisc::test_pickling and test_composite_funcs.py::TestMisc::test_pickling

devito/tools/abc.py

mloubout · 2023-06-05T12:39:11Z

devito/types/array.py

@@ -203,6 +211,13 @@ def _mem_shared(self):
    def _mem_constant(self):
        return self._scope == 'constant'

+    @property


Move to AbstractFunction I'd say

I will see how I can improve things. Note that DiscreteFunction has a _distributor attribute too

perhaps this should be hoisted to the superclass:

https://github.com/devitocodes/devito/blob/master/devito/types/dense.py#L177-L181

...

I think so yes

devito/types/misc.py

mloubout · 2023-06-05T12:44:42Z

tests/test_gpu_common.py

@@ -1,3 +1,5 @@
+import cloudpickle as pickle


We might want to do both picklle and cloudpickle as parameters for all pickling tests

AFAICT, cloudpickle >>> pickle though

Anyway, how would you implement it? Thinking about it, and it doesn't sound obvious... does it?

mloubout · 2023-06-05T12:45:33Z

tests/test_gpu_common.py

@@ -1343,6 +1350,25 @@ def test_npthreads(self):
            assert op.arguments(time_M=2, npthreads0=5)


+class TestMisc(object):
+
+    def test_pickling(self):


That should be in test_pickle but I guess it's easier for gpu to dso it here

mmhh in my view, the GPU tests are "more specific" than the pickling tests

Just like some MPI tests are found outside of test_mpi.py

mloubout · 2023-06-05T12:46:04Z

tests/test_pickle.py

@@ -1,7 +1,7 @@
 import pytest
 import numpy as np
 from sympy import Symbol
-import pickle
+import cloudpickle as pickle


As above, import both and make all tests for each

ah maybe i see what you mean now... are you saying something like

import pickle as pickle0 import cloudpickle as pickle1 @pytest.parametrize('pickle', [pickle0, pickle1]) def test_...(...): ...

?

isn't it overkill?

Yes what I mean. Since there seems to be breaking difference between those two pickling implementation I think it would be safer to have it yeah. THese are very cheap tests so it shouldn't be an issue

yeah. The point, though, is that even if cloudpickle didn't work, then pickle still surely wouldn't, at least with PRO, because pickle can't pickle (sorry...) lambda functions

BTW, the other interesting candidate is dill

Anyway, since it's a minor change, I'm happy to test both cloudpickle and pickle here in OSS. Will try to do it tomorrow

done. Please take a look, I've restructured a few things

FabioLuporini · 2023-06-07T06:49:29Z

@mloubout update: addressed your comments and rebased on latest master

georgebisbas

Not sure I have something to comment. Should I look into something specific that you believe needs more attention?

mloubout · 2023-06-07T12:31:52Z

devito/mpi/routines.py

@@ -338,35 +338,37 @@ def _make_all(self, f, hse, msg):

    def _make_copy(self, f, hse, key, swap=False):
        dims = [d.root for d in f.dimensions if d not in hse.loc_indices]
+        ofs = [Symbol(name='o%s' % d.root, is_const=True) for d in f.dimensions]


That's the same symbol as grid.origin, doesn't seem to create an issue anywhere but seems risky

FabioLuporini requested review from georgebisbas and mloubout May 31, 2023 09:03

FabioLuporini changed the title ~~compiler: Misc fixes and improvements~~ compiler: Misc compiler fixes and improvements -- part II May 31, 2023

FabioLuporini added the compiler label May 31, 2023

FabioLuporini force-pushed the fix-udx2-MPI branch from de9ace3 to 27fa6d3 Compare May 31, 2023 12:49

georgebisbas reviewed Jun 1, 2023

View reviewed changes

FabioLuporini force-pushed the fix-udx2-MPI branch from 3103a91 to 404339f Compare June 4, 2023 20:59

mloubout reviewed Jun 5, 2023

View reviewed changes

FabioLuporini added 19 commits June 7, 2023 06:21

compiler: Fix Array.shape

ee57fb8

mpi: Support C-land Array halo exchange

858c348

compiler: Relax WaitLock regions in a ScheduleTree

4a184e0

compiler: Relax HaloTouch behavior upon uxreplace

2d1573c

compiler: Improve HaloTouch + uxreplace

7d83331

mpi: Use ad-hoc Dimensions for buffers

14257b1

compiler: Patch merge_halospot in presence of Conditionals

3cd5ae6

compiler: Enable rcompile customization

134dcd3

mpi: Patch MPIMsg construction

864af11

compiler: Patch double-buffering

c683071

compiler: Fix BoundSymbol and Indirection pickling

23a5824

compiler: Enable HierarchyLayer comparison

ca81cea

misc: Switch from pickle to cloudpickle

e87a0c7

compiler: Patch HaloTouch via uxreplace

b596dae

compiler: Enhance Reconstructable with variadic args support

a185119

compiler: Revamp FIndexed for correct reconstruction

04ae0ec

compiler: Patch c_char_p lowering

0da295e

compiler: Simplify is_on_device

dc43c52

compiler: Impose canonical ordering for HaloTouch args

1a20fa8

FabioLuporini added 3 commits June 7, 2023 06:21

compiler: Promote distributor to AbstractFunction

a38db4b

tests: Try both pickle and cloudpickle

25fbc79

arch: Tidy up

9636366

FabioLuporini force-pushed the fix-udx2-MPI branch from 404339f to 9636366 Compare June 7, 2023 06:48

georgebisbas approved these changes Jun 7, 2023

View reviewed changes

mloubout approved these changes Jun 7, 2023

View reviewed changes

FabioLuporini merged commit aedc3b9 into master Jun 7, 2023

FabioLuporini deleted the fix-udx2-MPI branch June 7, 2023 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Misc compiler fixes and improvements -- part II #2138

compiler: Misc compiler fixes and improvements -- part II #2138

FabioLuporini commented May 31, 2023

review-notebook-app bot commented May 31, 2023

codecov bot commented May 31, 2023 •

edited

Loading

georgebisbas left a comment

FabioLuporini commented Jun 1, 2023

mloubout May 31, 2023

FabioLuporini Jun 5, 2023

mloubout Jun 5, 2023

FabioLuporini Jun 5, 2023 •

edited

Loading

mloubout Jun 5, 2023

FabioLuporini Jun 5, 2023

FabioLuporini Jun 5, 2023

mloubout Jun 5, 2023

FabioLuporini Jun 7, 2023

mloubout Jun 5, 2023

FabioLuporini Jun 5, 2023

mloubout Jun 5, 2023

FabioLuporini Jun 5, 2023

mloubout Jun 5, 2023

FabioLuporini Jun 5, 2023

mloubout Jun 5, 2023

FabioLuporini Jun 5, 2023

FabioLuporini Jun 7, 2023

FabioLuporini commented Jun 7, 2023

georgebisbas left a comment

mloubout Jun 7, 2023

compiler: Misc compiler fixes and improvements -- part II #2138

compiler: Misc compiler fixes and improvements -- part II #2138

Conversation

FabioLuporini commented May 31, 2023

review-notebook-app bot commented May 31, 2023

codecov bot commented May 31, 2023 • edited Loading

Codecov Report

georgebisbas left a comment

Choose a reason for hiding this comment

FabioLuporini commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini commented Jun 7, 2023

georgebisbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 31, 2023 •

edited

Loading

FabioLuporini Jun 5, 2023 •

edited

Loading