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

Improve API for subphase shifting #1129

Open
PhilMiller opened this issue Oct 27, 2020 · 4 comments · May be fixed by #1805
Open

Improve API for subphase shifting #1129

PhilMiller opened this issue Oct 27, 2020 · 4 comments · May be fixed by #1805
Assignees

Comments

@PhilMiller
Copy link
Member

What Needs to be Done?

Consider adding API to the phase manager runSubphaseCollective(auto&& action) or to CollectionChainSet to designate work for separate subphases, rather than feeding them through individual elements.

Is your feature request related to a problem? Please describe.

Describe potential solution outcome

Describe alternatives you've considered

Additional context

@nlslatt
Copy link
Collaborator

nlslatt commented May 3, 2022

@nlslatt
Copy link
Collaborator

nlslatt commented May 3, 2022

I'm thinking about starting with tests so that we can agree on what should happen in the relevant use cases.

@PhilMiller
Copy link
Member Author

Could you copy and paste the relevant bits of discussion here, for archival access? We're not paying for Slack, so that history will become inaccessible in a few months

@nlslatt
Copy link
Collaborator

nlslatt commented May 3, 2022

Phil Miller Apr 18th at 10:28 AM
I'm strongly in favor of computing and recording subphase statistics. In the displayed summary output, I even think the reported imbalance should take subphases into account

Nicole Slattengren 15 days ago
For subphase statistics to work, we're going to need to address the variation in the number of subphases across objects. We'll also need to properly attribute the loads of objects that are not collection elements to the appropriate subphases (or exclude these loads entirely, which may not be appropriate). Is it the right time to tackle this?

Phil Miller 15 days ago
Since we don’t yet have the fully attributed data, it would maybe not be appropriate to print based on it. I think there’s a bounding effect, though - if the calculated imbalance is higher considering just the objects that do have their load attributed by subphase, I think it would be good to report that instead-of/in-addition-to the whole-phase metric

Nicole Slattengren 15 days ago
I can think of existing cases where a subphase could actually be in perfect balance but omitting the contributions of the objgroups could make it seem imbalanced. I would be hesitant to go that route not knowing if it would end up being too misleading.

Nicole Slattengren 15 days ago
Is there any reason why it would be a bad idea to increment subphase based on advancing through collective epochs? Do you think implementing something along those lines would be a big undertaking?

Phil Miller 15 days ago
I'd maybe go a slightly different direction, of having a higher level API than runInEpochCollective that would be something like runSubphase, encapsulating both the increment and the collective epoch synchronization

Nicole Slattengren 15 days ago
I like that idea because I wouldn't want to force an increment on every collective epoch, just selected ones.

Nicole Slattengren 15 days ago
Are you envisioning the subphase being managed by the PhaseManager rather than stored in each object? Should the global subphase be incremented when first entering the runSubphase? If so, should a plain runInEpochCollective call that follows a runSubphase call use the same subphase number as that runSubphase?

Phil Miller 15 days ago
I am envisioning pulling the subphases out of the individual objects.I'm not sure what we should do with code that mixes those calls

Jonathan LIfflander 15 days ago
The other thing to consider about is how this ties into the CollectionChainSet`

Jonathan LIfflander 15 days ago
If the proxy is registered with the chain set then we can increment the subphase for the collections on a certain call

Jonathan LIfflander 15 days ago
But chain sets don’t know about object groups right now, so that would be problem

Nicole Slattengren 15 days ago
Since the synchronization is generally at the end of a collective epoch, would it be better to increment the subphase after termination? Would that work in the context of a CollectionChainSet as well, incrementing (in the PhaseManager so that it applies to object groups as well) after the tasks in the nextStepCollective have completed? Would that lead to any inconsistencies?

Jonathan LIfflander 15 days ago
nextStepCollective does have a staggered entry. I’m not sure if it matters if we mark it at termination or start of next as long as we get all the work where that new epoch starts

nlslatt added a commit that referenced this issue May 3, 2022
nlslatt added a commit that referenced this issue May 4, 2022
@nlslatt nlslatt linked a pull request May 17, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants