Repository navigation
fix(low-code): build only the checked static streams during check - #1247
Conversation
|
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/1792687587-check-build-named-streams#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/1792687587-check-build-named-streamsPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Fast)5 898 tests +34 5 867 ✅ +34 5m 12s ⏱️ -39s Results for commit c6002a1. ± Comparison against base commit 41af85a. This pull request removes 1 and adds 35 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)5 901 tests 5 870 ✅ 10m 40s ⏱️ Results for commit c6002a1. ♻️ This comment has been updated with latest results. |
|
Does this PR handle if multiple static streams are defined? Do we build each? |
|
Yes, as long as the checker names every one of them. Two cases fall back to
Static streams the checker doesn't name are not built. That is D1 in the issue. Our existing tests only name a single stream. If you'd like a test where the checker names two static streams out of three and asserts that both are built and read, I can add it. |
|
What happens if a name is incorrectly spelled when there is one static check stream? What if there are multiple and one is misspelled? How does this PR handle those two cases? What about if there are no defined check streams? |
|
All three behave the same as on 1. One check stream, misspelled. The full build runs, and then 2. Several check streams, one misspelled. The full build runs, because any unknown name makes I ran 3. No check streams defined. There are three variants:
So in every case where a name is unknown or the list is empty, this PR changes nothing. Selective building only applies when every name is a known static stream. |
Patrick Nilan (pnilan)
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI Agent.
Review: one blocker, then tests, release notes and cleanups
The refactor keeps discover and read identical, which an offline comparison confirmed:
- Every manifest-only connector was built on
v7.39.0and on this head. The inputs to stream building matched for 516 of 516. use_cachematched for all 5,513 static stream configs.
The fallbacks keep today's error messages, and D5 is a clean fix. One change blocks merge (#1). The rest are tests, release notes and cleanups.
Please address this review here first. Then rebase #1248 and apply the carry-over list in the comment on that PR.
| # | Priority | Finding | Where |
|---|---|---|---|
| 1 | P0 | Check skips a subclass's streams() override |
checks/check_stream.py:119, checks/check_dynamic_stream.py:45 |
| 2 | P1 | The CheckDynamicStream fallback test never reaches the guards it tests |
test_check_dynamic_stream.py:249 |
| 3 | P1 | No tests for setup (validation, API budget) on the new paths, or for the dynamic_streams_check_configs fallback |
concurrent_declarative_source.py:486, :502; check_stream.py:121 |
| 4 | P1 | The D1 release note is incomplete and won't reach the release | PR body |
| 5 | P2 | Stream-group validation misses a checked stream's grandparent | concurrent_declarative_source.py:499 |
| 6 | P2 | New public methods: wrong or missing docstrings, and #1248 changes the contract | concurrent_declarative_source.py:484, :501 |
| 7 | P2 | The new all_stream_configs= keyword breaks 6 source-linkedin-ads unit tests (latent) |
concurrent_declarative_source.py:463 |
| 8 | P2 | Several new-path invariants have no test | concurrent_declarative_source.py:463, :489, :498; check_dynamic_stream.py:48 |
| 9 | P3 | The fallback runs setup twice | concurrent_declarative_source.py:486 |
| 10 | P3 | The spec-validation test isn't guarded against network access | test_check_stream.py:1160 |
| 11 | P3 | The title's scope has a space; the commit uses perf |
PR title |
| 12 | P3 | "Resolves" would auto-close #17934; the config_overrides test claim is wrong |
PR body |
1. P0: check skips a subclass's streams() override
_build_streams sends every ConcurrentDeclarativeSource instance, subclasses included, to streams_by_name, which builds straight from the manifest. CheckDynamicStream does the same through first_streams. A Python connector that overrides streams() no longer runs that override during check.
Affected: source-google-ads.
- It is certified and depends on
airbyte-cdk ^7.33.0. An automated PR bumps its lock every week and merges itself. SourceGoogleAds.streams()dropsend_date: ""(its spec allows"") and derivescustomer_idsfromcustomer_id(source.py:27-40).- A repro of that shape returns SUCCEEDED on
v7.39.0. On this head it returnsFAILED: … time data '' does not match format '%Y-%m-%d'. - Six more monorepo connectors override
streams(), but they pin older CDK majors.
Fix (decided): take the fast path only when streams() is the CDK's own.
- Don't use an exact-class check. Manifest-only connectors run as
SourceLocalYaml, which is a subclass, sotype(source) is ConcurrentDeclarativeSourcewould turn the speed-up off for all of them. - Put the guard inside the new methods, so that #1248 and any later caller inherit it:
# concurrent_declarative_source.py
def _uses_default_streams(self) -> bool:
"""A subclass that overrides streams() must keep going through it during check."""
return type(self).streams is ConcurrentDeclarativeSource.streams
def streams_by_name(self, stream_names: List[str]) -> Optional[List[AbstractStream]]:
if not self._uses_default_streams():
return None
...
def first_streams(self, count: int) -> Optional[List[AbstractStream]]:
if not self._uses_default_streams():
return None
...# checks/check_dynamic_stream.py: fall back to streams() when first_streams returns None
streams: Optional[List[Union[Stream, AbstractStream]]] = None
if (
isinstance(source, ConcurrentDeclarativeSource)
and self.use_check_availability
and self.stream_count >= 1
):
streams = cast(
Optional[List[Union[Stream, AbstractStream]]], source.first_streams(self.stream_count)
)
if streams is None:
streams = source.streams(config=config) # type: ignore # keep the existing commentRegression tests: add one each for CheckStream and CheckDynamicStream.
- Arrange: a subclass that overrides
streams()the way google-ads does: popend_date: "", then callsuper().streams(config=config). Give the checked stream anend_datetimeof{{ config.get('end_date', today_utc()) }}, and use config{"end_date": ""}. - Assert:
checkreturns SUCCEEDED, and the override ran exactly once.
2. P1: the CheckDynamicStream fallback test is vacuous
test_check_dynamic_stream_uses_streams_for_non_positive_count_or_disabled_availability uses MagicMock(). That mock is not a ConcurrentDeclarativeSource, so the test never reaches the use_check_availability or stream_count >= 1 checks. Deleting both guards (check_dynamic_stream.py:47-48) leaves all 9 tests in the file green.
Without those guards:
stream_count: 0would fail with "No streams to connect to".use_check_availability: falsewould stop building every stream.
Fix: use MagicMock(spec=ConcurrentDeclarativeSource). Then assert source.first_streams.assert_not_called() and source.streams.assert_called_once_with(config={}).
3. P1: test the setup on the new paths
Each of these mutations passes the whole declarative and Connector Builder suites (2,489 tests):
- removing
self._prepare_for_stream_build()fromfirst_streams; - keeping spec validation but dropping
set_api_budgetinstreams_by_name; - removing
and not self.dynamic_streams_check_configsfromcheck_stream.py:121.
The regression test for the API-budget ordering bug (oncall #11954, fixed in #986) is test_api_budget_is_set_before_dynamic_streams_evaluated. It only calls streams().
Tests to add:
- Parametrize that ordering test over
streams(),streams_by_name([<static name>])andfirst_streams(1). - Add a
CheckDynamicStreamversion oftest_check_still_fails_when_spec_config_validations_fail. - Add a
CheckStreamtest withdynamic_streams_check_configsand an unread static stream that can't be built. Assert that check is still FAILED with "Encountered an error while discovering streams".
4. P1: release note
The repo squash-merges with the PR title and an empty commit message. The release drafter builds notes from commit titles only. So the note in the body reaches the release only if the releaser pastes it in by hand. The current note also covers only unread static streams under CheckStream.
Fix: add a "Releaser: paste into the release draft" section to the PR body with this text:
checkbuilds only the streams it reads (#1247).checkexists to verify that the credentials can reach the API, so it now builds only the streams it reads.CheckStreambuilds the static streams it names, andCheckDynamicStreambuilds its firststream_countstreams. Dynamic streams are still resolved and built duringcheckuntil #1248.A problem in a stream that
checkdoesn't read no longer failscheck;discoverandreadstill build every stream and still fail. This covers streams that can't be built, such as a broken custom component, and config values that only those streams use, such as an invalidstart_datein source-paypal-transaction or source-plaid. To reject a bad config value duringcheck, declare the rule in the spec: apatterninconnection_specification, or an entry inconfig_normalization_rules.validations. Both run before any stream is built. Connector Builder checks behave the same way.
Also add these to the body's list of known affected connectors:
- source-paypal-transaction and source-plaid, because of an invalid
start_date; - source-amazon-seller-partner and source-airtable, because
CheckDynamicStreamno longer builds streams paststream_count.
5. P2: stream-group validation (decided: fix it)
_create_streams passes only the built streams to _apply_stream_groups, which looks ancestors up by name among them.
- In a chain A → B → C where check builds only C, the walk stops at B. A conflict between C and its grandparent A is missed, and check passes.
v7.39.0fails the same check with "Stream 'C' and its parent stream 'A' are both in group 'g1'".- A conflict with a direct parent is still caught.
Fix: when a parent is a top-level stream that this call didn't build, walk the copy of it inlined in the child. Limit this to top-level streams. Falling back for every missing name also walks parents that exist only inline, and that makes streams() (so discover and read) raise for a manifest that passes today.
Patch, tested on a copy of this head (195 related tests and the 5 factory stream-group tests pass)
--- a/airbyte_cdk/sources/declarative/concurrent_declarative_source.py
+++ b/airbyte_cdk/sources/declarative/concurrent_declarative_source.py
@@ -477,7 +477,9 @@
for stream_config in prepared_configs
]
- self._apply_stream_groups(source_streams)
+ self._apply_stream_groups(
+ source_streams, {stream_config.get("name") for stream_config in all_configs}
+ )
return source_streams
@@ -503,7 +505,11 @@
stream_configs = self._stream_configs(self._source_config) + self.dynamic_streams
return self._create_streams(stream_configs[:count], stream_configs)
- def _apply_stream_groups(self, streams: List[AbstractStream]) -> None:
+ def _apply_stream_groups(
+ self,
+ streams: List[AbstractStream],
+ top_level_stream_names: Optional[Set[str]] = None,
+ ) -> None:
"""Set block_simultaneous_read on streams based on the manifest's stream_groups config.
Iterates over the resolved manifest's stream_groups and matches group membership
@@ -526,10 +532,15 @@
# Validate no stream shares a group with any of its ancestor streams
stream_name_to_instance: Dict[str, AbstractStream] = {s.name: s for s in streams}
- def _collect_all_ancestor_names(stream_name: str) -> Set[str]:
+ def _collect_all_ancestor_names(
+ stream_name: str, inline_instance: Optional[AbstractStream] = None
+ ) -> Set[str]:
"""Recursively collect all ancestor stream names."""
ancestors: Set[str] = set()
inst = stream_name_to_instance.get(stream_name)
+ if inst is None and stream_name in (top_level_stream_names or set()):
+ # A top-level stream that this call did not build: walk the copy inlined in the child
+ inst = inline_instance
if not isinstance(inst, DefaultStream):
return ancestors
partition_router = inst.get_partition_router()
@@ -546,7 +557,9 @@
for parent_config in router.parent_stream_configs:
parent_name = parent_config.stream.name
ancestors.add(parent_name)
- ancestors.update(_collect_all_ancestor_names(parent_name))
+ ancestors.update(
+ _collect_all_ancestor_names(parent_name, parent_config.stream)
+ )
return ancestors
for stream in streams:Tests to add. All three use incremental substreams, because DefaultStream.get_partition_router() only returns a router for a per-partition cursor:
- A → B → C, all top-level, group
[A, C], check names C: check is FAILED with today's message. - The same group, but B exists only inline in C's router:
streams()returns['A', 'C'](unchanged), andstreams_by_name(["C"])returns['C']. - Group
[A, C], check names A: SUCCEEDED, because C is not read (D1).
6. P2: new public methods
Prefer renaming them _streams_by_name and _first_streams:
- only the two checkers call them;
- they would appear in the pdoc API reference;
- #1248 changes what
streams_by_namereturns.
If they stay public:
- Fix the
streams_by_namedocstring. In this PR it also returns every dynamic stream. - Document
first_streams, including theNonereturn from #1 and the valid range ofcount. A negativecountsilently drops the last streams.
7. P2: source-linkedin-ads unit tests
Certified source-linkedin-ads patches _initialize_cache_for_parent_streams with a one-argument function (unit_tests/test_source.py:77, :111, :334). Every build path now passes all_stream_configs=, so 6 of its 193 tests fail on this head with TypeError: … unexpected keyword argument 'all_stream_configs'.
The break is latent: its unit_tests/poetry.lock pins CDK 7.4.1. No CDK change is needed. Mention in the PR body that the connector's mock must accept **kwargs once its test lock moves.
8. P2: tests for the remaining invariants
These mutations also pass every test:
- D8 wiring (
:463,:499,:504): passingall_stream_configs=None, passing only the selected configs asall_configs, or dropping thedeepcopy. - D7 (
:489): droppingnot stream_names or, or changinganytoall. - Dropping
+ dynamic_configs(:498), which would ship D2 inside this PR. - Changing
stream_count >= 1to> 1. The build-count test uses 3, while both fleet users ofCheckDynamicStreamuse 1.
Tests to add:
- An end-to-end
streams_by_nametest that assertsuse_cacheon a parent stream and an unchanged resolved manifest. - D7 parametrized over
[]and["<static>", "<unknown>"]. - A dynamic stream that can't be built still fails check. #1248 flips this one.
- A
stream_count: 1case.
9–12. P3 cleanups
- 9. In
streams_by_name, work out the static names before_prepare_for_stream_build(). Today the fallback runs validation andset_api_budgettwice. That logs validation messages twice and leaves an orphanedHTTPAPIBudget, whoseMovingWindowCallRatePolicythreads never stop. If_stream_configsraises in that early step, returnNone, so thatstreams()reports the error exactly as today. - 10. Wrap
test_check_still_fails_when_spec_config_validations_failinwith HttpMocker():, so a regression fails fast instead of reachingapi.test.com. - 11. Retitle to
fix(low-code): build only the checked static streams during checkand squash-merge. None of the last 28 scoped titles has a space, and the branch commit'sperftype is not allowed. - 12. Change "Resolves …17934" to "Part of #17934", so that merging doesn't close the issue before #1248 lands. Also correct the claim that new tests cover
config_overrides; the existingtest_concurrent_declarative_source_config_overrides.pycovers it.
After this review
- Rebase #1248 onto this branch and apply the carry-over comment there.
- Before release, maintainers will run
/prereleaseand a livecheckon source-google-ads (withend_date: "") and on source-amazon-seller-partner or source-airtable. The PR author doesn't need to do anything for this step.
|
Addressed the review in 1101804:
All requested mutations were caught by the new tests. Next: rebase #1248 onto this and apply its carry-over list. |
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
1101804 to
c6002a1
Compare
Overview
👉 TL;DR: Connection checks now set up only the streams they actually test, instead of every stream in the connector. A stream the check doesn't read can no longer make the check fail, while discover and sync still set up every stream and still fail on the same problems.
Specifically,
CheckStreambuilds only its named static streams viaConcurrentDeclarativeSource._streams_by_name, andCheckDynamicStreambuilds only its firststream_countstreams via_first_streams. Both fall back tostreams()when a subclass overrides it (_uses_default_streams()), or when a name is empty, unknown, or dynamic.This PR is part of a stacked set:
Summary
Speeds up
checkfor manifest-only sources:CheckStreamnow builds only the static streams it reads, instead of every stream in the manifest. This is PR 1 of the two stacked PRs planned in the issue (decisions D1, D5–D9). Dynamic streams are still resolved and built here; skipping them (D2) is PR 2.Part of https://github.com/airbytehq/airbyte-internal-issues/issues/17934 (PR 1 of 2). This PR does not close the issue; #1248 (D2) is still needed.
Changes
ConcurrentDeclarativeSource(concurrent_declarative_source.py)streams()now runs through_prepare_for_stream_build()(specconfig_validations+ API budget) and_create_streams(selected, all). Its result is unchanged._streams_by_name(stream_names) -> Optional[List[AbstractStream]]: evaluatesConditionalStreams, builds only the named static configs plus every dynamic stream. ReturnsNonewhenstream_namesis empty or a name is not a static stream, so callers fall back tostreams()(D7)._first_streams(count): builds the firstcountstreams (count >= 1), static then dynamic, the same order asstreams()(D9).Nonewhen a subclass overridesstreams()(_uses_default_streams()), so connectors such as source-google-ads keep going through their override duringcheck. Static names are worked out before setup, so the fallback runs setup only once._apply_stream_groupsalso walks the inlined copy of a top-level parent that was not built, so a conflict with a grandparent still failscheck._parent_stream_names()._initialize_cache_for_parent_streamstakes an optionalall_stream_configs, so a built stream that is the parent of an unbuilt stream still getsuse_cacheas today, and only the selected configs are deep-copied (D8).CheckStream(check_stream.py)_streams_by_namewhenisinstance(source, ConcurrentDeclarativeSource)and there are nodynamic_streams_check_configs. Otherwise it callsstreams()as today (legacyManifestDeclarativeSource,MagicMocksources, dynamic or unknown names,__airbyte_check_stream_namesnaming a non-static stream).should_check_dynamic_streamstestsdynamic_streams_check_configsbeforehasattr(source, "dynamic_streams"), so a passing check no longer resolves dynamic streams a second time (D5).CheckDynamicStream(check_dynamic_stream.py): uses_first_streams(stream_count)whenuse_check_availabilityis true andstream_count >= 1. Otherwise it keeps today's path.Behaviour change (D1, decided)
An unread static stream that cannot be built no longer fails
check.discoverandreadstill build every stream and still raise that error. Config values used only by unread streams are no longer validated bycheckeither.CheckDynamicStreamno longer builds streams paststream_count.Known affected connectors:
source-alpha-vantage,source-bigcommerce(archived), andsource-mntn/source-stravawhen the config has a badly formatted date (from the issue);source-paypal-transaction,source-plaid: an invalidstart_date;source-amazon-seller-partner,source-airtable:CheckDynamicStreamno longer builds streams paststream_count.Latent test break in source-linkedin-ads: its
unit_tests/test_source.pypatches_initialize_cache_for_parent_streamswith a one-argument function. Every build path now passesall_stream_configs=, so 6 of its tests will fail withTypeErroronce its test lock moves past CDK 7.4.1. The mock needs to accept**kwargsthen. No CDK change is needed.Releaser: paste into the release draft
Test Coverage
New tests in
unit_tests/sources/declarative/checks/test_check_stream.py,test_check_dynamic_stream.pyandtest_concurrent_declarative_source.pycover:ConditionalStreamstrue / false,StateDelegatingStream,__airbyte_check_stream_names, a misspelled name (sameis not part of the catalogerror), a dynamic stream name (fallback);use_cacheon a checked stream that is a parent of an unchecked stream, and no mutation ofall_stream_configs;dynamic_streamsevaluated once);config_validations(config_overridesis covered by the existingtest_concurrent_declarative_source_config_overrides.py);CheckDynamicStreamwithstream_countbelow the stream count,stream_count: 0,use_check_availability: false;ManifestDeclarativeSourceand aMagicMocksource still getstreams().Four of the new tests (only-named build, D1, D5,
first_streamscount) were run against unfixedmainand fail there.unit_tests/sources/declarative,unit_tests/connector_builderand the entrypoint tests pass locally (2543 passed, 1 skipped), as do ruff and mypy on the changed files.Declarative-First Evaluation
N/A — this is a CDK framework change, not a change to a connector manifest.
Not in this PR
Link to Devin session: https://app.devin.ai/sessions/328d83d551ec44219895b69f3d2f9bd3
Open in Devin Desktop: https://app.devin.ai/desktop/session/328d83d551ec44219895b69f3d2f9bd3?variant=devin