Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify & cleanup the teams example programs #11

Merged

Conversation

davidozog
Copy link

I've taken a stab at improving the teams example programs without changing the intent of the original examples, but there are a lot of changes. Is it possible to make these sort changes at this point, @jdinan?

PS - I apologize for not getting this posted earlier... please let me know how I can help during doc editing week!

@jdinan
Copy link
Owner

jdinan commented Feb 29, 2020

@davidozog Thanks for doing this. Let's pick up these changes for the RC2 draft.

jdinan pushed a commit that referenced this pull request Feb 29, 2020
Add size_t and ptrdiff_t to reduction types
@davidozog
Copy link
Author

@jdinan - this is ready for review, but I can't seem to add reviewers to the ticket.

@jdinan
Copy link
Owner

jdinan commented Mar 4, 2020

Working across forks is a bit of a challenge in this way. Could you create a placeholder issue on the RC2 project, link to this issue, and assign to committee members? This will also ensure this PR appears in our list of unresolved issues. Thanks!

example_code/shmem_reduce_example.c Show resolved Hide resolved
if (team == SHMEM_TEAM_INVALID) {
return SHMEM_CTX_INVALID;
}
int main(void)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole example is bit convoluted for a simple team-based context and team-based PE numbering in an RMA operation. Not sure, if we can change this now:

  1. Can we just create one team and use the team world instead of the team_3
  2. Am I missing some sort of a common usage model that the current example tries to provide?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure we can make such changes either, but if so, I'm ok with removing team_3. Translating to the world team is probably a more common use case. @jdinan, would this change be permissible?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing teams examples sorely need attention and I fully support using section committee power to fix them. The hitch is that we need 100% consensus for any changes at this point to clear upcoming votes. Unfortunately, we're only going to get one shot at that, since these won't be read until the RC2 meeting. Here's what I propose -- let's do the best job that we can with these in advance of the RC1 meeting. @manjugv can we get some time at the end of that meeting to read these changes for feedback and ensure that they don't cause us to fail the RC2 vote?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manjugv can we get some time at the end of that meeting to read these changes for feedback and ensure that they don't cause us to fail the RC2 vote?

👍

example_code/shmem_team_context.c Outdated Show resolved Hide resolved
example_code/shmem_team_context.c Show resolved Hide resolved
jdinan
jdinan previously requested changes Mar 11, 2020
Copy link
Owner

@jdinan jdinan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting in some work on these examples, they sorely needed it.

example_code/shmem_reduce_example.c Outdated Show resolved Hide resolved
example_code/shmem_reduce_example.c Show resolved Hide resolved
example_code/shmem_team_context.c Outdated Show resolved Hide resolved
example_code/shmem_team_context.c Outdated Show resolved Hide resolved
example_code/shmem_team_context.c Show resolved Hide resolved
@jdinan jdinan dismissed their stale review March 20, 2020 14:58

Feedback was addressed -- thanks!

@jdinan
Copy link
Owner

jdinan commented Apr 2, 2020

@davidozog Any more changes planned or can we go ahead and merge?

@davidozog
Copy link
Author

davidozog commented Apr 2, 2020

@jdinan I need to move the 2nd example from shmem_ctx_get_team to shmem_team_create_ctx, which I'll do asap. Thanks!

@davidozog
Copy link
Author

@jdinan - this PR should be ready.

It looks like the build failed b/c of a connection problem to ubuntu archives... not sure how to rerun it.

@jdinan jdinan merged commit 6b29678 into jdinan:sec/ctx-teams Apr 2, 2020
jdinan pushed a commit that referenced this pull request Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants