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

Loading some ARMI DBs without the App that created them #1917

Merged
merged 35 commits into from
Oct 22, 2024

Conversation

john-science
Copy link
Member

@john-science john-science commented Sep 30, 2024

What is the change?

This PR add supports for loading of some ARMI Databases without the App that created them. This is not meant to be a "complete solution" to load ANY ARMI database with that app that created it. But it should help ease the burden a bit.

Also, since reading a data file without understanding the program that created it is a potentially hazardous idea, I have placed some training wheels on the resultant reactor data model. The parameters on this data model are set to read-only mode. This is to make it CLEAR that you do not have enough information to continue to time-evolve or alter this data model.

Why is the change being made?

This was a user request.


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.

@john-science john-science added the feature request Smaller user request label Sep 30, 2024
@john-science john-science marked this pull request as draft September 30, 2024 21:30
@john-science john-science marked this pull request as ready for review October 2, 2024 21:12
@john-science
Copy link
Member Author

@alexhjames @jeffbaylor I just want to keep this on everyone's radar. I'm still hoping for a review.

@drewj-tp
Copy link
Contributor

Passing comment that came to me this morning: this only works if all the materials and shapes in the database are loadable, right? This change seems to

  • skip blueprints if we don't know how to process them
  • set all parameters to read only

Which is good.

But if my application defines a new component that only exists in my application, we will have no way to load that, correct? Same for a material.

If we really want to make it possible so any application can open any database, as the title here suggests, those are two additional things we'd need to handle

@john-science
Copy link
Member Author

But if my application defines a new component that only exists in my application, we will have no way to load that, correct? Same for a material.

If we really want to make it possible so any application can open any database, as the title here suggests, those are two additional things we'd need to handle

Correct. There are actually many more things that people can do in their code that will make it hard to open an ARMI DB without the Application that created it.

Please note that the code in this PR does not suggest a complete solution to that problem. I provide a tool to ignore blueprint sections, and another to ignore parameters. That's really all you get here.

The only "complete solution" to load a DB without the App that created it would mean storing huge portions of that code in the DB. And I don't think that's a rabbit hole we want to go down, it's been tried before.

TLDR; the title of this PR and its ticket are perhaps the problem here. I grant that. Here I provide 2 or 3 tools that downstream users can try to use to solve the problem, but only in a limited fashion.

Copy link
Contributor

@alexhjames alexhjames left a comment

Choose a reason for hiding this comment

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

I completed reviewing the PR contents and have a few comments. I have not done any testing with Oxbow yet. I'll work on that next.

armi/bookkeeping/db/database3.py Outdated Show resolved Hide resolved
armi/bookkeeping/db/passiveDBLoadPlugin.py Outdated Show resolved Hide resolved
armi/bookkeeping/db/passiveDBLoadPlugin.py Outdated Show resolved Hide resolved
armi/bookkeeping/db/passiveDBLoadPlugin.py Outdated Show resolved Hide resolved
armi/bookkeeping/db/passiveDBLoadPlugin.py Outdated Show resolved Hide resolved
armi/bookkeeping/db/tests/test_databaseInterface.py Outdated Show resolved Hide resolved
armi/reactor/reactorParameters.py Outdated Show resolved Hide resolved
armi/reactor/tests/test_parameters.py Outdated Show resolved Hide resolved
armi/bookkeeping/db/passiveDBLoadPlugin.py Outdated Show resolved Hide resolved
@john-science john-science changed the title Loading an ARMI DB without the App that created it Loading some ARMI DBs without the App that created them Oct 17, 2024
Copy link
Contributor

@alexhjames alexhjames left a comment

Choose a reason for hiding this comment

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

My only question would be if there should be any accompanying SQA documentation for the read only load (like req/implementation tags)? If you want to discuss that at a later date, I'm fine with this PR as is.

Copy link
Member

@jakehader jakehader left a comment

Choose a reason for hiding this comment

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

Nice job! Thanks @john-science!

@john-science john-science merged commit f7d7eed into main Oct 22, 2024
19 checks passed
@john-science john-science deleted the load_db_without_app branch October 22, 2024 22:04
drewj-tp added a commit that referenced this pull request Oct 23, 2024
…xial-linkage

* origin/main:
  Loading some ARMI DBs without the App that created them (#1917)
  Revert "Revert "Updating cluster settings (#1958)" (#1965)" (#1969)
  Allowing users to define flag names with digits (#1966)
  Revert "Updating cluster settings (#1958)" (#1965)
  Updating cluster settings (#1958)
  Reducing warnings while building the docs (#1959)
  Moving anl-afci-177 test files to their own directory (#1957)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants