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

Deprecate finalize_all() #5134

Merged
merged 3 commits into from
Jun 23, 2022
Merged

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jun 22, 2022

Oversight on our part, not used and not tested

Copy link
Contributor

@ldh4 ldh4 left a comment

Choose a reason for hiding this comment

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

Does this mean that this functionality is not needed or is this already being done in the regular finalize?

@@ -995,10 +995,12 @@ void push_finalize_hook(std::function<void()> f) { finalize_hooks.push(f); }

void finalize() { Impl::finalize_internal(); }

void finalize_all() {
#ifdef KOKKOS_ENABLE_DEPRECATED_CODE_3
KOKKOS_DEPRECATED void finalize_all() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is to be deprecated, should Impl::finalize_internal also be refactored to not have a default argument? Currently, it seems to be only called from finalize and finalize_all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I pretty much have the same question. What functionality do we need to offer to initialize and finalize individual backends? What are we promising to, say, Legion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped the boolean argument from Implemented::finalize().
Cleaning further raises a few questions

void ExecSpaceManager::finalize_spaces(const bool all_spaces) {
for (auto& to_finalize : exec_space_factory_list) {
to_finalize.second->finalize(all_spaces);
}
}

virtual void finalize(const bool all_spaces) = 0;

Presumably they are space we don't know about that inherits from ExecSpaceInitializerBase so we need to be careful there.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, it's fair to also drop it from

void ExecSpaceManager::finalize_spaces(const bool all_spaces)

since we control that as well but I agree to be careful about going deeper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dalg24
Copy link
Member Author

dalg24 commented Jun 23, 2022

Failures (one of the machine out of disk space) are unrelated. This is ready for merge.

@dalg24 dalg24 merged commit 2555716 into kokkos:develop Jun 23, 2022
@dalg24 dalg24 deleted the deprecate_finalize_all branch June 23, 2022 16:03
ndellingwood added a commit to ndellingwood/Trilinos that referenced this pull request Sep 30, 2022
Compatibility update for disable of kokkos deprecated code
Necessary for kokkos@4.0 release, see kokkos/kokkos#5134
ndellingwood added a commit to ndellingwood/Trilinos that referenced this pull request Oct 3, 2022
Compatibility update for disable of kokkos deprecated code
Necessary for kokkos@4.0 release, see kokkos/kokkos#5134
@ajpowelsnl ajpowelsnl mentioned this pull request Nov 7, 2022
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.

None yet

4 participants