Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_op_compute_fn #16618

schrockn · 2023-09-19T10:55:04Z

Summary & Motivation

This is a simple refactoring so that I can use this function to implement #16620

How I Tested These Changes

BK

schrockn · 2023-09-19T10:55:17Z

Current dependencies on/for this PR:

master
- PR Source Asset -> Unexecutable Asset Adapter #16617
  - PR Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_op_compute_fn #16618 👈
    - PR Wrap observable source assets in AssetsDefinition #16620
      - PR Add Playground Example folder and Observable source asset test #17063
      - PR Add a new AssetExecutionType (Observable) and use it to omit materializations for observable source asset wrapping #16621
        
        PR @observable_source_asset adapter to generate and SensorDefinition #16716
        
        PR Make observable source adapter layer compatibility with AMP #16718
        
        PR RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16712
        
        PR RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16623
    - PR Wrap observable source assets in AssetsDefinition #16619

This comment was auto-generated by Graphite.

schrockn · 2023-09-19T13:29:02Z

python_modules/dagster/dagster/_core/definitions/source_asset.py

@@ -50,10 +50,87 @@
 from dagster._utils.merger import merge_dicts
 from dagster._utils.warnings import disable_dagster_warnings

+# getting the following error


@smackesey do you have any idea why ruff was firing this error?

I assume if you uncomment [1] and [2] things should work - ruff just wants you to import in the TYPE_CHECKING block if you only use the type as a typehint like in [2]

its unclear to me if [1] is part of the error message you got for a import no longer present, or if you had [1] and commented it out and put the error above it

alangenfeld · 2023-09-19T14:30:44Z

python_modules/dagster/dagster/_core/definitions/source_asset.py

+# if TYPE_CHECKING:
+#     from dagster._core.definitions.decorators.op_decorator import (
+#         DecoratedOpFunction,
+#     )


alangenfeld · 2023-09-19T14:30:49Z

python_modules/dagster/dagster/_core/definitions/source_asset.py

+def wrap_source_asset_observe_fn_in_op_compute_fn(
+    source_asset: "SourceAsset",
+):
+    # ) -> "DecoratedOpFunction":


alangenfeld · 2023-09-19T14:33:06Z

python_modules/dagster/dagster/_core/definitions/source_asset.py

@@ -50,10 +50,87 @@
 from dagster._utils.merger import merge_dicts
 from dagster._utils.warnings import disable_dagster_warnings

+# getting the following error


I assume if you uncomment [1] and [2] things should work - ruff just wants you to import in the TYPE_CHECKING block if you only use the type as a typehint like in [2]

its unclear to me if [1] is part of the error message you got for a import no longer present, or if you had [1] and commented it out and put the error above it

…p_compute_fn This refactoring will be useful in a subsequent PR fix type hecking

…_fn_in_op_compute_fn (#16618)" This reverts commit 4d3369a.

…_fn_in_op_compute_fn (#16618)" (#16688) ## Summary & Motivation This caused bugs caught in manual testing: ``` Could not load location dagster_test.toys.repo to check for sensors due to the following error: TypeError: 'staticmethod' object is not callable Stack Trace: File "/Users/johann/dagster/python_modules/dagster/dagster/_grpc/server.py", line 295, in __init__ self._loaded_repositories: Optional[LoadedRepositories] = LoadedRepositories( File "/Users/johann/dagster/python_modules/dagster/dagster/_grpc/server.py", line 139, in __init__ loadable_targets = get_loadable_targets( File "/Users/johann/dagster/python_modules/dagster/dagster/_grpc/utils.py", line 47, in get_loadable_targets else loadable_targets_from_python_module(module_name, working_directory) File "/Users/johann/dagster/python_modules/dagster/dagster/_core/workspace/autodiscovery.py", line 35, in loadable_targets_from_python_module module = load_python_module( File "/Users/johann/dagster/python_modules/dagster/dagster/_core/code_pointer.py", line 135, in load_python_module return importlib.import_module(module_name) File "/Users/johann/.pyenv/versions/3.9.10/lib/python3.9/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name[level:], package, level) File "<frozen importlib._bootstrap>", line 1030, in _gcd_import File "<frozen importlib._bootstrap>", line 1007, in _find_and_load File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked File "<frozen importlib._bootstrap>", line 680, in _load_unlocked File "<frozen importlib._bootstrap_external>", line 850, in exec_module File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed File "/Users/johann/dagster/python_modules/dagster-test/dagster_test/toys/repo.py", line 201, in <module> def data_versions_repository(): File "/Users/johann/dagster/python_modules/dagster/dagster/_core/definitions/decorators/repository_decorator.py", line 405, in repository return _Repository()(definitions_fn) File "/Users/johann/dagster/python_modules/dagster/dagster/_core/definitions/decorators/repository_decorator.py", line 161, in __call__ else CachingRepositoryData.from_list( File "/Users/johann/dagster/python_modules/dagster/dagster/_core/definitions/repository_definition/repository_data.py", line 346, in from_list return build_caching_repository_data_from_list( File "/Users/johann/dagster/python_modules/dagster/dagster/_core/definitions/repository_definition/repository_data_builder.py", line 195, in build_caching_repository_data_from_list for job_def in get_base_asset_jobs( File "/Users/johann/dagster/python_modules/dagster/dagster/_core/definitions/assets_job.py", line 76, in get_base_asset_jobs for observable in [sa for sa in source_assets if sa.is_observable]: File "/Users/johann/dagster/python_modules/dagster/dagster/_core/definitions/assets_job.py", line 76, in <listcomp> for observable in [sa for sa in source_assets if sa.is_observable]: File "/Users/johann/dagster/python_modules/dagster/dagster/_core/definitions/source_asset.py", line 256, in is_observable return self.node_def is not None File "/Users/johann/dagster/python_modules/dagster/dagster/_core/definitions/source_asset.py", line 270, in node_def compute_fn=wrap_source_asset_observe_fn_in_op_compute_fn(self), ``` Reverting for now ## How I Tested These Changes BK

…_observe_fn_in_op_compute_fn (#16618)" (#16688)" This reverts commit e4337b9.

@johannkm

…p_compute_fn (Take two) (#16699) ## Summary & Motivation This is another shot at #16618 which was reverted in #16688 after @johannkm discovered a bug. It turns out I had spuriously left a `@staticmethod` decoration on wrap_source_asset_observe_fn_in_op_compute_fn which worked fine both locally and in CI. This because this only worked in Python 3.10 and later. https://docs.python.org/3/whatsnew/3.10.html#other-language-changes ``` Static methods (@staticmethod) and class methods (@classmethod) now inherit the method attributes (__module__, __name__, __qualname__, __doc__, __annotations__) and have a new __wrapped__ attribute. Moreover, static methods are now callable as regular functions. ``` We only run 3.10 in most CI now for cost control. However, sometimes we pay the iron price for this optimization. ## How I Tested These Changes Load original PR locally in python 3.9. Confirm error on original PR. Apply this patch. See no error.

schrockn mentioned this pull request Sep 19, 2023

Source Asset -> Unexecutable Asset Adapter #16617

Merged

schrockn mentioned this pull request Sep 19, 2023

Wrap observable source assets in AssetsDefinition #16619

Closed

schrockn force-pushed the source-asset-wrapping-1 branch from dee2615 to ad61240 Compare September 19, 2023 11:01

schrockn force-pushed the source-asset-wrapping-2 branch from c1fea5a to 9b37183 Compare September 19, 2023 11:01

schrockn force-pushed the source-asset-wrapping-1 branch from ad61240 to 2fa51cc Compare September 19, 2023 11:24

schrockn force-pushed the source-asset-wrapping-2 branch 2 times, most recently from 11f8bf6 to e999885 Compare September 19, 2023 12:38

schrockn force-pushed the source-asset-wrapping-1 branch from 808fc37 to 2e3b4a1 Compare September 19, 2023 13:20

schrockn mentioned this pull request Sep 19, 2023

Wrap observable source assets in AssetsDefinition #16620

Merged

schrockn force-pushed the source-asset-wrapping-1 branch from f47b698 to 5aabb85 Compare September 19, 2023 13:25

schrockn requested review from alangenfeld and smackesey September 19, 2023 13:28

schrockn commented Sep 19, 2023

View reviewed changes

schrockn force-pushed the source-asset-wrapping-2 branch from e999885 to 3df7ddc Compare September 19, 2023 13:29

schrockn marked this pull request as ready for review September 19, 2023 13:30

schrockn mentioned this pull request Sep 19, 2023

Add a new AssetExecutionType (Observable) and use it to omit materializations for observable source asset wrapping #16621

Merged

alangenfeld approved these changes Sep 19, 2023

View reviewed changes

schrockn mentioned this pull request Sep 19, 2023

RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16623

Closed

schrockn force-pushed the source-asset-wrapping-1 branch from 5aabb85 to 4e8484e Compare September 20, 2023 03:42

schrockn force-pushed the source-asset-wrapping-2 branch from 3df7ddc to c1afec3 Compare September 20, 2023 03:42

schrockn force-pushed the source-asset-wrapping-1 branch from 4e8484e to 5f71dbf Compare September 20, 2023 20:36

Base automatically changed from source-asset-wrapping-1 to master September 20, 2023 21:19

schrockn added 2 commits September 20, 2023 17:20

Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_o…

7c95b2b

…p_compute_fn This refactoring will be useful in a subsequent PR fix type hecking

ruff

4354aa0

schrockn force-pushed the source-asset-wrapping-2 branch from c1afec3 to 4354aa0 Compare September 20, 2023 21:20

schrockn merged commit 4d3369a into master Sep 21, 2023

schrockn deleted the source-asset-wrapping-2 branch September 21, 2023 13:30

schrockn added a commit that referenced this pull request Sep 21, 2023

Revert "Extract _get_op_def_compute_fn into wrap_source_asset_observe…

fa35eb2

…_fn_in_op_compute_fn (#16618)" This reverts commit 4d3369a.

schrockn mentioned this pull request Sep 21, 2023

Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_op_compute_fn (Take two) #16699

Merged

schrockn added a commit that referenced this pull request Sep 21, 2023

Revert "Revert "Extract _get_op_def_compute_fn into wrap_source_asset…

9bf5afd

…_observe_fn_in_op_compute_fn (#16618)" (#16688)" This reverts commit e4337b9.

This was referenced Sep 22, 2023

RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16712

Closed

@observable_source_asset adapter to generate and SensorDefinition #16716

Closed

Make observable source adapter layer compatibility with AMP #16718

Closed

schrockn mentioned this pull request Oct 6, 2023

Add Playground Example folder and Observable source asset test #17063

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_op_compute_fn #16618

Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_op_compute_fn #16618

schrockn commented Sep 19, 2023 •

edited

Loading

schrockn commented Sep 19, 2023 •

edited

Loading

schrockn Sep 19, 2023

alangenfeld Sep 19, 2023

alangenfeld Sep 19, 2023

alangenfeld Sep 19, 2023

alangenfeld Sep 19, 2023

Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_op_compute_fn #16618

Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_op_compute_fn #16618

Conversation

schrockn commented Sep 19, 2023 • edited Loading

Summary & Motivation

How I Tested These Changes

schrockn commented Sep 19, 2023 • edited Loading

schrockn Sep 19, 2023

Choose a reason for hiding this comment

alangenfeld Sep 19, 2023

Choose a reason for hiding this comment

alangenfeld Sep 19, 2023

Choose a reason for hiding this comment

alangenfeld Sep 19, 2023

Choose a reason for hiding this comment

alangenfeld Sep 19, 2023

Choose a reason for hiding this comment

schrockn commented Sep 19, 2023 •

edited

Loading

schrockn commented Sep 19, 2023 •

edited

Loading