[FEATURE] FDS persist `DataAsset` to YAML file immediately on creation #7705

Kilo59 · 2023-04-24T13:05:12Z

Changes proposed in this pull request:

Call context._save_project_config() after adding a fluent DataAsset
- This saves the current asset to the great_expectations.yml
Call context._save_project_config() after .delete_asset() is called.
Changed default Datasource._data_context value to be None instead of undefined.
Add custom json encoder for TextClause
override PandasDatasource.dict() to prevent serialization of #ephemeral_pandas_asset named assets
raise a GxSerializationWarning instead of erroring if asset is unserializable.

Note

This should only affect FileDataContexts.
Update for CloudDataContext will follow.

Definition of Done

My code follows the Great Expectations style guide
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added unit tests where applicable and made sure that new and existing tests are passing.
I have run any local integration tests and made sure that nothing is broken.

netlify · 2023-04-24T13:05:18Z

✅ Deploy Preview for niobium-lead-7998 canceled.

Name	Link
🔨 Latest commit	`637d6e6`
🔍 Latest deploy log	https://app.netlify.com/sites/niobium-lead-7998/deploys/644814094d20ad00083c5ac3

ghost · 2023-04-24T13:07:05Z

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

Legend

tests/datasource/fluent/test_contexts.py

great_expectations/datasource/fluent/interfaces.py

Kilo59 · 2023-04-24T14:06:37Z

great_expectations/datasource/fluent/interfaces.py

@@ -398,7 +398,7 @@ class Datasource(
    assets: MutableSequence[_DataAssetT] = []

    # private attrs
-    _data_context: GXDataContext = pydantic.PrivateAttr()
+    _data_context: Optional[GXDataContext] = pydantic.PrivateAttr(None)


Updated the type here to reflect that this isn't guaranteed to be set, and the default value to be None instead of the attribute being undefined.

Generally, it is set but we've experienced some bugs due to it not being set and we also have some unit-tests where it isn't set.

great_expectations/datasource/fluent/interfaces.py

billdirks

Thanks for doing this. Added 2 small comments.

great_expectations/datasource/fluent/interfaces.py

billdirks · 2023-04-24T16:26:13Z

great_expectations/datasource/fluent/pandas_datasource.py

@@ -545,7 +545,8 @@ def _validate_asset_name(asset_name: Optional[str] = None) -> str:

    def _get_validator(self, asset: _PandasDataAsset) -> Validator:
        batch_request: BatchRequest = asset.build_batch_request()
-        return self._data_context.get_validator(batch_request=batch_request)
+        # TODO: raise error if `_data_context` not set
+        return self._data_context.get_validator(batch_request=batch_request)  # type: ignore[union-attr] # self._data_context must be set


Maybe _data_context should be a property or we have a private get_validator method on this datasource that will raise an error for us in a consistent way.

That sounds like a good idea but I'd like to defer implementing that in a different PR.
This PR doesn't make this particular error any more or less likely, it just makes mypy better understand this issue.

… type error

alexsherstinsky

Looks good. Should going through the various parts of the codebase (in particular new fluent tests, such as the fixtures for FDS Checkpoint tests) and removing context. _save_context_project_config() -- where it is no longer necessary -- be part of this work? Thanks.

Kilo59 · 2023-04-25T14:10:48Z

@alexsherstinsky I don't see this method being called unnecessarily in our tests.

alexsherstinsky

LGTM!

* develop: [MAINTENANCE] add warning messages when using CLI to edit an expectaiton suite if fluent datasources are present (#7714) [MAINTENANCE] fix get available data assets names for fds (#7723) [MAINTENANCE] Minor stylistic cleanup (#7732) [DOCS] Update for fluent datasources: Create a new Checkpoint (#7729) [MAINTENANCE] Iterate over the regex_pattern characters too in (#7720) [MAINTENANCE] Add CLI warnings when adding a checkpoint with fluent datasources (#7685) [DOCS] Update batch glossary docs. (#7726) [BUGFIX] Add missing pyspark reference (#7684) [FEATURE] FDS persist `DataAsset` to YAML file immediately on creation (#7705)

remove cloud marker

272dbad

Kilo59 self-assigned this Apr 24, 2023

github-actions bot added the core label Apr 24, 2023

Kilo59 changed the title ~~[FEATURE] Persist asset on creation~~ [FEATURE] FDS persist DataAsset immediately on creation Apr 24, 2023

persist asset immediately TDD test

581f889

Kilo59 force-pushed the f/lakitu-104/persist-asset-on-add branch from b6ccbb4 to 581f889 Compare April 24, 2023 13:10

Kilo59 added 3 commits April 24, 2023 09:15

_save_project_config() at end of _add_asset() call

4e42d51

test for deletion

18d531a

_save_project_config() on delete_asset()

1c1acf6

Kilo59 marked this pull request as ready for review April 24, 2023 13:31

Kilo59 requested review from billdirks, NathanFarmer and cdkini April 24, 2023 13:32

NathanFarmer approved these changes Apr 24, 2023

View reviewed changes

tests/datasource/fluent/test_contexts.py Outdated Show resolved Hide resolved

Kilo59 added 3 commits April 24, 2023 09:44

make a valid query

83431ac

make query valid

c89289c

use hasattr

95fd281

Kilo59 changed the title ~~[FEATURE] FDS persist DataAsset immediately on creation~~ [FEATURE] FDS persist DataAsset to YAML file immediately on creation Apr 24, 2023

account for _data_context being None

d504b12

Kilo59 commented Apr 24, 2023

View reviewed changes

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

Kilo59 commented Apr 24, 2023

View reviewed changes

Kilo59 enabled auto-merge (squash) April 24, 2023 14:09

github-actions bot and others added 4 commits April 24, 2023 14:22

Merge develop into f/lakitu-104/persist-asset-on-add

d17453c

use Union instead of Optional

e547f66

_data_context could be None

8af2c1a

ignore _data_context could be None error

d9753fd

Kilo59 commented Apr 24, 2023

View reviewed changes

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

Kilo59 requested a review from alexsherstinsky April 24, 2023 15:58

billdirks reviewed Apr 24, 2023

View reviewed changes

Merge develop into f/lakitu-104/persist-asset-on-add

9b9ad06

Kilo59 disabled auto-merge April 24, 2023 18:02

Kilo59 added 4 commits April 24, 2023 14:18

FakeFilesystem needs to create context.root_directory

1a4652b

Merge branch 'develop' into f/lakitu-104/persist-asset-on-add

2003f7c

_save_context_project_config() method

b622c6e

throw a GxSerializationWarning if cannot save_project_config due to…

a76a4a8

… type error

alexsherstinsky suggested changes Apr 25, 2023

View reviewed changes

Kilo59 requested a review from alexsherstinsky April 25, 2023 14:11

custom TextClause json encoder

ea1883f

alexsherstinsky approved these changes Apr 25, 2023

View reviewed changes

Kilo59 added 7 commits April 25, 2023 11:43

TDD test for ephemeral asset serialization

6f854c6

working impl of "#ephemeral_pandas_asset" serialization exclusion

96b108a

Merge branch 'develop' into f/lakitu-104/persist-asset-on-add

5880637

cleanup

b9ea232

better explanation

a0c5af2

Merge branch 'develop' into f/lakitu-104/persist-asset-on-add

af96225

PUBLIC_API_MISSING_THRESHOLD = 93

84e5612

Kilo59 enabled auto-merge (squash) April 25, 2023 17:06

Merge develop into f/lakitu-104/persist-asset-on-add

637d6e6

Kilo59 merged commit 59f5133 into develop Apr 25, 2023
47 checks passed

Kilo59 deleted the f/lakitu-104/persist-asset-on-add branch April 25, 2023 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] FDS persist `DataAsset` to YAML file immediately on creation #7705

[FEATURE] FDS persist `DataAsset` to YAML file immediately on creation #7705

Kilo59 commented Apr 24, 2023 •

edited

netlify bot commented Apr 24, 2023 •

edited

ghost commented Apr 24, 2023 •

edited by ghost

Legend

Kilo59 Apr 24, 2023 •

edited

billdirks left a comment

billdirks Apr 24, 2023

Kilo59 Apr 25, 2023

alexsherstinsky left a comment

Kilo59 commented Apr 25, 2023 •

edited

alexsherstinsky left a comment

[FEATURE] FDS persist DataAsset to YAML file immediately on creation #7705

[FEATURE] FDS persist DataAsset to YAML file immediately on creation #7705

Conversation

Kilo59 commented Apr 24, 2023 • edited

Changes proposed in this pull request:

Note

Definition of Done

netlify bot commented Apr 24, 2023 • edited

✅ Deploy Preview for niobium-lead-7998 canceled.

ghost commented Apr 24, 2023 • edited by ghost

Legend

Kilo59 Apr 24, 2023 • edited

Choose a reason for hiding this comment

billdirks left a comment

Choose a reason for hiding this comment

billdirks Apr 24, 2023

Choose a reason for hiding this comment

Kilo59 Apr 25, 2023

Choose a reason for hiding this comment

alexsherstinsky left a comment

Choose a reason for hiding this comment

Kilo59 commented Apr 25, 2023 • edited

alexsherstinsky left a comment

Choose a reason for hiding this comment

[FEATURE] FDS persist `DataAsset` to YAML file immediately on creation #7705

[FEATURE] FDS persist `DataAsset` to YAML file immediately on creation #7705

Kilo59 commented Apr 24, 2023 •

edited

netlify bot commented Apr 24, 2023 •

edited

ghost commented Apr 24, 2023 •

edited by ghost

Kilo59 Apr 24, 2023 •

edited

Kilo59 commented Apr 25, 2023 •

edited