Repository navigation
perf(declarative): skip runtime metaschema check of the component schema - #1224
devin-ai-integration[bot] wants to merge 2 commits into
Conversation
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/1791502007-skip-component-schema-check#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/1791502007-skip-component-schema-checkPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
The issue this PR resolves has one addition, under "Proposed change", "Risks to cover" and "Acceptance criteria" of airbytehq/airbyte-internal-issues#17933. Please update the PR for it. Check the schema on the failure path When validation finds an error, or raises, call validator_class = validator_for(schema)
try:
error = best_match(validator_class(schema).iter_errors(manifest))
except Exception:
validator_class.check_schema(schema) # a broken schema is reported as SchemaError
raise
if error is not None:
validator_class.check_schema(schema)
raise ValidationError(...) from errorA valid manifest never reaches Why: a CDK developer who breaks
Please cover, for the concurrent and the legacy source:
The "Risk" section of the description will need updating too. |
…n fails Co-Authored-By: bot_apk <apk@cognition.ai>
|
Patrick Nilan (@pnilan) Done in 29b03be, reply to this comment.
I updated the Summary, Test Coverage and Risk sections of the description to match. |
Patrick Nilan (pnilan)
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI Agent.
Review: fix before merge
The change is correct and non-breaking. For a valid manifest, _validate_source is now jsonschema.validate() without check_schema, and results matched in 56,238 cases across all 21 jsonschema releases that >=4.17.3,<5.0 allows. The new tests also catch the regression they target: on main only the two does_not_check tests fail, and on ff64dbe only the four broken-schema tests fail.
Please address the items below. Items 1–3 are needed before merge. Items 4–6 are smaller fixes to make in the same pass.
1. Keep a traceback on the chained cause (P2)
concurrent_declarative_source.py:360-364 and manifest_declarative_source.py:462-466
The failure-path structure requested earlier in this thread has a side effect. best_match() returns an error that is never raised, so __cause__.__traceback__ is None. traceback.format_exc() then prints the cause without a Traceback (most recent call last): header, and the whole string starts with jsonschema.exceptions.ValidationError: ….
The SDM sends that string as the TRACE stack_trace (_run.py:235, and :121 for bundled manifests). The platform's Sentry reporter parses Python frames only when the trace starts with that header (SentryExceptionHelper.kt:66). Otherwise it tags stacktrace_parse_error=1 and groups by message text (SentryJobErrorReportingClient.kt:92), so invalid-manifest failures stop grouping by stack frame. The exception type, wrapper message and failure_type are unchanged, so job outcomes are not affected.
Raise the best match inside the try instead, in both files:
validator_class = validator_for(self._declarative_component_schema)
try:
error = best_match(
validator_class(self._declarative_component_schema).iter_errors(self._source_config)
)
if error is not None:
raise error
except ValidationError as e:
validator_class.check_schema(self._declarative_component_schema)
raise ValidationError(
"Validation against json schema defined in declarative_component_schema.yaml schema failed"
) from e
except Exception:
validator_class.check_schema(self._declarative_component_schema)
raiseI tested this. The trace starts with the header again (4 frames, against 5 on main; only jsonschema's internal frame is gone), and all 140 tests in the three changed test files pass. Also add assert cause.__traceback__ is not None to both test_invalid_manifest_raises_the_best_match_error_not_the_first tests. They compare every field of the cause except its traceback, which is how this got through.
2. Retitle the PR (P2)
perf is not an allowed type in semantic_pr_check.yml. The required Validate PR title check skipped both of its steps because the PR is a draft, so its green status means nothing yet. It will fail once the PR is marked ready for review. Use:
fix(declarative): speed up manifest validation at source startup
The released version is the same either way: the release drafter bumps patch for both perf and fix.
3. Merge main and re-run CI
One line conflicts in unit_tests/sources/declarative/test_concurrent_declarative_source.py: #1145 added TestLimits, to the same import block. Keep both names:
from airbyte_cdk.sources.declarative.concurrent_declarative_source import (
ConcurrentDeclarativeSource,
TestLimits,
_get_declarative_component_schema,
)The current CI results are for a base 10 commits behind main (916d6f6).
4. Tighten the new tests (P2)
- Re-raise branch. No test covers a schema that passes
check_schemabut makesiter_errorsraise (:357-359, legacy:459-461). Withraisedeleted, all 8 source tests still pass, and the code would raiseUnboundLocalError. Add a broken-schema case with a dangling$ref:HttpRequester.properties.path = {"$ref": "#/definitions/DoesNotExist"}. Assert thatcheck_schemapasses on it, then that building the source raises the same exception type aslist(Draft7Validator(broken).iter_errors(manifest)). - Version-dependent assertions.
cause.json_path == "$.streams[1].retriever.requester.http_method"(concurrent test:7032, legacy:2606) and theenum: "GET"premise (:7054,:2623) fail on jsonschema 4.17.3, thepyproject.tomlfloor; 4.17.3–4.20.0 checkenumas a substring. They pass in CI only because of the 4.25.1 lock. Keep the first error from the guard and assertcause.json_path != first.json_path, and break the schema withrequired: "type"instead ofenum: "GET". - Validator class.
patch.object(Draft7Validator, "check_schema", ...)(:7037,:2611) stops guarding anything if the schema's$schemachanges. Patchvalidator_for(schema)instead, and addassert_not_called()after building the source. - Migration path. Builder and the manifest server build sources with
migrate_manifest=True, which validates twice. Parametrize bothdoes_not_checktests overmigrate_manifest.
5. Remove the duplication (P3)
The new _validate_source body, its fixture and its four tests are copied into both modules, so every change in item 4 has to be made twice. Add a module-level _validate_manifest(manifest, schema) next to _get_declarative_component_schema in concurrent_declarative_source.py, and call it from both classes. Pass schema as a parameter so the existing patches of each module's _get_declarative_component_schema keep working. The legacy tests can then shrink to one test that checks the call is delegated, and the item 4 changes only need to be made once.
6. Fix the wording (P3)
- The comment at
:349(legacy:451) says "On failure the schema is checked first", which describes the old order. Suggested: "If validation fails or raises,check_schemaruns before the error propagates, so a broken schema surfaces asSchemaErrorinstead of being blamed on the manifest." - The docstring of
test_the_shipped_component_schema_is_a_valid_draft_7_schemasays_validate_sourceskipscheck_schemaat runtime, but it still runs whenever validation fails. Suggested: "_validate_sourcerunscheck_schemaonly when a manifest fails validation, so a malformed schema that still accepts manifests is caught only here." - The module docstring's "Two guards" (
test_declarative_component_schema_is_loadable.py:15) no longer counts the new test.
Then update the Summary snippet and the Risk section of the description to match.
Check: destination-motherduck fails on a MotherDuck token authentication error, the same as on #1221, #1223 and #1145. It is unrelated to this PR and needs no action.
Summary
_validate_source()inConcurrentDeclarativeSourceand the legacyManifestDeclarativeSourcecalledjsonschema.validators.validate(manifest, schema). In jsonschema 4.x that first runscls.check_schema(schema), which checks the CDK's own shippeddeclarative_component_schema.yamlagainst the Draft 7 metaschema again on every source build. It gets the same answer every time, and it runs a second time when_migrate_manifestvalidates again.This PR builds the validator directly, keeps
validate's error selection, and only checks the schema when validation fails:A valid manifest never reaches
check_schema, so it gets the full saving.best_matchis kept on purpose:Validator.validate()raises the first error, which is often a different one (see the issue).The metaschema check also runs in CI:
test_the_shipped_component_schema_is_a_valid_draft_7_schema.Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17933:
Close on merge: yes, internally filed, no customer to confirm with.
Part of https://github.com/airbytehq/airbyte-internal-issues/issues/17920. Requested by Patrick Nilan (Patrick Nilan (@pnilan)).
Timing
I timed validation directly. Each manifest is resolved with
ManifestReferenceResolverandManifestComponentTransformer, the best of 3 runs is kept, and the comparison is oldvalidate()vs newbest_match(iter_errors()). Setup: jsonschema 4.25.1, the 516manifest.yamlfiles underairbytehq/airbyte@79bef47d, and a dev VM.validate()best_match(validator.iter_errors())Draft7Validator.check_schema(component_schema)alone (min of 5)All 516 manifests are valid, so none of them reaches the failure-path
check_schema. The outcome was identical for all 516. Absolute numbers on this VM are higher than the issue's 1-CPU container numbers (25 ms saving). The saved step is the same in both measurements. An invalid manifest still pays forcheck_schemaonce, as it did before.Test Coverage
test_the_shipped_component_schema_is_a_valid_draft_7_schema(test_declarative_component_schema_is_loadable.py): asserts thatvalidator_for(schema) is Draft7Validator, then runscheck_schema.test_invalid_manifest_raises_the_best_match_error_not_the_first(concurrent and legacy): with the shipped schema, the manifest has two errors. The first error fromiter_errorsis at$.streams[0](a guard assertion checks this), and the best match is$.streams[1].retriever.requester.http_method(enum). The test checks that the wrapper message is unchanged. It also checks that__cause__matches whatjsonschema.validateraises on the same manifest in message,json_path,schema_path,validatorandvalidator_value.test_building_a_source_does_not_check_the_component_schema(concurrent and legacy): patchesDraft7Validator.check_schemato raise, then builds a valid source.test_a_broken_component_schema_that_fails_the_manifest_raises_schema_error(concurrent and legacy):HttpRequester.http_methodgetsenum: "GET", so a valid manifest fails validation. Building the source raisesSchemaError.test_a_broken_component_schema_that_breaks_validation_raises_schema_error(concurrent and legacy):HttpRequester.pathgetstype: strng, soiter_errorsraisesUnknownType(a guard checks this). Building the source raisesSchemaError.Verified against unfixed code: both
does_not_checktests fail onmain, becausevalidate()callscheck_schema. The four broken-schema tests fail on this branch's first commit, which raisedValidationErrorandUnknownTypeinstead. The best-match tests and the metaschema test pass onmaintoo, which shows the error is the same as before. With the fix, all pass: 139 passed, 1 skipped across the three test files. On the first commit I also ranunit_tests/sources/declarative,unit_tests/legacyandunit_tests/connector_builder(2547 passed, 1 skipped). Ruff and mypy pass on the changed source files.Risk
SchemaError, becausecheck_schemaruns before any validation error or exception is raised. Only a CDK change can break the schema, since it ships inside the CDK wheel. The metaschema test catches that before release.ValidateAdheresToSchemaandschema_helpersare intentionally unchanged, because their schemas come from the manifest or the connector.Link to Devin session: https://app.devin.ai/sessions/22162a3a9629407c95ac5ff107c3f0af
Open in Devin Desktop: https://app.devin.ai/desktop/session/22162a3a9629407c95ac5ff107c3f0af?variant=devin