chore: upgrade Dagster stack to 1.13#533
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughPins Dagster dependencies to 1.13 minor range, changes DagsterRuntime.run_id to read from context.run.run_id with tests, makes BundledStackHarness.read_env merge .phlo/.env.local over .phlo/.env, and adds defensive partition_key resolution plus tests in Trino publishing. ChangesDagster integration and dependency updates
BundledStackHarness env merging
Trino publishing partition-key guards
🚥 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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Cairn Quality ReportCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/phlo-dagster/tests/test_integration_dagster.py (1)
136-149: ⚡ Quick winConsider adding test coverage for the fallback path.
The new test validates that
run_idis read fromcontext.run.run_id. To ensure complete coverage of the refactored logic inadapter.py, consider adding a test case wherecontext.run.run_idis not available and the code falls back tocontext.run_id.🧪 Suggested additional test case
def test_dagster_runtime_reads_run_id_fallback(): """DagsterRuntime should fall back to context.run_id when run object lacks run_id.""" from phlo_dagster.adapter import DagsterRuntime context = SimpleNamespace( run=SimpleNamespace(tags={}), # run object exists but no run_id run_id="fallback-run-id", has_partition_key=False, log=SimpleNamespace(), resources=SimpleNamespace(), ) runtime = DagsterRuntime(context=cast(AssetExecutionContext, context)) assert runtime.run_id == "fallback-run-id"🤖 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 `@packages/phlo-dagster/tests/test_integration_dagster.py` around lines 136 - 149, The test suite covers the path where DagsterRuntime reads run_id from context.run.run_id but misses the fallback path; add a new test similar to test_dagster_runtime_reads_run_id_from_run_object that constructs a context with run present but without run_id (e.g., run=SimpleNamespace(tags={})) and with context.run_id set (e.g., "fallback-run-id"), instantiate phlo_dagster.adapter.DagsterRuntime with that context and assert runtime.run_id == "fallback-run-id" to validate the fallback branch in DagsterRuntime.
🤖 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.
Nitpick comments:
In `@packages/phlo-dagster/tests/test_integration_dagster.py`:
- Around line 136-149: The test suite covers the path where DagsterRuntime reads
run_id from context.run.run_id but misses the fallback path; add a new test
similar to test_dagster_runtime_reads_run_id_from_run_object that constructs a
context with run present but without run_id (e.g., run=SimpleNamespace(tags={}))
and with context.run_id set (e.g., "fallback-run-id"), instantiate
phlo_dagster.adapter.DagsterRuntime with that context and assert runtime.run_id
== "fallback-run-id" to validate the fallback branch in DagsterRuntime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b38e08a-ed2d-45ad-b8b6-3fccb04f5e93
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/phlo-dagster/pyproject.tomlpackages/phlo-dagster/src/phlo_dagster/adapter.pypackages/phlo-dagster/tests/test_integration_dagster.pypackages/phlo-testing/pyproject.tomlpyproject.toml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: python / package tests (platform)
- GitHub Check: python / package tests (data)
- GitHub Check: python / core tests (3.11)
- GitHub Check: python / core tests (3.12)
- GitHub Check: frontend / observatory
- GitHub Check: docs / build
- GitHub Check: integration / required suites
🔇 Additional comments (5)
packages/phlo-testing/pyproject.toml (1)
10-11: LGTM!pyproject.toml (2)
45-46: LGTM!
124-125: LGTM!packages/phlo-dagster/pyproject.toml (1)
11-11: Check repo compatibility withdagster-graphql>=1.13,<1.14(Dagster 1.13 breaking GraphQL-adjacent removals).
Dagster 1.13 removed/changed several GraphQL-related APIs/deprecated parameters that may affect this codebase, including:external_asset_from_spec/external_assets_from_specs, deprecateddepssupport as a single-AssetKeyargument (useAssetDepobjects),Definitions.get_all_asset_specs,@observable_source_assetlegacy freshness/auto-observe parameters,AssetsDefinition.legacy_freshness_policies_by_output_name, andComponentLoadContext.load_component_at_path/build_defs_at_path. Scan the repo for these symbols/usages and update call sites if any are present.packages/phlo-dagster/src/phlo_dagster/adapter.py (1)
179-184: Dagster 1.13run_idaccess pattern is aligned.
AssetExecutionContext.context.run_idis deprecated in favour ofcontext.run.run_id;packages/phlo-dagster/src/phlo_dagster/adapter.pynow prefersself.context.run.run_idwhen available and safely falls back toself.context.run_id(returningNoneif neither exists).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/phlo-trino/tests/test_publishing.py (1)
346-353: 💤 Low valueThe current test already forces the
partition_keyaccess; thehas_partition_key=Truetweak is only for clarity.
_resolve_partition_keyonly short-circuits toNonewhencontext.has_partition_key is False. Ifhas_partition_keyis missing (as in_ContextWithProviderError), it still readscontext.partition_key, so theRuntimeErrorpropagates and the “does not swallow” intent is already exercised.🤖 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 `@packages/phlo-trino/tests/test_publishing.py` around lines 346 - 353, Test already triggers partition_key access but is unclear; update the test_resolve_partition_key_does_not_swallow_runtime_specific_errors to make intent explicit by adding a has_partition_key attribute set to True on the _ContextWithProviderError test class so that publishing._resolve_partition_key will not short-circuit and will attempt to read partition_key (thereby letting RuntimeError propagate). Locate the test and the _ContextWithProviderError class and add has_partition_key = True to it so the behavior and intent are unambiguous.
🤖 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.
Nitpick comments:
In `@packages/phlo-trino/tests/test_publishing.py`:
- Around line 346-353: Test already triggers partition_key access but is
unclear; update the
test_resolve_partition_key_does_not_swallow_runtime_specific_errors to make
intent explicit by adding a has_partition_key attribute set to True on the
_ContextWithProviderError test class so that publishing._resolve_partition_key
will not short-circuit and will attempt to read partition_key (thereby letting
RuntimeError propagate). Locate the test and the _ContextWithProviderError class
and add has_partition_key = True to it so the behavior and intent are
unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7a624f2-5df9-454f-bd76-3ad3f04e6fbd
📒 Files selected for processing (4)
packages/phlo-dagster/src/phlo_dagster/adapter.pypackages/phlo-dagster/tests/test_integration_dagster.pypackages/phlo-trino/src/phlo_trino/publishing.pypackages/phlo-trino/tests/test_publishing.py
📜 Review details
🔇 Additional comments (5)
packages/phlo-dagster/src/phlo_dagster/adapter.py (1)
176-179: LGTM!packages/phlo-dagster/tests/test_integration_dagster.py (1)
136-151: LGTM!Also applies to: 160-160
packages/phlo-trino/src/phlo_trino/publishing.py (1)
365-372: LGTM!packages/phlo-trino/tests/test_publishing.py (2)
5-6: LGTM!
303-343: LGTM!
1 participant
Summary
context.run.run_id, avoiding Dagster 1.13 deprecated context accessValidation
uv lock --checkuv run python - <<'PY' ...confirmed dagster, dagster-graphql, dagster-webserver, dagster-pipes, and dagster-shared are all 1.13.7uv run pytest packages/phlo-dagster/tests/test_operations.py tests/runtime/test_orchestrator_selection.py tests/plugins/test_capabilities_runtime.py tests/plugins/test_package_dependency_declarations.pyuv run pytest -m integration packages/phlo-dagster/tests/test_integration_dagster.pyuv run pytest packages/phlo-dagster/testsuv run pytest -m 'not integration'uv run ruff check .uv run ruff format --check .uv run ty check src/phlo packages/phlo-dagster/src packages/phlo-testing/src(completed with existing warn-level diagnostics)