[BUGFIX] Fluent `PandasFilesytemDatasources` data_connector fixes #7414

Kilo59 · 2023-03-21T14:23:36Z

Proof of concept for "fixing" the DataConnector abstraction

Changes proposed in this pull request:

implement _build_data_connector
build data_connectors when loading from config
- test for use of data_connectors after loading from config
add connect_options field
pipe through connect_options field to where it's needed
remove manual add_<ASSET_TYPE>_asset() methods
~~update stubs~~ done in [MAINTENANCE] PandasFilesystemDatasource stubs and Schema corrections #7428

Notes

These changes are initially only for PandasFilesytemDatasources. Followup PR's re-implement this pattern for the remaining datasources which use `DataConnectors. This will mostly entail deleting code 😄 .

netlify · 2023-03-21T14:23:44Z

✅ Deploy Preview for niobium-lead-7998 canceled.

Name	Link
🔨 Latest commit	`28ea231`
🔍 Latest deploy log	https://app.netlify.com/sites/niobium-lead-7998/deploys/641c82381bc54d0007ead4c6

ghost · 2023-03-21T14:25:28Z

👇 Click on the image for a new way to code review

Legend

great_expectations/datasource/fluent/file_path_data_asset.py

alexsherstinsky · 2023-03-21T15:59:26Z

great_expectations/datasource/fluent/interfaces.py

@@ -350,6 +351,8 @@ class Datasource(

    # class attrs
    asset_types: ClassVar[Sequence[Type[DataAsset]]] = []
+    # Not all Datasources require a DataConnector
+    data_connector_type: ClassVar[Optional[Type[DataConnector]]] = None


@Kilo59 If this addition is toward including DataConnector in Fluent configuration, then we are not supposed to move in that direction (because in Fluent, DataConnector is an implementational utility, not a core concept). If this is for some other purpose, then please let me know and I will study more carefully. Thanks.

No, it's about ensuring we don't need DataConnector in the configuration.
At least directly. We do need to be able to provide options that the data connector can ingest in config (even if we don't call it a data_connector). But before this change you could not provide any asset level data_connector params in config (glob_directive, prefix, delimiter, etc.)

For my edification: Sounds like if a property is None, it will not end up in Configuration? Or if this is incorrect, how does one specify a property "to be part of configuration" and "to not be part of configuration"? Thanks.

We check this class attribute to know whether or not the Datasource and data assets need to utilize a data_connector. If it does, we need to build that specific DataConnector with this attribute.

alexsherstinsky · 2023-03-21T16:04:58Z

great_expectations/datasource/fluent/interfaces.py

@@ -508,7 +517,7 @@ def parse_batching_regex_string(
    ) -> re.Pattern:
        pattern: re.Pattern
        if not batching_regex:
-            pattern = re.compile(".*")
+            pattern = MATCH_ALL_PATTERN


Can we move MATCH_ALL_PATTERN to the default argument and remove this if branch? That is def parse_batching_regex_string(batching_regex: Union[re.Pattern, str] = MATCH_ALL_PATTERN).

Yes we can! 😄
But some of the other Datasource types rely on it. Once they've all been updated to follow the pattern in this PR we can remove it.

great_expectations/datasource/fluent/interfaces.py

great_expectations/datasource/fluent/sources.py

great_expectations/datasource/fluent/interfaces.py

great_expectations/datasource/fluent/pandas_filesystem_datasource.py

…ion_keys`

alexsherstinsky

@Kilo59 Thank you for this exploratory work. I identified one action item and raised a number of questions. Happy to discuss. Thanks!

Kilo59 · 2023-03-23T01:58:32Z

tests/datasource/fluent/test_metadatasource.py

@@ -528,7 +528,6 @@ def test_update_datasource_with_datasource_object(
                    "csv_asset": {
                        "batching_regex": "(?P<file_name>.*).csv",
                        "name": "csv_asset",
-                        "order_by": [],


This was present because this value was being set in the manually defined add_csv_asset() method.
Fields that are not set should be excluded from serialization, so this is the desired behavior.

NathanFarmer

✅ following our synchronous review

great_expectations/data_context/data_context/abstract_data_context.py

alexsherstinsky · 2023-03-23T16:02:58Z

great_expectations/datasource/fluent/data_asset/data_connector/data_connector.py

@@ -40,6 +40,10 @@ class DataConnector(ABC):
        data_asset_name: The name of the DataAsset using this DataConnector instance
    """

+    # needed to select the asset level kwargs needed to build the DataConnector
+    asset_level_option_keys: ClassVar[tuple[str, ...]] = ()
+    asset_options_type: ClassVar[Type] = dict


@Kilo59 If possible, could you please provide a rich docstring for asset_options_type and asset_level_option_keys here in the top-level DataConnector module? The reason for this request is that whenever a reader sees a class variable, which does not appear to be used anywhere in the present module, the experience of interpreting the code slows down appreciably. We have an analogous situation in the Metrics component (I have made some effort at documenting the class variables used, but a lot more is in order). On a separate thought, is the class variable the best way of achieving our goal here? Could this be an indicator that a better / cleaner design is lurking somewhere and we just have not yet nailed it? Thank you.

Yes, I agree this needs to be well explained.

Unfortunately, we do have to do something like this (even if not exactly this) if we want to preserve the simultaneous goals of...

No exponential explosion of DataAsset classes.

Auto-complete for specific DataAsset Dataconnector connect options.

alexsherstinsky · 2023-03-23T16:03:52Z

great_expectations/datasource/fluent/data_asset/data_connector/filesystem_data_connector.py

@@ -33,6 +39,9 @@ class FilesystemDataConnector(FilePathDataConnector):
        file_path_template_map_fn: Format function mapping path to fully-qualified resource on filesystem (optional)
    """

+    asset_level_option_keys: ClassVar[tuple[str, ...]] = ("glob_directive",)
+    asset_options_type: ClassVar[Type[FilesystemOptions]] = FilesystemOptions


@Kilo59 Yes, at least documenting for how asset_level_option_keys and asset_level_option_keys will be utilized would be nice (here, rather than have to search for it elsewhere). Thanks.

great_expectations/datasource/fluent/file_path_data_asset.py

alexsherstinsky · 2023-03-23T16:06:41Z

great_expectations/datasource/fluent/interfaces.py

@@ -354,6 +354,8 @@ class Datasource(

    # class attrs
    asset_types: ClassVar[Sequence[Type[DataAsset]]] = []
+    # Not all Datasources require a DataConnector


@Kilo59 Side comment -- I suspect that down the road all Datasource would do well by utilizing some kind of a DataConnector in order to achieve simpler, cleaner, and consistent method interfaces. Thanks.

alexsherstinsky · 2023-03-23T16:08:50Z

great_expectations/datasource/fluent/interfaces.py

@@ -462,7 +464,9 @@ def get_asset(self, asset_name: str) -> _DataAssetT:
                f"'{asset_name}' not found. Available assets are {list(self.assets.keys())}"
            ) from exc

-    def add_asset(self, asset: _DataAssetT) -> _DataAssetT:
+    def add_asset(
+        self, asset: _DataAssetT, connect_options: dict | None = None


@Kilo59 Unless it is available elsewhere, some documentation and/or examples about connect_options would be great. Thanks.

great_expectations/datasource/fluent/interfaces.py

great_expectations/datasource/fluent/sources.py

alexsherstinsky · 2023-03-23T16:17:27Z

tests/datasource/fluent/great_expectations.yml

@@ -27,6 +27,31 @@ fluent_datasources:
        order_by:
          - year
          - -month
+  sqlite_taxi:


❤️ Nice new use cases in the configuration! Thanks!

alexsherstinsky

@Kilo59 Thank you for this thrust for a massive improvement. I spotted an oddity, requested docstrings, and asked some questions for my own understanding. Happy to discuss! Thanks!

great_expectations/datasource/fluent/sources.py

alexsherstinsky · 2023-03-23T17:19:25Z

great_expectations/datasource/fluent/pandas_filesystem_datasource.py

+        self, data_asset: _FilePathDataAsset, glob_directive: str = "**/*", **kwargs
+    ) -> None:
+        """Builds and attaches the `FilesystemDataConnector` to the asset."""
+        if kwargs:


❤️ I like this technique!

alexsherstinsky

LGTM -- awesome! Thanks! P.S.: Really looking forward to making time to disentangle the DataConnector now covering both connecting to data and partitioning the assets).

Kilo59 added 5 commits March 21, 2023 09:24

dc_options field

9ebb0ec

add data_connector_type as Datasource ClassVar

77d0cf1

call _build_data_connector during _add_asset()

ac5f51e

PandasFilesystemDatasource implement _build_data_connector()

f2d5b9e

add note about _add_asset_factory()

5c91192

github-actions bot added core-team platform labels Mar 21, 2023

Kilo59 self-assigned this Mar 21, 2023

Kilo59 added 2 commits March 21, 2023 10:42

MATCH_ALL_PATTERN constant

0ecabdb

give the batching_regex field a default

8c049e7

Kilo59 commented Mar 21, 2023

View reviewed changes

great_expectations/datasource/fluent/file_path_data_asset.py Show resolved Hide resolved

Kilo59 requested review from NathanFarmer and talagluck March 21, 2023 15:09

alexsherstinsky reviewed Mar 21, 2023

View reviewed changes

great_expectations/datasource/fluent/file_path_data_asset.py Outdated Show resolved Hide resolved

alexsherstinsky reviewed Mar 21, 2023

View reviewed changes

great_expectations/datasource/fluent/interfaces.py Show resolved Hide resolved

alexsherstinsky reviewed Mar 21, 2023

View reviewed changes

great_expectations/datasource/fluent/sources.py Outdated Show resolved Hide resolved

alexsherstinsky reviewed Mar 21, 2023

View reviewed changes

great_expectations/datasource/fluent/interfaces.py Show resolved Hide resolved

alexsherstinsky reviewed Mar 21, 2023

View reviewed changes

great_expectations/datasource/fluent/pandas_filesystem_datasource.py Show resolved Hide resolved

extract dc_options from kwargs using `DataConnector.asset_level_opt…

94bead9

…ion_keys`

alexsherstinsky suggested changes Mar 21, 2023

View reviewed changes

Kilo59 added 5 commits March 21, 2023 12:38

keyword only arguments

e61b6dc

cleanup comments

4116156

validate_data_connector_options

0692b2b

fix dc_options kwarg selection

6f02e4c

better tests

4e3580a

Kilo59 changed the title ~~[BUGFIX/FEATURE??] Fluent Datasources - DataConnector usage update~~ [BUGFIX/FEATURE??] Fluent Datasources - DataConnector abstraction update Mar 22, 2023

Kilo59 commented Mar 23, 2023

View reviewed changes

Kilo59 added 9 commits March 22, 2023 22:09

add connect options to example configs

0050116

Merge branch 'develop' into f/great-1750/dc-design

9455009

Merge branch 'develop' into f/great-1750/dc-design

feda0ab

cleanup comments

f8b8cb1

from_all_config parametrized fixture

5d61622

fluent_gx_config_yml fixture

45611e1

test dc datasources when loaded from config

ab55627

working dc tests

b6166f8

cleanup tests

87019e9

NathanFarmer approved these changes Mar 23, 2023

View reviewed changes

Kilo59 enabled auto-merge (squash) March 23, 2023 15:37

alexsherstinsky reviewed Mar 23, 2023

View reviewed changes

great_expectations/data_context/data_context/abstract_data_context.py Show resolved Hide resolved

alexsherstinsky reviewed Mar 23, 2023

View reviewed changes

great_expectations/datasource/fluent/file_path_data_asset.py Show resolved Hide resolved

alexsherstinsky reviewed Mar 23, 2023

View reviewed changes

great_expectations/datasource/fluent/interfaces.py Show resolved Hide resolved

alexsherstinsky reviewed Mar 23, 2023

View reviewed changes

great_expectations/datasource/fluent/sources.py Outdated Show resolved Hide resolved

alexsherstinsky reviewed Mar 23, 2023

View reviewed changes

Merge develop into f/great-1750/dc-design

855f2ba

alexsherstinsky suggested changes Mar 23, 2023

View reviewed changes

Kilo59 commented Mar 23, 2023

View reviewed changes

great_expectations/datasource/fluent/sources.py Outdated Show resolved Hide resolved

Apply suggestions from code review

28ea231

alexsherstinsky reviewed Mar 23, 2023

View reviewed changes

alexsherstinsky approved these changes Mar 23, 2023

View reviewed changes

Kilo59 merged commit 492fb1e into develop Mar 23, 2023

Kilo59 deleted the f/great-1750/dc-design branch March 23, 2023 17:40

Kilo59 mentioned this pull request Mar 23, 2023

[BUGFIX] Fluent Datasource load from config fixes for remaining Pandas Datasources #7442

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX] Fluent `PandasFilesytemDatasources` data_connector fixes #7414

[BUGFIX] Fluent `PandasFilesytemDatasources` data_connector fixes #7414

Kilo59 commented Mar 21, 2023 •

edited

Loading

netlify bot commented Mar 21, 2023 •

edited

Loading

ghost commented Mar 21, 2023 •

edited by ghost

Loading

Legend

alexsherstinsky Mar 21, 2023

Kilo59 Mar 21, 2023 •

edited

Loading

alexsherstinsky Mar 23, 2023

Kilo59 Mar 23, 2023

alexsherstinsky Mar 21, 2023

billdirks Mar 22, 2023

Kilo59 Mar 22, 2023 •

edited

Loading

alexsherstinsky left a comment

Kilo59 Mar 23, 2023 •

edited

Loading

NathanFarmer left a comment

alexsherstinsky Mar 23, 2023

Kilo59 Mar 23, 2023

alexsherstinsky Mar 23, 2023 •

edited

Loading

alexsherstinsky Mar 23, 2023

alexsherstinsky Mar 23, 2023

alexsherstinsky Mar 23, 2023

alexsherstinsky left a comment

alexsherstinsky Mar 23, 2023

alexsherstinsky left a comment

[BUGFIX] Fluent PandasFilesytemDatasources data_connector fixes #7414

[BUGFIX] Fluent PandasFilesytemDatasources data_connector fixes #7414

Conversation

Kilo59 commented Mar 21, 2023 • edited Loading

Changes proposed in this pull request:

Notes

netlify bot commented Mar 21, 2023 • edited Loading

✅ Deploy Preview for niobium-lead-7998 canceled.

ghost commented Mar 21, 2023 • edited by ghost Loading

Legend

Choose a reason for hiding this comment

Kilo59 Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kilo59 Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

alexsherstinsky left a comment

Choose a reason for hiding this comment

Kilo59 Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

NathanFarmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsherstinsky Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsherstinsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsherstinsky left a comment

Choose a reason for hiding this comment

[BUGFIX] Fluent `PandasFilesytemDatasources` data_connector fixes #7414

[BUGFIX] Fluent `PandasFilesytemDatasources` data_connector fixes #7414

Kilo59 commented Mar 21, 2023 •

edited

Loading

netlify bot commented Mar 21, 2023 •

edited

Loading

ghost commented Mar 21, 2023 •

edited by ghost

Loading

Kilo59 Mar 21, 2023 •

edited

Loading

Kilo59 Mar 22, 2023 •

edited

Loading

Kilo59 Mar 23, 2023 •

edited

Loading

alexsherstinsky Mar 23, 2023 •

edited

Loading