#229: Airflow local + CI test-environment spike - #238
Merged
Conversation
First child of epic #228. Decides and records the Airflow install + test stack so later children reference one contract instead of re-litigating it. - docs/research/airflow-test-environment.md: DEC-1..DEC-5 (version floor >=2.8,<3 certified at 2.10.4/py3.11; [airflow] extra under constraints; gated airflow marker; label-gated CI job; fake-first split + one live E2E DAG), constraints-pinned install, env-var contract, in-process vs subprocess decision input, maintainer certification checklist. - pyproject.toml: register gated `airflow` marker (excluded from default addopts); exclude tests/airflow from pyright (Airflow is not a typecheck dep). - examples/airflow/signalforge_spike_dag.py: trivial DAG + inline placeholder operator proving the authoring pattern. - tests/airflow/test_dag_parse.py: gated DagBag parse certification with belt-and-suspenders gating (marker + runtime importorskip). - .gitignore: ignore .venv-airflow/ and .airflow-home/ scratch.
Certified the spike against a real constraints-pinned Airflow install. Two bugs the live run caught, now fixed: - Parse leg must read DagBag(...).dags, not bag.get_dag(): get_dag() consults the metadata DB (DagModel.get_current) and fails with "no such table: dag" in a fresh venv. Parse certification must not require `airflow db init`. - CI install step must add pytest/pytest-cov/pytest-asyncio under the same constraints file, or `--no-cov` is an unrecognised flag. Doc now records the certification results, the DB-init split (DagBag parse vs dag.test() execute), and the benign older-pytest strict_markers ini warning.
Add the label-gated `airflow` job (sibling to lint-test, not in the default matrix) + `workflow_dispatch` trigger. Runs only on PRs labelled `airflow` or manual dispatch. Recipe corrected against the live certification: - `uv venv` + constrained `uv pip install` + `.venv/bin/python -m pytest`, instead of the inconsistent `--system` + `uv run --no-sync` pairing (uv run targets the project .venv, not the --system env). - signalforge editable install carries --constraint (else protobuf 4->5 upgrade risks breaking Airflow 2.10.4); `-e .` until the skeleton child adds [airflow]. - pytest/pytest-cov/pytest-asyncio installed under constraints so --no-cov is valid. Doc updated to match the verified recipe + records the two findings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a complete Apache Airflow integration: an example DAG ( ChangesAirflow Integration
Sequence Diagram(s)sequenceDiagram
rect rgba(135, 206, 235, 0.5)
Note over pytest,DagBag: Parse test: verify DAG structure (no DB, no credentials)
pytest->>test_dag_parse: collect -m airflow
test_dag_parse->>test_dag_parse: importorskip("airflow")
test_dag_parse->>_load_example_dag: load DAG via DagBag
_load_example_dag->>DagBag: DagBag(dag_folder=examples/airflow, include_examples=False)
DagBag-->>_load_example_dag: bag.dags["signalforge_generate"], import_errors={}
_load_example_dag-->>test_dag_parse: DAG instance
test_dag_parse->>test_dag_parse: assert tasks == {generate, gate}, generate upstream of gate
end
rect rgba(144, 238, 144, 0.5)
Note over pytest,signalforge_CLI: Live test: invoke generate task (gated by SF_RUN_AIRFLOW + credentials)
pytest->>test_dag_parse: test_generate_task_runs_live_against_demo
test_dag_parse->>test_dag_parse: _live_skip_reason() → skip if env vars absent
test_dag_parse->>test_dag_parse: create demo via copy_demo, set SF_PROJECT_DIR/SF_MODEL
test_dag_parse->>_run_generate: invoke python_callable directly
_run_generate->>signalforge_CLI: subprocess ["signalforge", "generate", "--format", "json"]
signalforge_CLI-->>_run_generate: {model_unique_id, flagged, ok, total, ...}
_run_generate-->>test_dag_parse: XCom payload dict
test_dag_parse->>test_dag_parse: assert model_unique_id suffix, non-negative counts, required fields present
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Pull request overview
Establishes a reproducible, constraints-pinned Apache Airflow “spike” environment (local + CI) that later Airflow-integration work will certify against, without adding Airflow as a default dev/runtime dependency.
Changes:
- Adds a gated
airflowpytest marker + default deselection, and excludestests/airflowfrom pyright. - Introduces a minimal example DAG and a gated DAG-parse certification test using
DagBag. - Documents the full local/CI constraints-based install contract and adds a label-gated
airflowCI job (plusworkflow_dispatch) and local scratch.gitignoreentries.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/airflow/test_dag_parse.py |
Adds a gated DagBag parse test to certify example DAG parsing under real Airflow. |
pyproject.toml |
Registers airflow marker, excludes it from default pytest selection, and excludes tests/airflow from pyright. |
examples/airflow/signalforge_spike_dag.py |
Adds a minimal “spike” DAG with an inline placeholder operator for parse certification. |
docs/research/airflow-test-environment.md |
Records decisions/contract for constraints-pinned Airflow local setup + CI job shape. |
.gitignore |
Ignores .venv-airflow/ and .airflow-home/ scratch directories. |
.github/workflows/ci.yml |
Adds a label-gated airflow CI job and enables manual workflow_dispatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
81-81: ⚡ Quick winGuard PR-label access by event type in the job condition.
The current
ifreads PR-label fields even on non-PR events. Make the event-type check explicit first to avoid brittle expression evaluation across trigger types.Suggested change
- if: contains(github.event.pull_request.labels.*.name, 'airflow') || github.event_name == 'workflow_dispatch' + if: github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'airflow'))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 81, The job condition attempts to access PR-label fields regardless of the event type, which can fail or behave unexpectedly on non-PR events like workflow_dispatch. Reorder the condition to check the event type first: add an explicit check for github.event_name == 'pull_request' before accessing github.event.pull_request.labels.*.name, so that PR-label evaluation only happens when the event is actually a pull_request event. This prevents brittle expression evaluation across different trigger types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/research/airflow-test-environment.md`:
- Around line 70-77: The DEC-4 section header incorrectly describes the CI
gating as "path-filtered" when the actual implemented contract uses label-based
triggering with manual dispatch. Additionally, the checklist item about wiring
the CI job is now stale since this work has already been completed. Update the
header of the DEC-4 section to accurately reflect the label/manual trigger
mechanism rather than path filtering, update any checklist items related to
"wire the CI job" to reflect that this is now completed, and apply the same
corrections at line 285 where this contract is also referenced.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 81: The job condition attempts to access PR-label fields regardless of
the event type, which can fail or behave unexpectedly on non-PR events like
workflow_dispatch. Reorder the condition to check the event type first: add an
explicit check for github.event_name == 'pull_request' before accessing
github.event.pull_request.labels.*.name, so that PR-label evaluation only
happens when the event is actually a pull_request event. This prevents brittle
expression evaluation across different trigger types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53ad94e2-be42-4af5-80d5-16e4e3f9ed11
📒 Files selected for processing (6)
.github/workflows/ci.yml.gitignoredocs/research/airflow-test-environment.mdexamples/airflow/signalforge_spike_dag.pypyproject.tomltests/airflow/test_dag_parse.py
Replaces the placeholder spike DAG with a real, parameterized example and the user-facing ops surface (epic #228's example + docs). - examples/airflow/signalforge_generate_dag.py: PythonOperator wrapping `signalforge generate` — four-tier exit-code -> task-state, graded tier counts -> XCom, a `gate` task that fails on a configurable flagged-count threshold. Config via Airflow Variables with env overrides; manual schedule by default. (Dedicated operators remain a roadmap drop-in swap; the contract is identical.) - docs/airflow-ops.md + MkDocs nav: install-under-constraints, the example walkthrough, the result->task-state + XCom contract, gating, live env gates. - tests/airflow/test_dag_parse.py: repoint the gated parse test at the shipped example; add a gated live `generate`-callable test (self-skips without SF_RUN_AIRFLOW + ANTHROPIC_API_KEY + GOOGLE_CLOUD_PROJECT + SF_RUN_BQ). - CHANGELOG: Unreleased entry. Research doc references updated spike->generate. Certified: parse test green under Airflow 2.10.4/py3.11; live test green against the init-demo project (real BigQuery + Anthropic).
- tests/airflow: move pytest.importorskip out of module scope into each test so a default `uv run pytest` collection never imports Airflow (Copilot). The gated tests now deselect cleanly rather than collect-and-skip. - ci.yml: guard the airflow job `if:` behind an explicit pull_request event so `github.event.pull_request.labels` is never evaluated on push events (Copilot). - research doc: DEC-4 heading "path-filtered" -> "label-gated"; mark the "wire the CI job" checklist item done (Copilot + CodeRabbit — stale wording).
Owner
Author
PR Review SummaryAll five threads addressed — fixes in Fixed (5 items)
False Positives (0)None — every comment was a real, valid fix. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/airflow/test_dag_parse.py (1)
71-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the required external-service test markers to the live test.
This test uses Anthropic, BigQuery, and subprocess execution, but it only inherits the
airflowmarker. Please add explicit markers (e2e,anthropic,bigquery,cli_subprocess) on this test to preserve the project’s gated-test contract.As per coding guidelines, “Gated test markers (
bigquery,anthropic,cli_subprocess,e2e,wheel_smoke,snowflake) must be applied to tests that require external services or subprocess execution; these tests are excluded by default in local runs and CI runs them with--no-cov.”Suggested fix
+@pytest.mark.e2e +@pytest.mark.cli_subprocess +@pytest.mark.anthropic +@pytest.mark.bigquery def test_generate_task_runs_live_against_demo(tmp_path: Path) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/airflow/test_dag_parse.py` around lines 71 - 95, The test function test_generate_task_runs_live_against_demo is missing required external-service markers that indicate it depends on Anthropic, BigQuery, and subprocess execution. Add pytest.mark decorators with the markers e2e, anthropic, bigquery, and cli_subprocess above the function definition (before the def line) to properly gate this test and ensure it is excluded from local runs and marked for special handling in CI as per the project's testing guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/airflow/signalforge_generate_dag.py`:
- Around line 131-136: The code attempts to convert threshold to an integer
without first validating that it contains a valid numeric value. Add validation
after the None check and before the comparison in the line with int(threshold)
to catch non-numeric values early. Wrap the int() conversion in a try-except
block or validate the threshold string beforehand, and when conversion fails,
raise or log a clear error message that identifies the configuration key
SF_MAX_FLAGGED and the invalid value provided, rather than letting a raw
ValueError propagate.
- Line 94: The subprocess.run() call invocation lacks a timeout parameter, which
can cause indefinite blocking if the CLI or dependencies hang. Add a timeout
parameter to the subprocess.run() call to prevent worker threads from being tied
up. The timeout value should be set to an appropriate duration that allows
normal execution while protecting against hangs, and consider wrapping the call
in a try-except block to handle subprocess.TimeoutExpired exceptions gracefully.
In `@tests/airflow/test_dag_parse.py`:
- Around line 85-86: Replace the direct os.environ mutations for SF_PROJECT_DIR
and SF_MODEL with monkeypatch.setenv calls to prevent environment variable state
leakage across tests. The monkeypatch fixture (which should be a parameter to
the test function) provides automatic cleanup, so instead of
os.environ["SF_PROJECT_DIR"] = str(project_dir), use
monkeypatch.setenv("SF_PROJECT_DIR", str(project_dir)), and similarly for the
SF_MODEL environment variable.
---
Outside diff comments:
In `@tests/airflow/test_dag_parse.py`:
- Around line 71-95: The test function test_generate_task_runs_live_against_demo
is missing required external-service markers that indicate it depends on
Anthropic, BigQuery, and subprocess execution. Add pytest.mark decorators with
the markers e2e, anthropic, bigquery, and cli_subprocess above the function
definition (before the def line) to properly gate this test and ensure it is
excluded from local runs and marked for special handling in CI as per the
project's testing guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9ceae1e-7039-4523-b7a0-7c22ecfc37e6
📒 Files selected for processing (6)
CHANGELOG.mddocs/airflow-ops.mddocs/research/airflow-test-environment.mdexamples/airflow/signalforge_generate_dag.pymkdocs.ymltests/airflow/test_dag_parse.py
✅ Files skipped from review due to trivial changes (3)
- mkdocs.yml
- CHANGELOG.md
- docs/research/airflow-test-environment.md
- example DAG: bound the `signalforge` subprocess with timeout=3600 (+ raise AirflowFailException on TimeoutExpired) so a hung CLI can't tie up a worker. - example DAG: validate signalforge_max_flagged / SF_MAX_FLAGGED as int, raising a clear AirflowFailException instead of a raw ValueError. - live test: use monkeypatch.setenv instead of os.environ mutation (no env leak).
Owner
Author
PR Review Summary (round 2)CodeRabbit follow-up on the fix commit — 3 quick wins, all fixed in Fixed (3 items)
False Positives (0)None. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/airflow/test_dag_parse.py`:
- Around line 70-72: The test function test_generate_task_runs_live_against_demo
is missing required gated markers for its external service and subprocess
execution requirements. Add the appropriate pytest markers as decorators above
the function definition in addition to the existing airflow marker. Since this
is a live test that exercises external services and subprocess execution,
include markers such as cli_subprocess and e2e to properly gate the test
according to the coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7b8ec07-a750-4d6e-9dfb-a0053fa9ca95
📒 Files selected for processing (2)
examples/airflow/signalforge_generate_dag.pytests/airflow/test_dag_parse.py
Add external-service + subprocess gated markers (e2e/anthropic/bigquery/ cli_subprocess) to the live Airflow test per testing-signal.md's marker convention. Applied per-function so the parse test stays airflow-only; the `-m airflow` CI job still selects the live test (it self-skips without the env).
Owner
Author
PR Review Summary (round 3)CodeRabbit follow-up — 1 quick win, fixed. No false positives. Fixed (1 item)
False Positives (0)None. |
This was referenced Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #229. First child of epic #228 — establishes the local + CI test environment every later Airflow child certifies against, and ships the first user-facing Airflow surface (example DAG + ops docs).
What this ships
docs/research/airflow-test-environment.md— decision record (DEC-1…DEC-5): floorapache-airflow>=2.8,<3certified at 2.10.4 / py3.11;[airflow]extra under the constraints file; gatedairflowpytest marker; label-gated CI job; fake-first split + one live e2e DAG; constraints-pinned install + env-var contract.examples/airflow/signalforge_generate_dag.py— shipped example DAG.PythonOperatorwrappingsignalforge generate: four-tier exit-code → task-state, graded tier counts → XCom, agatetask that fails on a configurable flagged-count threshold. Config via Airflow Variables / env. (DedicatedSignalForgeGenerateOperatoris a roadmap drop-in swap — same result→task-state + XCom contract.)docs/airflow-ops.md+ MkDocs nav — install-under-constraints, the example walkthrough, the contract, gating, live-run env gates.pyproject.toml— gatedairflowmarker (excluded from defaultaddopts);tests/airflowexcluded from pyright (Airflow isn't a typecheck dep)..github/workflows/ci.yml— label-gatedairflowCI job (sibling tolint-test, not in the default 3.11–3.13 matrix; runs on PRs labelledairfloworworkflow_dispatch). Theif:is guarded behind an explicitpull_requestevent.tests/airflow/test_dag_parse.py— gated parse test + a gated livedag.test()-style test (self-skips withoutSF_RUN_AIRFLOW+ Anthropic/BigQuery gates).importorskipis inside the tests so a defaultpytestnever imports Airflow..gitignore—.venv-airflow/+.airflow-home/scratch.Certification
apache-airflow==2.10.4resolves cleanly underconstraints-2.10.4/constraints-3.11.txtin an isolated venv; signalforge editable install carries--constraint(else protobuf 4→5 risks breaking Airflow).airflowjob).generatetask ran the real pipeline against aninit-demoproject on real BigQuery + Anthropic (1 passed in 125s).Validation
ruff·ruff format·pyright(0 errors) ·pytest(3570 passed) ·mkdocs build— all green. CI:lint-test×3,airflow,docs-buildall success.Out of scope (later epic #228 children)
The
[airflow]extra +signalforge.airflowpackage skeleton; dedicated operators; result→task-state/XCom operator; connection hook; drift mode.Summary by CodeRabbit