Fix DagsterInvalidInvocationError when checking size, emptiness, or membership of historical entity subsets with deleted partitions#33911
Conversation
Greptile SummaryThis PR adds
Confidence Score: 3/5The The changed
|
| Filename | Overview |
|---|---|
| python_modules/dagster/dagster/_core/asset_graph_view/serializable_entity_subset.py | Adds DagsterInvalidInvocationError guards to size, is_empty, and __contains__; changes __contains__ type signature from AssetKeyPartitionKey to a non-existent AssetSubset type, and breaks the non-partitioned branch by calling item.bool_value on callers that pass AssetKeyPartitionKey. |
| python_modules/dagster/dagster_tests/core_tests/asset_graph_view_tests/test_serializable_entity_subset.py | Adds regression test using monkeypatch for size and is_empty fallbacks; test for __contains__ doesn't actually trigger the DagsterInvalidInvocationError fallback path since DefaultPartitionsSubset.__contains__ is not mocked. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Caller: item in SerializableEntitySubset"] --> B{is_partitioned?}
B -- "No (non-partitioned)" --> C["NEW: self.bool_value and item.bool_value"]
C --> D{"item has .bool_value?"}
D -- "AssetSubset ✓" --> E["Returns bool"]
D -- "AssetKeyPartitionKey ✗" --> F["AttributeError 💥"]
B -- "Yes (partitioned)" --> G["try: item.partition_key in self.subset_value"]
G --> H{"DagsterInvalidInvocationError?"}
H -- "No" --> I["Returns bool ✓"]
H -- "Yes (deleted partition)" --> J["Returns False ✓"]
Reviews (2): Last reviewed commit: "Fix DagsterInvalidInvocationError when r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| broken_subset.__class__.__len__ = lambda self: (_ for _ in ()).throw( | ||
| DagsterInvalidInvocationError("Nonexistent partition keys") | ||
| ) | ||
|
|
||
| type(broken_subset).is_empty = property( | ||
| lambda self: (_ for _ in ()).throw( | ||
| DagsterInvalidInvocationError("Nonexistent partition keys") | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Both broken_subset.__class__.__len__ = ... and type(broken_subset).is_empty = property(...) modify DefaultPartitionsSubset at the class level — not just on the instance. Because there is no teardown, the overridden __len__ and is_empty remain in place for every subsequent test in the same process that instantiates a DefaultPartitionsSubset. Any test that later calls len() or .is_empty on a static-partition subset will raise DagsterInvalidInvocationError instead of returning the real value, producing false failures with misleading error messages.
The fix is to use pytest's monkeypatch fixture, which automatically restores the original attributes after the test exits.
| assert entity_subset.size == 0 | ||
| assert entity_subset.is_empty is True No newline at end of file |
There was a problem hiding this comment.
| assert entity_subset.size == 0 | |
| assert entity_subset.is_empty is True | |
| assert entity_subset.size == 0 | |
| assert entity_subset.is_empty is True |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ets with deleted partitions
imann128
force-pushed
the
fix-deleted-partition-fallback
branch
from
1d56534 to
cf87c27
Compare
imann128
changed the title
| def __contains__(self, item: "AssetSubset") -> bool: | ||
| if not self.is_partitioned: | ||
| return item.asset_key == self.key and item.partition_key is None and self.bool_value | ||
| else: | ||
| return item.asset_key == self.key and item.partition_key in self.subset_value | ||
|
|
||
| return self.bool_value and item.bool_value | ||
|
|
||
| try: | ||
| return item.partition_key in self.subset_value | ||
| except DagsterInvalidInvocationError: | ||
| # If a dynamic partition key was deleted out from under this historical | ||
| # subset evaluation, gracefully treat it as not contained. | ||
| return False |
There was a problem hiding this comment.
AttributeError on non-partitioned __contains__ with existing callers
The non-partitioned branch now evaluates item.bool_value, but the known caller in legacy_context.py (line 369: asset_partition in parent_subset.convert_to_serializable_subset()) passes an AssetKeyPartitionKey, which is a NamedTuple with only asset_key and partition_key fields — no bool_value. Any non-partitioned asset going through will_update_asset_partition will raise AttributeError instead of the previous graceful logic. The old implementation checked item.partition_key is None and self.bool_value, which correctly handled AssetKeyPartitionKey without accessing a non-existent attribute.
1 participant
Summary:
This PR aims to resolve a regression in which reading historical asset records, tracking asset evaluation health, or performing partition membership lookups via GraphQL crashes the user interface or the system tracking.
Specifically, when accessing properties like .size, .is_empty, or evaluating membership checks using the in operator (contains) on a SerializableEntitySubset, Dagster throws a DagsterInvalidInvocationError if an underlying dynamic partition key has been deleted out from under that historical evaluation subset.
By catching DagsterInvalidInvocationError within the size, is_empty, and contains evaluations, we provide a clean, defensive fallback. The system safely treats the broken subset evaluations as having a size of 0, an empty state of True, and membership presence of False rather than crashing the execution path.
What was Changed:
serializable_entity_subset.py: Refactored the size, is_empty, and contains methods to catch DagsterInvalidInvocationError. If caught, they cleanly fall back to 0, True, and False, respectively, instead of crashing the thread.
test_serializable_entity_subset.py: Added test_deleted_partition_graceful_fallback to ensure regression protection by utilizing pytest's monkeypatch fixture to safely mock a native DefaultPartitionsSubset class footprint, simulating a missing/deleted partition exception path without leaking state across the test suite execution.
Test
A unit test (test_deleted_partition_graceful_fallback) has been added to the test suite to ensure structural durability and guard against regression.
The test uses an isolated runtime monkeypatch that dynamically injects a crashing scenario (DagsterInvalidInvocationError) directly into the DefaultPartitionsSubset class's footprint to cleanly simulate dynamic partition deletion. It verifies that:
entity_subset.size returns 0 instead of propagating the error.
entity_subset.is_empty correctly returns True.
Membership evaluation using the in operator gracefully returns False