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

Ensuring we throw an error on bad YAML files #1358

Merged
merged 3 commits into from
Aug 22, 2023
Merged

Conversation

john-science
Copy link
Member

@john-science john-science commented Jul 21, 2023

What is the change?

Currently, ARMI will except bad YAML files. That is, if you have the same tag twice, it accepts the second one without warning.

This PR makes such bad inputs hard failures.

Why is the change being made?

Chris opened ticket "4 4 3", where he points out you can have two components with the same name in a Block. Which, in the end, is an invalid YAML file. And we should catch that as a hard error.

Though that ticket was about a very specific problem, this solution handles a much wider range of problems. Like duplicate settings and the like.

Unfortunately, as described below, we can only solve the problem for top-level duplicate keys. Sigh.


Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) are up-to-date with any important changes.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@john-science john-science added the enhancement New feature or request label Jul 21, 2023
@keckler
Copy link
Member

keckler commented Jul 22, 2023

Is this PR complete? Looking at it, it is not immediately obvious to me how this fixes #433 . In particular, I don't see how this addresses the blueprints section of our input yaml files.

Maybe I'm missing something here or don't understand, so an example or a test explicitly on the blueprints would be helpful.

@john-science
Copy link
Member Author

john-science commented Aug 22, 2023

Okay, I hate the people that wrote the YAML spec.

I thought the problem was yamlize, then I thought it was ruamel.yaml, but no. Apparently, the official YAML spec defines this as a duplicate key, and thus an error:

blocks:
    # things
blocks:
    # stuff

But if the duplicate keys are NOT at top level, it isn't an error:

blocks:
    fuel:
        duct:
            # things
        duct:
            # stuff

So, now we are in the extremely unenviable position that if we want to catch such things we need to write our own YAML-text-parsing tool to go through and find these not-top-level duplicates.

Because, after extensive testing, ruamel.yaml and pyyaml both silently pass on this and they have no tools to throw errors or even warnings in this case.

NOTE: This PR DOES fix the problem of duplicate keys at the top-level though, so we are still keeping this PR. I have tests to prove this works for top-level keys in the PR now.

@john-science
Copy link
Member Author

I also tried the yamllint library, to find our duplicates. But it follows the YAML spec, so it only sees top-level duplicates as errors.

Sigh.

@john-science
Copy link
Member Author

@keckler This PR still has value, but I have removed the original ticket from it. As I don't think we can close that ticket.

yamlString = yamlString[:i] + section + yamlString[i : i + lenSection]

# validate that this is now an invalid YAML blueprint
with self.assertRaises(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

Is there not a more specific exception that we can assert here?

@john-science john-science merged commit afabda9 into main Aug 22, 2023
19 checks passed
@john-science john-science deleted the no_yaml_dups branch August 22, 2023 20:27
drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Aug 24, 2023
…id-refactor-pre-1278

* terrapower/main:
  Move max assembly number out of global scope (terrapower#1383)
  Ensuring we throw an error on bad YAML files (terrapower#1358)
  Removing unused settings (terrapower#1393)
  Fixing some miscellaneous TODOs (terrapower#1392)
  Removing global plugin flags (terrapower#1388)
  Update reactivity coefficient parameters (terrapower#1355)
  Fixing ruff formatting of doc gallery examples (terrapower#1389)
  Fixing broken link (terrapower#1390)
  Removing unreachable code block (terrapower#1391)
  Removing unnecessary f-strings (terrapower#1387)
  Updating documentation dependencies for Python 3.11 (terrapower#1378)
  Adding a little code coverage to the CLIs (terrapower#1371)
  Remove coveragepy-lcov dependency during CI. (terrapower#1382)
  Reconnect logger after disconnecting in test. (terrapower#1381)
  Add Python 3.11 to GH Actions (terrapower#1341)
  Removing global variable from runLog.py (terrapower#1370)
  Moving the First-Time Contributors guide to the docs (terrapower#1368)
drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Sep 6, 2023
…t-mod-empty-validation

* terrapower/main: (23 commits)
  Allowing users to set powerDensity instead of power (terrapower#1395)
  Fix assemNum parameter for uniform mesh reactor. (terrapower#1398)
  Update to black v22.6.0 (terrapower#1396)
  Fixing gallery example for new working Grids (terrapower#1394)
  Refactor armi.reactor.grids to wider submodule; Grids as ABC (terrapower#1373)
  Make SFP a child of reactor. (terrapower#1349)
  Move max assembly number out of global scope (terrapower#1383)
  Ensuring we throw an error on bad YAML files (terrapower#1358)
  Removing unused settings (terrapower#1393)
  Fixing some miscellaneous TODOs (terrapower#1392)
  Removing global plugin flags (terrapower#1388)
  Update reactivity coefficient parameters (terrapower#1355)
  Fixing ruff formatting of doc gallery examples (terrapower#1389)
  Fixing broken link (terrapower#1390)
  Removing unreachable code block (terrapower#1391)
  Removing unnecessary f-strings (terrapower#1387)
  Updating documentation dependencies for Python 3.11 (terrapower#1378)
  Adding a little code coverage to the CLIs (terrapower#1371)
  Remove coveragepy-lcov dependency during CI. (terrapower#1382)
  Reconnect logger after disconnecting in test. (terrapower#1381)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants