Skip to content

Commit

Permalink
Default to None for dagster_sling_translator in sling_assets (#21340)
Browse files Browse the repository at this point in the history
## Summary & Motivation

Right now the `dagster_sling_translator` argument of `sling_assets` defaults to `DagsterSlingTranslator()`. Per https://docs.python-guide.org/writing/gotchas/ a single object instance will be shared across *all* invocations of that function. Even if technically the current object is immutable or has no state this is Python and people monkeypatch all the time. Additionally, if someone changes these objects in the future and aren't aware of this usage, it will introduce subtle bugs later. We should instead do the safe, obvious thing and create a new instance on every invocation.

## How I Tested These Changes

BK
  • Loading branch information
schrockn authored Apr 23, 2024
1 parent dc7c564 commit 7b34aba
Showing 1 changed file with 9 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
AssetSpec,
BackfillPolicy,
PartitionsDefinition,
_check as check,
multi_asset,
)
from dagster._utils.security import non_secure_md5_hash_str
Expand All @@ -29,7 +30,7 @@ def get_streams_from_replication(
def sling_assets(
*,
replication_config: SlingReplicationParam,
dagster_sling_translator: DagsterSlingTranslator = DagsterSlingTranslator(),
dagster_sling_translator: Optional[DagsterSlingTranslator] = None,
name: Optional[str] = None,
partitions_def: Optional[PartitionsDefinition] = None,
backfill_policy: Optional[BackfillPolicy] = None,
Expand Down Expand Up @@ -80,6 +81,13 @@ def my_assets(context, sling: SlingResource):
streams = get_streams_from_replication(replication_config)
code_version = non_secure_md5_hash_str(str(replication_config).encode())

dagster_sling_translator = (
check.opt_inst_param(
dagster_sling_translator, "dagster_sling_translator", DagsterSlingTranslator
)
or DagsterSlingTranslator()
)

return multi_asset(
name=name,
compute_kind="sling",
Expand Down

0 comments on commit 7b34aba

Please sign in to comment.