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

Provide plugin hook getAxialExpansionChanger #1870

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

drewj-tp
Copy link
Contributor

@drewj-tp drewj-tp commented Sep 12, 2024

What is the change?

Plugins are now able to implement the getAxialExpansionChanger hook to provide their own class responsible for
performing the initial thermal axial expansion of assemblies. The default implemented hook, brought by the ReactorPlugin, exposes the armi.reactor.converters.axialExpansion.AxialExpansionChanger class.

Why is the change being made?

This allows plugins to bring their own machinery for thermal axial expansion during the blueprints construction stage. Plugins can even opt to make a subclass that does not perform axial expansion at all.

Closes #1843

Todo

  • Release notes (need pr number to edit release notes)
  • Testing (need to prove only the first such class is returned)
    • Local dev experience indicates this is the case, but tests will be needed to prove this and prevent future problems

Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

Produce a class, not an instance thereof, that is responsible
for performing thermal axial expansion. Marked with the
`firstresult=True` such that the pluggy hook call will stop after
the first hook produces a non-`None` result.

https://pluggy.readthedocs.io/en/stable/index.html#first-result-only
Provides the "default"
`armi.reactor.converters.axialExpansionChanger.AxialExpansionChanger`
class. Marked with `trylast=True` to help make sure other hooks added
to the application are called before this one, even if they are not
marked with `tryfirst=True`.

https://pluggy.readthedocs.io/en/stable/index.html#call-time-order
Was in the area so make a minor cleanup
@drewj-tp drewj-tp self-assigned this Sep 12, 2024
@drewj-tp drewj-tp marked this pull request as ready for review September 12, 2024 18:39
Copy link
Member

@albeanth albeanth left a comment

Choose a reason for hiding this comment

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

This looks great to me. Just some feedback on the comments for clarity and posterity/long-term maintenance.

armi/plugins.py Outdated Show resolved Hide resolved
armi/plugins.py Outdated Show resolved Hide resolved
armi/plugins.py Outdated Show resolved Hide resolved
armi/plugins.py Outdated Show resolved Hide resolved
armi/plugins.py Outdated Show resolved Hide resolved
armi/tests/test_plugins.py Show resolved Hide resolved
armi/tests/test_plugins.py Show resolved Hide resolved
armi/tests/test_plugins.py Show resolved Hide resolved
doc/release/0.4.rst Outdated Show resolved Hide resolved
@john-science
Copy link
Member

I only have two small things on this PR. Take a look.

So... the purpose of this design is two-fold?

  1. Developers can now drop in their own AxialExpansionChanger subclasses.
  2. Developers can now opt to not have an AxialExpansionChanger changer at all, by just not having this plugin hook in any of their plugins?

Do I have the goals right?

IF I do, I guess I have some questions:

  1. What happens if you use this plugin hook in more than one plugin?
    • I assume this has to fail, right?
    • Have you tested that?
    • How does it fail?
  2. Does ARMI run if zero plugins implement this hook?
    • I want to make sure this change doesn't break for reactors with no assemblies.

Co-authored-by: John Stilley <[email protected]>
Copy link
Contributor Author

@drewj-tp drewj-tp left a comment

Choose a reason for hiding this comment

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

I only have two small things on this PR. Take a look.

So... the purpose of this design is two-fold?

  1. Developers can now drop in their own AxialExpansionChanger subclasses.
  2. Developers can now opt to not have an AxialExpansionChanger changer at all, by just not having this plugin hook in any of their plugins?

Do I have the goals right?

Goal 1 is correct. Any application can drop in their own AxialExpansionChanger subclass.

Goal 2 is slightly incorrect. The default ReactorPlugin implements this hook that brings the current AxialExpansionChanger. If an application does not register any plugin that implement this hook, the "default" state is to bring the existing expansion changer into the mix. To put it another way, this plugin hook is always implemented, but applications can circumvent the default expander by implementing the hook.

And, if somehow you do not register the ReactorPlugin and find yourself in a situation where no plugin registered implements this hook, I have no idea what pluggy will do. It may return None and then get some attribute errors trying to call None.expandColdDimsToHot as if it were an expander class?

IF I do, I guess I have some questions:

  1. What happens if you use this plugin hook in more than one plugin?

    • I assume this has to fail, right?
    • Have you tested that?
    • How does it fail?

@albeanth had this same question. If multiple plugins implement this hook, the first non-None result will be "selected" and used. The goal is there should be at most two plugins that implement this hook and registered with the application

  1. Default ReactorPlugin.getAxialExpanderClass
  2. Some other plugin deemed responsible of owning axial expansion.

The ReactorPlugin.getAxialExpanderClass hook implementation is marked with trylast=True so, regardless of the plugin registration ordering, it should be the last such hook to be called.

The test I added covers two plugins implementing this hook and showing the behavior of the trylast marker.

  1. No new plugin -> call plugin hooks -> default AxialExpansionChanger fetched from ReactorPlugin
  2. Register a different plugin that implements the hook -> call plugin hooks hooks -> expansion changer from second plugin is registered

def test_axialExpansionHook(self):
"""Test that plugins can override the axial expansion of assemblies via a hook."""
pm = self.app.pluginManager
first = pm.hook.getAxialExpansionChanger()
# By default, make sure we get the armi-shipped expansion class
self.assertIs(first, AxialExpansionChanger)
pm.register(SillyAxialPlugin)
second = pm.hook.getAxialExpansionChanger()
# Registering a plugin that implements the hook means we get
# that plugin's axial expander
self.assertIs(second, SillyAxialExpansionChanger)

  1. Does ARMI run if zero plugins implement this hook?

    • I want to make sure this change doesn't break for reactors with no assemblies.

One plugin already implements and registers this hook - ReactorPlugin.getAxialExpanderClass. And that performs the current behavior. So reactors with no assemblies should not see any changes if they do not implement this hook.

However, such applications could implement this hook on a plugin that produces a do-nothing expander class. That is (potentially, untested) way to skip axial expansion beyond setting Tinput=Thot

armi/tests/test_plugins.py Show resolved Hide resolved
drewj-tp and others added 2 commits September 17, 2024 16:53
Can happen if no plugin implements the hook at all. This
may involve unregistering the ReactorPlugin, but it's not impossible
Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

After CI passes, I think this is ready for show time, if you guys do.

@albeanth
Copy link
Member

After CI passes, I think this is ready for show time, if you guys do.

This is apart of a larger downstream slough of changes, so let's hold tight till those are ready?

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.

Plugin hook for initial axial expansion
3 participants