Re-incorporate the teams example/tests from OpenSHMEM specification #907

davidozog · 2019-11-19T21:42:17Z

This reverts commit 90886c8, which removed some teams-related tests that are based on the examples from the specification. This PR is intended to resolve the outstanding issues with these tests.

test/shmemx/shmemx_team_split_2D.c fails with 12 PEs on the split_2D at line 32 with the following error:


[0008] WARN:  ../../src/shmem_team.c:246: shmem_internal_team_split_strided
[0008]        Invalid start, stride, or size in team_split operation
[0008] ERROR: ../../src/shmem_team.c:329: shmem_internal_team_split_2d
[0008]        x-axis 2D strided split failed

test/shmemx/shmemx_team_context.c fails with:

$ mpiexec -np 4 test/shmemx/shmemx_team_context
Send to neighbor fail due to invalid context
Send to neighbor fail due to invalid context
Send to neighbor fail due to invalid context
Send to neighbor fail due to invalid context
Send to neighbor fail due to invalid context
Fail to translate pe 4 from 3s context to 2s context

test/shmemx/shmemx_team_sync.c (and shmemx_reduce.c?) also appears to have some bugs.

Related issues:
openshmem-org/specification#302
openshmem-org/specification#313
#901

This reverts commit 90886c8.

Signed-off-by: Md <[email protected]>

davidozog · 2019-11-21T19:51:02Z

test/shmemx/Makefile.am

-	shmemx_wait_until_all \
-	shmemx_wait_until_any \
-	shmemx_wait_until_some \
-	shmemx_test_all \


I don't believe shmemx_test_all is actually a part of the spec, if that matters...

Also, it looks like shmemx_test_all is another case of using shmem_p instead of shmem_atomic_set.

davidozog · 2019-11-21T19:56:54Z

test/shmemx/Makefile.am

@@ -31,26 +31,10 @@ check_PROGRAMS += \
 	atomic_nbi \
 	put_signal \
 	put_signal_nbi \
-	shmemx_wait_until_all \


shmemx_wait_until_all looks like it's a little different than the spec example (it uses shmem_p instead of shmem_atomic_set). Do we want to update that in this PR or separately?

Actually, it looks like all the wait/test any/all/some routines have this problem.

This was updated upstream in the spec (wait/test clarification proposal). One motivation for separating out the spec tests is that this should make it easier for us to sync with the upstream sources.

davidozog · 2019-11-21T19:57:54Z

test/shmemx/Makefile.am

@@ -31,26 +31,10 @@ check_PROGRAMS += \
 	atomic_nbi \
 	put_signal \
 	put_signal_nbi \
-	shmemx_wait_until_all \
-	shmemx_wait_until_any \


shmemx_wait_until_any also looks like it uses shmem_p instead of shmem_atomic_set...

This is fixed upstream in the OpenSHMEM spec. One reason for this change is to make it easier for us to pull in updates from the spec.

davidozog · 2019-11-21T20:00:17Z

test/shmemx/Makefile.am

@@ -31,26 +31,10 @@ check_PROGRAMS += \
 	atomic_nbi \
 	put_signal \
 	put_signal_nbi \
-	shmemx_wait_until_all \
-	shmemx_wait_until_any \
-	shmemx_wait_until_some \


shmemx_wait_until_some also looks like it uses shmem_p instead of shmem_atomic_set...

davidozog · 2019-11-21T20:03:40Z

test/shmemx/Makefile.am

-	shmemx_wait_until_any \
-	shmemx_wait_until_some \
-	shmemx_test_all \
-	shmemx_test_any \


shmemx_test_any is another case of using shmem_p instead of shmem_atomic_set...

davidozog · 2019-11-21T20:04:38Z

test/shmemx/Makefile.am

-	shmemx_wait_until_some \
-	shmemx_test_all \
-	shmemx_test_any \
-	shmemx_test_some \


shmemx_test_some is another case of using shmem_p instead of shmem_atomic_set.

(Yup, they all have that problem - my bad).

wrrobin · 2019-11-21T20:20:16Z

@davidozog Yes, I saw those differences. But, I still kept them here because there are enough similarities. And also, the text before the header suggests that these tests are derived from spec. I am ok with keeping them at either location. Other options might be, we can keep two versions of these tests if we want to, or we can change them to reflect the spec.

jdinan · 2019-11-21T20:54:06Z

@wrrobin Can you check for updates here: https://github.com/openshmem-org/specification/tree/master/example_code

Also, I think a few of the test files have different names in SOS. We should fix this to match the spec, so it is easier for us to keep them in sync.

wrrobin · 2019-11-21T20:57:24Z

@jdinan I noticed the name differences and I thought SOS has better names. Is it ok to change the file names in the spec repo? For example, many of the file names have example in it, which is unnecessary.

wrrobin · 2019-11-21T21:14:06Z

@jdinan, I believe we should keep the exact same code from the spec, right? For example, if we see the diff in shmem_ctx.c (First is in spec, Second is in SOS)

43c43
<     int tl, i;
---
>     int tl, i, ret;
49,50c49,60
<     shmem_init_thread(SHMEM_THREAD_MULTIPLE, &tl);
<     if (tl != SHMEM_THREAD_MULTIPLE) shmem_global_exit(1);
---
>     ret = shmem_init_thread(SHMEM_THREAD_MULTIPLE, &tl);
>
>     if (tl != SHMEM_THREAD_MULTIPLE || ret != 0) {
>         printf("Init failed (requested thread level %d, got %d, ret %d)\n",
>                SHMEM_THREAD_MULTIPLE, tl, ret);
>
>         if (ret == 0) {
>             shmem_global_exit(1);
>         } else {
>             return ret;
>         }
>     }
62,63c72,73
<             printf("%d: Error creating context (%d)\n", me, ret);
<             shmem_global_exit(2);
---
>             printf("%d: Warning, unable to create context (%d)\n", me, ret);
>             ctx = SHMEM_CTX_DEFAULT;
69c79
<             long task = shmem_atomic_fetch_inc(ctx, &task_cntr, task_pe);
---
>             long task = shmem_ctx_long_atomic_fetch_inc(ctx, &task_cntr, task_pe);
73c83
<                 task = shmem_atomic_fetch_inc(ctx, &task_cntr, task_pe);
---
>                 task = shmem_ctx_long_atomic_fetch_inc(ctx, &task_cntr, task_pe);
79c89
<         shmem_ctx_destroy(ctx);
---
>         if (ctx != SHMEM_CTX_DEFAULT) shmem_ctx_destroy(ctx);
84a95,97
>     if (me == 0 && result)
>         printf("Error: total_done is %ld, expected %ld\n", total_done, ntasks * npes);
>

It looks like one in SOS are better coded. Should we merge those changes in the spec then as doc edits?

wrrobin · 2019-11-21T22:13:35Z

test/spec-example/shmemx_team_split_2D.c

+        }
+        shmemx_team_sync(SHMEMX_TEAM_WORLD);
+      }
+


What is the failure scenario for this test apart from the npes check?

…xamples Signed-off-by: Md <[email protected]>

Signed-off-by: Md <[email protected]>

jdinan · 2019-12-04T19:31:54Z

src/synchronization_c.c4

+                if (status[i]) num_ignored++;                                                  \
+            }                                                                                  \
+        }                                                                                      \
+        if (nelems == 0 || num_ignored == nelems) {                                            \


Do we have a unit test that exercises the num_ignored == nelems case?

Good question. I don't think so.

In such a unit test, we should just check whether wait_until_all returns, right? There is no need to check the completion of the AMO since the wait set is empty.

Looking at the spec, there is one sentence at the end of the API description for wait_until_all which may not hold true in these cases.

Implementations must ensure that shmem_wait_until_all does not return before the update of the memory indicated by ivars is fully complete.

Do we have to add an unless clause indicating if the status array does not include all PEs.

Doesn't this sentence cover the issue?

If all elements in status are set to 1 or nelems is 0, the wait set is empty and this routine returns immediately.

Yes, it does. But the last sentence in the API description is not covering the special case. I guess it was confusing to me since that line is written as a separate paragraph. It might be easier to merge this sentence to the end of the first paragraph.

Ah, I see the confusion. It might be correct, since there is no "update of the memory" in this special case right? But yeah, it could use some clarification - if I understand correctly, your suggestion could be a doc edit.

Yes, definitely doc edit should be sufficient. Similar edits may go to any/some/vector APIs as well.

jdinan

@wrrobin Could you separate out moving files and merging upstream changes? Many of the upstream changes are deleting a bunch of local changes that we need to keep (contexts and C99 compatibility, and error checking). I'd like to review those changes closely.

test/spec-example/shmem_ctx.c

test/spec-example/shmem_ctx_pipelined_reduce.c

wrrobin · 2019-12-04T20:02:50Z

@jdinan To keep these checks, do we want these tests in both directories then? One in the spec_example would be exactly same as in the spec and the other one in unit would have all the checks. I have similar doubt in mind; so for the wait/test unit tests, I did not remove these extra checks and kept them different from the spec examples.

…ompile/runs and other checks Signed-off-by: Md <[email protected]>

jdinan · 2019-12-05T14:48:06Z

If we want to run the examples in the spec verbatim, we can do that off of the spec repository. It's useful to maintain copies in the SOS repository in cases where we make changes to adapt the examples into proper unit tests.

Edit: Is it helpful to add a README file with the above info to the spec-example directory to explain the motivation and how we are maintaining these tests?

Signed-off-by: Md <[email protected]>

wrrobin · 2019-12-11T20:38:22Z

@jdinan @davidozog Let me know if the changes look good.

jdinan · 2019-12-11T20:49:29Z

sandia-openshmem.spec.in

@@ -51,6 +51,8 @@ install -d %{testdir}/unit
 install test/unit/.libs/* %{testdir}/unit
 install -d %{testdir}/shmemx
 install test/shmemx/.libs/* %{testdir}/shmemx
+install -d %{testdir}/spec-example
+install test/spec-example/.libs/* %{testdir}/spec-example


I am impressed that you remembered to update the spec file! 💯

jdinan · 2019-12-11T20:53:50Z

@wrrobin Looks like there is a merge conflict that also needs to be resolved.

Signed-off-by: Md <[email protected]>

…st in shmemx Signed-off-by: Md <[email protected]>

wrrobin · 2019-12-11T21:32:02Z

Apologies for multiple commits. Merge conflict is resolved now.

jdinan · 2019-12-12T14:36:47Z

@davidozog This looks good to me, please take a look and merge when you are ready.

davidozog · 2019-12-12T15:45:55Z

@jdinan @wrrobin - looks like shmem_test_all also needs the status array, correct?

Want me to make that update?

wrrobin · 2019-12-12T15:50:43Z

@davidozog Oh.. did not check that, Yes, please go ahead if you want to.

jdinan · 2019-12-12T15:59:56Z

Let's merge these changes and handle shmem_test_all as a separate PR.

jdinan · 2019-12-12T16:00:14Z

Oops. 🤕

davidozog · 2019-12-12T16:02:08Z

@jdinan - ok, let me just finish my review, then I'll post the separate PR for shmem_test_all.

davidozog

Looks good!

davidozog and others added 2 commits November 19, 2019 11:49

Revert "Remove sketchy spec tests"

3e76d0b

This reverts commit 90886c8.

Moved specification example tests to test/spec-example

68512de

Signed-off-by: Md <[email protected]>

davidozog commented Nov 21, 2019

View reviewed changes

wrrobin reviewed Nov 21, 2019

View reviewed changes

wrrobin added 3 commits November 22, 2019 13:41

Updated ctx, test, and wait_until tests to be sync'ed with the spec e…

01d03c3

…xamples Signed-off-by: Md <[email protected]>

Added status argument to wait_until_all

ae7a958

Signed-off-by: Md <[email protected]>

Added back missing test file shmemx_test_all

8ef0b5c

Signed-off-by: Md <[email protected]>

jdinan reviewed Dec 4, 2019

View reviewed changes

jdinan requested changes Dec 4, 2019

View reviewed changes

test/spec-example/shmem_ctx.c Outdated Show resolved Hide resolved

test/spec-example/shmem_ctx.c Outdated Show resolved Hide resolved

test/spec-example/shmem_ctx_pipelined_reduce.c Outdated Show resolved Hide resolved

Keeping local changes to context texts in spec-example to allow c99 c…

f95cbf9

…ompile/runs and other checks Signed-off-by: Md <[email protected]>

Removed the 'Remove sketchy spec tests' tests

b4473d5

Signed-off-by: Md <[email protected]>

jdinan approved these changes Dec 11, 2019

View reviewed changes

wrrobin added 3 commits December 11, 2019 15:16

Adding new team tests to resolve merge conflict

3404475

Signed-off-by: Md <[email protected]>

Merge remote-tracking branch 'upstream/master' into pr/teams_spec_tests

dd5bc84

Removed the spec tests from shmemx dir; Kept only team_translate_2 te…

9116ffa

…st in shmemx Signed-off-by: Md <[email protected]>

jdinan closed this Dec 12, 2019

jdinan reopened this Dec 12, 2019

davidozog mentioned this pull request Dec 12, 2019

Update SHMEMX extended wait/test to match final proposal #863

Closed

davidozog commented Dec 12, 2019

View reviewed changes

davidozog merged commit b3bac65 into Sandia-OpenSHMEM:master Dec 12, 2019

davidozog mentioned this pull request Dec 12, 2019

Add teams spec tests to "spec-example" directory #918

Merged

Re-incorporate the teams example/tests from OpenSHMEM specification #907

Re-incorporate the teams example/tests from OpenSHMEM specification #907

Conversation

davidozog commented Nov 19, 2019 • 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

wrrobin commented Nov 21, 2019 • edited Loading

jdinan commented Nov 21, 2019

wrrobin commented Nov 21, 2019

wrrobin commented Nov 21, 2019

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

jdinan left a comment

Choose a reason for hiding this comment

wrrobin commented Dec 4, 2019

jdinan commented Dec 5, 2019 • edited Loading

wrrobin commented Dec 11, 2019

Choose a reason for hiding this comment

jdinan commented Dec 11, 2019

wrrobin commented Dec 11, 2019

jdinan commented Dec 12, 2019

davidozog commented Dec 12, 2019 • edited Loading

wrrobin commented Dec 12, 2019

jdinan commented Dec 12, 2019

jdinan commented Dec 12, 2019

davidozog commented Dec 12, 2019

davidozog left a comment

Choose a reason for hiding this comment

davidozog commented Nov 19, 2019 •

edited

Loading

wrrobin commented Nov 21, 2019 •

edited

Loading

jdinan commented Dec 5, 2019 •

edited

Loading

davidozog commented Dec 12, 2019 •

edited

Loading