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

Consolidate register #12161

Merged
merged 9 commits into from Sep 27, 2018
Merged

Consolidate register #12161

merged 9 commits into from Sep 27, 2018

Conversation

rwcarlsen
Copy link
Contributor

@rwcarlsen rwcarlsen commented Sep 20, 2018

Consolidate the registerObjects, registerExecFlags, and associateSyntax static functions used to load up stuff for apps into a new, single static registerAll function. all registration stuff goes in it - including conditional test-object registration and registration of app/module dependency objects. This is made possible by modifications that make the registration process mostly idempotent - so the diamond problem of inter-app dependencies doesn't cause duplicate registration errors and such. Dynamic loading has also been updated to use this new registerAll func (i.e. extern C and dlopen stuff). This enables the full vision described in #12106.

The old register[Objects/ExecFlags] and associateSyntax functions in moose and modules have been deprecated. Apps should be updated to do all their stuff inside registerAll - this should simplify code a lot - and they no longer need to register stuff for their transitive dependencies - only their direct dependencies. The old functions will not be able to be removed, however, until all depending apps have been updated to use/call registerAll.

@rwcarlsen rwcarlsen force-pushed the consolidate-register branch 2 times, most recently from 957624f to d9b9715 Compare September 20, 2018 19:40
@moosebuild
Copy link
Contributor

moosebuild commented Sep 20, 2018

Job Documentation on a8218e7 wanted to post the following:

View the site here

This comment will be updated on new commits.

@rwcarlsen rwcarlsen force-pushed the consolidate-register branch 5 times, most recently from 7a32753 to 40f7401 Compare September 21, 2018 20:42
libmesh Outdated
@@ -1 +1 @@
Subproject commit 8516b0b93ad357ea9ec904d9021c18cf939e3bcb
Subproject commit bd111f4421ee3145f7dde6ac14aa82966cd28653
Copy link
Contributor

Choose a reason for hiding this comment

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

Caution! This contains a submodule update

@moosebuild
Copy link
Contributor

Job App tests on e6e00f9 : invalidated by @rwcarlsen

1 similar comment
@moosebuild
Copy link
Contributor

Job App tests on e6e00f9 : invalidated by @rwcarlsen

libmesh Outdated
@@ -1 +1 @@
Subproject commit 8516b0b93ad357ea9ec904d9021c18cf939e3bcb
Subproject commit bd111f4421ee3145f7dde6ac14aa82966cd28653
Copy link
Contributor

Choose a reason for hiding this comment

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

Caution! This contains a submodule update

@rwcarlsen rwcarlsen force-pushed the consolidate-register branch 2 times, most recently from c180404 to c19bfa0 Compare September 26, 2018 21:09
for Moose and for the test app.  Deprecate the
Moose::register[Objects/Actions/etc.] functions.  Make ActionFactory
and Syntax classes be idempotent w.r.t. registering stuff - so they
correctly allow and don't incorrectly error for repeated/identical
registrations. Modify MooseTestApp to only use/call
registerAll and to not do anything else in its constructor.

fully fixes idaholab#12106. ref idaholab#12136
- fix bugs and update some modules and unit test stuff
- more Test and Unit app cleanup
…ld funcs

- don't error on identical moose enum item additions. fix format test
- make .C local funcs static to avoid ambiguity calling each other
- cleanup the fix for enum item errors
- fix misc module all the way
- fix bugs in dynamics registration with new regAll stuff
- update contact and fluid_properties modules
- update functional_expansion_tools module
- update heat_conduction module
- functional_expansion_tools and heat_conduction forgotten mods
- update level_set module
- update navier_stokes module
- update porous_flow module
- update rdg module
- update richards module
- update solid_mechanics module
- update stochastic_tools module
- update xfem module
auto key = std::make_pair(name, task);
if (_current_objs.count(key) > 0)
return;
_current_objs.insert(key);
Copy link
Member

Choose a reason for hiding this comment

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

For future reference we have a moose_try_emplace() method (C++17 wrapper) which is more efficient (and a single line) for this type of thing.

@permcody permcody merged commit eff2ed9 into idaholab:devel Sep 27, 2018
rwcarlsen added a commit to rwcarlsen/moose that referenced this pull request Sep 27, 2018
@brianmoose
Copy link
Contributor

Looks like this broke both our static build targets:

https://civet.inl.gov/job/237487/

https://civet.inl.gov/job/237473/

rwcarlsen added a commit to rwcarlsen/moose that referenced this pull request Sep 27, 2018
{
FluidPropertiesApp::registerObjects(factory);
registerSyntaxTask(
"AddFluidPropertiesAction", "Modules/FluidProperties/*", "add_fluid_properties");
Copy link
Contributor

Choose a reason for hiding this comment

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

RELAP-7 and pronghorn register this syntax under FluidProperties/*, i.e. without the Modules prefix. The registerAll approach is nice, but we would need a way how to achieve the custom syntax path. Otherwise when we start using it it will break all our input files.

Originally, we would skip the syntax registration and would have our own implementation that would do the right thing. We would be fine with the registerAll approach as long as we could tell (may be upfront), that there will be some remapping going on. Or, any other simple solution...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling:

syntax.replaceActionSyntax(AddFluidPropertiesAction,
                            "FluidProperties/*",
                            "add_fluid_properties",
                            __FILE__,
                            __LINE__);

after the registerAll call should do the trick.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! I will test it in RELAP. Thanks!

Copy link
Contributor

@andrsd andrsd Oct 1, 2018

Choose a reason for hiding this comment

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

This worked. Just a small correction, the first parameter for replaceActionSyntax is std::string, so it should be "AddFluidPropertiesAction" - in case somebody finds this in future.

@tophmatthews
Copy link
Contributor

@rwcarlsen @permcody Is there going to be an email announcement or blog post about this? For those of us with non-registered apps, these types of changes can be tedious to fix if they accumulate.

@rwcarlsen
Copy link
Contributor Author

It'll at least get mentioned in the next mooseletter. Over the next several weeks, we'll work on updating apps that we can at INL. But there is no API breakage here - just deprecation.

rwcarlsen added a commit to rwcarlsen/moose that referenced this pull request Oct 4, 2018
Fixes problems introduced by some doof in idaholab#12161.  This was causing one test
in rattlesnake to exodiff for distributed mesh runs on linux.
rwcarlsen added a commit to rwcarlsen/moose that referenced this pull request Oct 4, 2018
Fixes problems introduced by some doof in idaholab#12161.  This was causing one test
in rattlesnake to exodiff for distributed mesh runs on linux.
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

6 participants