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.

Sign up for GitHub

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

Jump to bottom

CORE: fix error handling for ctx create epilog #818

Merged
merged 4 commits into from
Sep 7, 2023
Merged

CORE: fix error handling for ctx create epilog #818

merged 4 commits into from
Sep 7, 2023

Conversation

samnordmann
Copy link
Collaborator

What

Fix some bugs in the core dealing with context create epilog error handling

@samnordmann samnordmann force-pushed the core/epilog_error_handling branch 2 times, most recently from 0be4e30 to 668e55c Compare August 7, 2023 13:33
@samnordmann samnordmann force-pushed the core/epilog_error_handling branch 2 times, most recently from 5bd28cd to 98853d9 Compare August 7, 2023 14:21
shimmybalsam
shimmybalsam approved these changes Aug 8, 2023
View reviewed changes
src/core/ucc_context.c Outdated Show resolved Hide resolved
src/core/ucc_context.c Outdated Show resolved Hide resolved
Sergei-Lebedev
Sergei-Lebedev reviewed Aug 8, 2023
View reviewed changes
src/core/ucc_context.c Outdated Show resolved Hide resolved
src/core/ucc_context.c Show resolved Hide resolved
@samnordmann
Copy link
Collaborator Author

samnordmann commented Aug 8, 2023 •
edited
Loading

Additions / Issues:

  • I fixed tl/sharp ctx epilog cleanup.
  • if ctx create epilog fails, we need to cleanup ucc_tl_context_t (i.e. the base class). Do you have a suggestion on how to properly do that? Should it be in core or in each tl's ctx create epilog function ?
  • We might want/need to have two separate ctx cleanup functions, one for whats been setup during ctx init, and one for the cleaning up the full ctx after ctx create epilog. We should then use in it place of context destroy here

@samnordmann samnordmann force-pushed the core/epilog_error_handling branch from e952fa1 to 2a989e8 Compare August 8, 2023 11:40
Sergei-Lebedev
Sergei-Lebedev reviewed Aug 15, 2023
View reviewed changes
src/components/tl/sharp/tl_sharp_context.c Outdated Show resolved Hide resolved
src/core/ucc_context.c Outdated Show resolved Hide resolved
@samnordmann samnordmann force-pushed the core/epilog_error_handling branch from 0c8d0e5 to f339d80 Compare August 28, 2023 14:47
@samnordmann
Copy link
Collaborator Author

So at the end, I propose to adopt the convention that it is the responsibility of the TL implementer to ensure that ctx destroy can be called after ctx init and before ctx init epiolog.
@Sergei-Lebedev is it ok?

@Sergei-Lebedev Sergei-Lebedev force-pushed the core/epilog_error_handling branch from f339d80 to b976769 Compare September 7, 2023 16:12
@Sergei-Lebedev Sergei-Lebedev enabled auto-merge (squash) September 7, 2023 16:12
@Sergei-Lebedev Sergei-Lebedev merged commit 806fd14 into openucx:master Sep 7, 2023
@bureddy bureddy mentioned this pull request Sep 19, 2023
Merged
nsarka pushed a commit to nsarka/ucc that referenced this pull request Oct 24, 2023
@samnordmann @nsarka
CORE: fix error handling for ctx create epilog (openucx#818)
e61d6bb 
* CORE: fix error handling for ctx create epilog

* CORE: minor comments and fix

* TL/SHARP: fix ctx create epilog cleanup

* REVIEW: minor comments
nsarka pushed a commit to nsarka/ucc that referenced this pull request Oct 24, 2023
@samnordmann @nsarka
CORE: fix error handling for ctx create epilog (openucx#818)
8453a22 
* CORE: fix error handling for ctx create epilog

* CORE: minor comments and fix

* TL/SHARP: fix ctx create epilog cleanup

* REVIEW: minor comments
janjust pushed a commit to janjust/ucc that referenced this pull request Jan 31, 2024
@samnordmann @janjust
CORE: fix error handling for ctx create epilog (openucx#818)
4d446df 
* CORE: fix error handling for ctx create epilog

* CORE: minor comments and fix

* TL/SHARP: fix ctx create epilog cleanup

* REVIEW: minor comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shimmybalsam shimmybalsam shimmybalsam approved these changes

@Sergei-Lebedev Sergei-Lebedev Sergei-Lebedev approved these changes

@manjugv manjugv manjugv approved these changes

Assignees
No one assigned
Labels
Ready-for-Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samnordmann @manjugv @Sergei-Lebedev @shimmybalsam