-
-
Notifications
You must be signed in to change notification settings - Fork 41
🔧 Turn on XCP-QC by default in preconfigs #2204
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
Conversation
Co-authored-by: Florian Rupprecht <[email protected]> Co-authored-by: Greg Kiar <[email protected]>
…ept `blank`" to CHANGELOG [skip ci]
|
|
||
| # Generate eXtensible Connectivity Pipeline-style quality control files | ||
| generate_xcpqc_files: False | ||
| generate_xcpqc_files: On |
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 can use both on and True right?
I searched the code repo for any other occurrences and only found this comment. Should we update it as well?
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.
In YAML 1.1 (which CPAC uses) yes. 1.1 has a bunch of aliases for booleans:
y|Y|yes|Yes|YES|n|N|no|No|NO |true|True|TRUE|false|False|FALSE |on|On|ON|off|Off|OFF
https://yaml.org/type/bool.html
Starting with YAML 1.2 its only true/false though ( https://yaml.org/spec/1.2-old/spec.html#id2803629 )
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.
Funnily enough this also leads to the "The Norway Problem": when you abbreviate Norway to its ISO 3166-1 ALPHA-2 form NO, YAML will return false when parsing list of countries f.e "[GB, IN, NO,…]"
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.
I searched the code repo for any other occurrences and only found this comment. Should we update it as well?
That comment is a doctest for
Line 2211 in 7523a5c
| def update_nested_dict(d_base, d_update, fully_specified=False): |
dict, str, bool`, etc.; no YAML involved), so YAML parsing isn't relevant in that test.
In YAML 1.1 (which CPAC uses) yes. 1.1 has a bunch of aliases for booleans:
y|Y|yes|Yes|YES|n|N|no|No|NO |true|True|TRUE|false|False|FALSE |on|On|ON|off|Off|OFFhttps://yaml.org/type/bool.html
Starting with YAML 1.2 its only true/false though ( https://yaml.org/spec/1.2-old/spec.html#id2803629 )
Yeah, I'm of two minds about sticking with YAML 1.1. On the one hand, "On" / "Off" makes conceptual sense for a pipeline config, and it doesn't seem to be causing any significant problems. On the other hand, if we had something that had more than two options (like if we had n/as), I could see needing to go beyond true/false, but since we're really just using on/off to mean true/false, is the toggle switch metaphor (we're turning options on and off, not making options true or false) worth the nonintuitively broad YAML < 1.2 boolean space? If we upgrade our YAML syntax, we don't need special handling for validating bools coming from the configs.
Fixes
Related to
by @nx10 on
Related to conversation in 2025-03-27
DAIR Topic Focus: Pipeline, Platforms & Sustainability
Description
Turns
pipeline_setup.output_directory.quality_control.generate_xcpqc_fileson in all preconfigs exceptblank.Checklist
Update index.md).developbranch of the repository.Developer Certificate of Origin
Developer Certificate of Origin