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

Remove PyYaml Dependency #586

Merged
merged 8 commits into from
Mar 2, 2022
Merged

Remove PyYaml Dependency #586

merged 8 commits into from
Mar 2, 2022

Conversation

john-science
Copy link
Member

Description

PyYaml is a fine library, I have nothing against it. But per #575 , we have THREE different YAML libraries as dependencies in ARMI. And that seems like too many.

By far, the easiest one of the three to lose is PyYaml, as that is only used in two files, for super basic YAML-to-dictionary reading. And one of those files is a unit test.

So, in the interest of reducing our memory foot print, our dependency overhead, and making it easier to understand how we interact with our YAML files, I am removing PyYaml as an ARMI dependency.

Checklist

  • The code is understandable and maintainable to people beyond the author.
  • Tests have been added/updated to verify that the new or changed code works.
  • There is no commented out code in this PR.
  • The commit message follows good practices.
  • All docstrings are still up-to-date with these changes.

If user exposed functionality was added/changed:

  • Documentation added/updated in the doc folder.
  • New or updated dependencies have been added to setup.py.

@john-science john-science added the cleanup Code/comment cleanup: Low Priority label Feb 28, 2022
@john-science john-science linked an issue Mar 1, 2022 that may be closed by this pull request
Do we have too many YAML libraries? #575
Closed
@john-science john-science requested review from ntouran and removed request for ntouran March 1, 2022 16:19
ntouran
ntouran approved these changes Mar 1, 2022
View reviewed changes
Copy link
Member

@ntouran ntouran left a comment

Choose a reason for hiding this comment

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

very nice. love to see the optimizations along the way.

armi/physics/neutronics/fissionProductModel/lumpedFissionProduct.py Show resolved Hide resolved
@john-science john-science merged commit 035711a into terrapower:master Mar 2, 2022
@john-science john-science deleted the reduce_yaml_libs branch March 2, 2022 04:10
drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Apr 20, 2022
@drewj-usnctech
Merge remote-tracking branch 'terrapower/master' into drewj/plugins-d…
2f12bfc 
…efine-system-builders

* terrapower/master: (24 commits)
  Update docstring formatting in `getPotentialParentFromSettingValue` (terrapower#627)
  Removing inherited docstrings from Docs (terrapower#628)
  Removing bare "import armi" statements (terrapower#626)
  Fixing assembly number issue in DB Load (terrapower#615)
  Making the app version more available (terrapower#624)
  Adding MPI tests to code coverage (terrapower#625)
  Removing support for XML Settings files (terrapower#612)
  Minor language cleanup in new standards and practices. (terrapower#610)
  remove extraneous `mpicov` arg from tox.ini envlist (terrapower#604)
  Swap flux param (terrapower#605)
  Fixing _fluxSwapParam for NumPy arrays (terrapower#607)
  Unshaped component (terrapower#600)
  Fixing docstring (terrapower#601)
  Adding options to settings report (terrapower#598)
  Adding coding standards and practices to Docs (terrapower#597)
  Removing defunct code from HistoryTrackerInterface (terrapower#594)
  Fix to a bug where an ARMI application could not call (terrapower#593)
  Tracking ARMI Requirements (terrapower#590)
  Remove PyYaml Dependency (terrapower#586)
  Filling out code coverage for entry points (terrapower#587)
  ...
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
@john-science @scottyak
Remove PyYaml Dependency (terrapower#586)
277121a 
* Removing PyYaml dependency
* Removing and Fixing old TODOs
* Fail Build if code coverage under 80%
* Improving code coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntouran ntouran ntouran approved these changes

Assignees
No one assigned
Labels
cleanup Code/comment cleanup: Low Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do we have too many YAML libraries?
2 participants
@john-science @ntouran