-
Notifications
You must be signed in to change notification settings - Fork 97
Update ruamel.yaml dependency pin
#2405
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
base: main
Are you sure you want to change the base?
Conversation
ff2145a to
037c26f
Compare
armi/reactor/blueprints/__init__.py
Outdated
| RoundTripLoader.max_depth = None | ||
| loader = RoundTripLoader if roundTrip else CLoader | ||
| return super().load(stream, Loader=loader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not have a need for CLoader. we only ever use load with typ="safe" or "typ=rt". speed isn't a concern for these 1 off yaml loading events...we aren't doing thousands of these
I think if we drop the CLoader thing and clean up this method, we sidestep the entire issue with the deprecation of max_depth attribute for RTLoader class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-science whatcha think about a mini cleanup job here, and then we can 1. unpin ruamel.yaml and 2. likely drop the clib dep, which appears to be a dep ruamel.yaml dropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for these 1 off yaml loading events...we aren't doing thousands of these
Aren't we doing exactly that, in the unit tests? In ARMI and in the big packages with lots of tests downstream?
Honest question. How big a hit are we talking?
likely drop the clib dep
I thought that was the goal here, but the pyproject.toml doesn't have that change yet. I would love to lose the depenendency though. For sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a great question. let me get this PR running with that change and we can compare runtime for ARMI CI as well as downstream.
pyproject.toml
Outdated
| "ruamel.yaml.clib ; python_version >= '3.11.0'", # C-based core of ruamel below | ||
| "ruamel.yaml.clib<=0.2.7 ; python_version < '3.11.0'", # C-based core of ruamel below |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "ruamel.yaml.clib ; python_version >= '3.11.0'", # C-based core of ruamel below | |
| "ruamel.yaml.clib<=0.2.7 ; python_version < '3.11.0'", # C-based core of ruamel below |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea -- Let me spend a few minutes on this! I dropped th ball yesterday. Maybe It'll be all for nothing but won't know until we try
What is the change? Why is it being made?
BRB
SCR Information
Change Type: trivial
One-Sentence Rationale: The ruamel.yaml project released a new version with updated API and changed dependencies that we need to conform to.
One-line Impact on Requirements: NA
Checklist
docfolder.pyproject.toml.