Repository navigation
perf(low-code): parse manifest and component schema with libyaml CSafeLoader when available - #1223
devin-ai-integration[bot] wants to merge 2 commits into
Conversation
…eLoader when available Co-Authored-By: bot_apk <apk@cognition.ai>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1791499901-yaml-csafeloader#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1791499901-yaml-csafeloaderPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
The issue this PR resolves has one addition, under "Proposed change" and "Acceptance criteria" of airbytehq/airbyte-internal-issues#17932. Please update the PR for it. Keep today's message for a YAML syntax error When the C loader raises a Why: the C loader's message keeps line and column but drops the source-line snippet when the YAML is read from a string, which is how a connector image reads its manifest ( Example: a list item indented one space shortPure-Python loader, today: C loader: Please cover:
The "Behavior notes for reviewers" line in the description, which says the snippet is omitted, will need updating too. |
…r fails Co-Authored-By: bot_apk <apk@cognition.ai>
|
Patrick Nilan (@pnilan) re: Keep today's message for a YAML syntax error (#1223 (comment)). Done in 4864013. When the C loader raises a
Parsing valid YAML never touches the pure-Python loader. New tests in
I updated the "Behavior notes" section of the PR description to match. |
Summary
Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17932:
Close on merge: yes — internally filed, no customer to confirm with.
Part of epic https://github.com/airbytehq/airbyte-internal-issues/issues/17920. Requested by Patrick Nilan (Patrick Nilan (@pnilan)).
At startup, every manifest-only connector parses its
manifest.yamland the CDK's ~5.9k-linedeclarative_component_schema.yamlwith PyYAML's pure-PythonSafeLoader. This PR parses both with libyaml'sCSafeLoaderwhen PyYAML was built with libyaml, and falls back toSafeLoaderotherwise (without libyaml, accessingyaml.CSafeLoaderraisesAttributeError).New helper in
airbyte_cdk/utils/yaml_loader.py:Call sites switched to
safe_load_yaml:YamlDeclarativeSource._read_and_parse_yaml_fileandYamlDeclarativeSource._parseconcurrent_declarative_source._get_declarative_component_schemacli/source_declarative_manifest/_run._parse_manifest_from_file(--manifest-path)legacy/.../manifest_declarative_source._get_declarative_component_schema. The issue doesn't name this one, but it is the same schema load, so I changed it too.Left unchanged, because the issue puts them out of scope: the small-input
yaml.safe_loadcalls inconfig_components_resolver,parametrized_components_resolverandmigrations_registry, plusconnector.pyand the test utilities.Measured locally (linux/amd64, CDK poetry env, PyYAML 6.0.2,
yaml.__with_libyaml__ == True)SafeLoaderCSafeLoaderdeclarative_component_schema.yamlBoth loaders produce equal objects. This PR does not add the before/after row to the benchmark table in the epic (https://github.com/airbytehq/airbyte-internal-issues/issues/17920). Patrick Nilan (@pnilan), that row still needs to be measured and added.
Behavior notes for reviewers
CSafeLoaderis not a subclass ofSafeLoader, and nothing here depends onisinstance/issubclass.yaml.YAMLError,safe_load_yamlparses the same input again withyaml.SafeLoaderand raises that error. The error type and message are therefore the same as before this PR, including the source-line snippet that the C loader leaves out for string input (which is howYamlDeclarativeSource._parsereads manifests). File objects are rewound to where they started before the second parse. If a stream can't be rewound, the C loader's error is raised. The second parse only runs after the first one fails, so parsing valid YAML never touches the pure-Python loader.Test Coverage
New
unit_tests/utils/test_yaml_loader.py:test_declarative_call_sites_use_c_loader: spies onyaml.CSafeLoaderand checks that all five call sites use it. Confirmed it fails with the old call sites (assert 0 == 1) and passes with the fix.test_syntax_error_in_string_matches_pure_python_message:str(error)matches the error from a directSafeLoaderparse, both throughsafe_load_yamland throughYamlDeclarativeSource._parse, and the^snippet is present. Confirmed it fails without the second parse.test_syntax_error_in_file_matches_pure_python_message: same check for an open file object; the message includes the file path. Confirmed it fails without the second parse.test_syntax_error_in_non_seekable_stream_keeps_c_loader_message: a stream that can't seek gets the C loader's error, not an error from the fallback.test_successful_parse_never_uses_pure_python_loader: with a spy onyaml.SafeLoader, valid str, bytes and file input never create aSafeLoader.test_pure_python_result_returned_if_only_c_loader_fails.test_falls_back_to_pure_python_loader_without_libyaml: withmonkeypatch.delattr(yaml, "CSafeLoader"),SafeLoaderis chosen and parsing (includingYamlDeclarativeSource._parse) still works.test_component_schema_parses_identically_with_both_loaders.test_malformed_yaml_raises_parser_error_with_both_loaders.test_get_safe_loader_uses_c_loader_when_available.Run locally:
ruff check,ruff formatandmypyare clean.pytest unit_tests/sources/declarativegave 2345 passed, 1 skipped (includingtest_yaml_declarative_source.py::test_source_fails_for_invalid_yaml). Thetest_yaml_loader.py,test_yaml_declarative_source.pyand source_declarative_manifest CLI tests pass with the error-message change.Declarative-First Evaluation
Not applicable: this changes the CDK framework, not a declarative connector's manifest or custom components.
Breaking change evaluation
Not breaking: parsed objects, exception classes and error messages are the same as before, and no schema, spec, stream or state changes. semantic-release sets the CDK version from the PR title.
Link to Devin session: https://app.devin.ai/sessions/aa6c3fbe780246f99b7658fa21e285ac
Open in Devin Desktop: https://app.devin.ai/desktop/session/aa6c3fbe780246f99b7658fa21e285ac?variant=devin