Remove the dead CFullLoader branch from the YAML helpers - #70943
Conversation
| if name == "FullLoader": | ||
| # Try to use CFullLoader by default | ||
| getattr(yaml, "CFullLoader", yaml.FullLoader) | ||
| return getattr(yaml, "CFullLoader", yaml.FullLoader) |
There was a problem hiding this comment.
CFullLoader isn't a subclass of FullLoader (MRO is CParser, FullConstructor, Resolver), and yaml.add_constructor(tag, fn) only registers on Loader/FullLoader/UnsafeLoader. So registering a tag through this same wrapper and then loading with it goes from working to raising, not to silently ignoring:
from airflow.utils import yaml
yaml.add_constructor("!tag", lambda l, n: {"v": l.construct_scalar(n)})
yaml.load("a: !tag hi", Loader=yaml.FullLoader)
# before: {'a': {'v': 'hi'}}
# after: ConstructorError: could not determine a constructor for the tag '!tag'No in-tree caller reads FullLoader through these wrappers, but airflow.sdk.yaml is re-exported into template macros, so {{ macros.yaml.FullLoader }} is reachable from DAG code. Deleting the branch (leaving FullLoader as pyyaml's) is the zero-risk option, and the module docstring does promise a drop-in yaml. Which way do we want it? Worth updating the PR body either way, "does not see constructors" reads as a silent no-op.
There was a problem hiding this comment.
Thank you! I learned a lot from this. I hadn't thought about the compatibility issues before.
I've decided to remove the CFullLoader part and keep the pure Python version. Does this sound good to you?
| c_full_loader = type("CFullLoader", (), {}) | ||
| monkeypatch.setattr(pyyaml, "CFullLoader", c_full_loader, raising=False) | ||
|
|
||
| assert yaml.FullLoader is c_full_loader |
There was a problem hiding this comment.
All three copies of this test only pin the libyaml-present branch. Every PyYAML wheel on PyPI bundles libyaml, so CI never exercises the fallback, which means return yaml.CFullLoader would pass here and AttributeError on a source build. A monkeypatch.delattr(pyyaml, "CFullLoader", raising=False) case asserting yaml.FullLoader is pyyaml.FullLoader closes that cheaply.
These YAML wrappers are documented as a drop-in replacement for pyyaml, and the branch meant to swap in libyaml's FullLoader has never taken effect: it discarded the value it looked up. Making it work is the wrong repair. CFullLoader is not a FullLoader subclass, and yaml.add_constructor registers only on the pure-Python loaders, so tags registered through the same wrapper would start raising ConstructorError instead of resolving. That surface is reachable from Dag code, since the Task SDK copy is re-exported as macros.yaml. Keeping pyyaml's FullLoader is what callers already depend on; safe_load and dump continue to use the C implementation, where Airflow controls both ends.
rjgoyln
changed the title
386564a to
a137038
Compare
2 participants
Summary
All three copies of the YAML wrapper carry a branch in
__getattr__meant to swap in libyaml'sFullLoader. It discards the value it looks up, so it has never taken effect.Change
Deleting the branch rather than adding the missing
return.CFullLoaderis not aFullLoadersubclass, andyaml.add_constructorregisters only on the pure-Python loaders, so activating it would turn tags registered through the same wrapper intoConstructorError— a surface Dag authors reach asmacros.yaml.safe_loadanddumpkeep the C implementations, where Airflow controls both ends.No runtime change:
FullLoaderresolved to pyyaml's loader before, and still does.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines