[MAINTENANCE] Fluent Datasources: Eliminate redundant Datasource name and DataAsset name from dictionary and JSON configuration

[alexsherstinsky]

Scope

• While YAML configuration representations are consistent with object representation in Fluent Datasources object structures and the requirements for YAML view of configuration to be human readable, dictionary and JSON representations have contained duplicated "name" value as the key to object structures. This key is superfluous (and required additional error handling, let alone being logically redundant), because the object structures themselves already contain the "name" attribute and are thus self-contained. The present effort repairs this duplication.
• Additional convenience methods are implemented in GxConfig and Datasource classes (with docstrings included).

Please annotate your PR title to describe what the PR does, then give a brief bulleted description of your PR below. PR titles should begin with [BUGFIX], [FEATURE], [DOCS], or [MAINTENANCE]. If a new feature introduces breaking changes for the Great Expectations API or configuration files, please also add [BREAKING]. You can read about the tags in our contributor checklist.

Changes proposed in this pull request:

• JIRA: GREAT-1709/GREAT-1755

After submitting your PR, CI checks will run and @cla-bot will check for your CLA signature.

For a PR with nontrivial changes, we review with both design-centric and code-centric lenses.

In a design review, we aim to ensure that the PR is consistent with our relationship to the open source community, with our software architecture and abstractions, and with our users' needs and expectations. That review often starts well before a PR, for example in github issues or slack, so please link to relevant conversations in notes below to help reviewers understand and approve your PR more quickly (e.g. closes #123).

Previous Design Review notes:

Definition of Done

Please delete options that are not relevant.

• 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.

Thank you for submitting!

[netlify]

✅ Deploy Preview for niobium-lead-7998 ready!

Name	Link
🔨 Latest commit	c22a5e6
🔍 Latest deploy log	https://app.netlify.com/sites/niobium-lead-7998/deploys/6437252915187f0008997bec
😎 Deploy Preview	https://deploy-preview-7573--niobium-lead-7998.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[ghost]

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

Legend

[Kilo59]

I thought we had agreed we weren't making this change yet.

Having said that, I broadly agree with the change and the way it's implemented.

I still think it would be better to do this in two steps, first by implementing __get_item__/__set_item__,__iter__ on Datasource and use those to get/update and iterate over assets, and then the second PR could purely be about the config representation changes.

I don't like that we are still special casing the yaml.

And I need to make sure this won't interfere with the fluent Cloud work.

Edit

Having spent more time thinking about this I don't understand why we are making this change.
I had initially thought it was because we were removing any of the serialization special casing for YAML. But we are still doing that, in-fact now the YAML special case deviates even more from the internal representation of the objects.

[Kilo59]

I think we need to look before leaping here.
You are appending the asset before checking for a name conflict. If there is a conflict it's too late, the asset has already been appended.

This is where __set_item__ (or another dedicated method) can be useful. It can be responsible for checking your uniqueness and only set the item if it has a unique name.

[alexsherstinsky]

@Kilo59 I will look into it; my feeling is that we should borrow from the best of __set_item__ like you said, but not actually implement this "dunder" method.

[alexsherstinsky]

@Kilo59 I made an update -- please review.

[Kilo59]

This seems like it should be a private method. If we need it at all.

[alexsherstinsky]

@Kilo59 We are calling it from a DataContext subclass. With this fact, is your recommendation the same (change to private)? Or keep as public? Thanks.

[Kilo59]

The great_expectations.yml example hasn't changed.
So we are still special casing the yaml... why?
Our json and dicts now have an asset field that are lists of objects/dicts but our yaml is not a list of objects.
So yaml.load() of our yaml looks like this..

{
    "fluent_datasources": {
        "my_pandas_abs_ds": {
            "assets": {
                "my_csv_asset_w_custom_connect_options": {
                    "connect_options": {
                        "abs_container": "test",
                        "abs_delimiter": "/",
                        "abs_name_starts_with": "",
                    },
                    "delimiter": ",",
                    "type": "csv",
                },
                "my_csv_asset_with_default_connect_options": {
                    "connect_options": {"abs_container": "this_is_always_required"},
                    "delimiter": ",",
                    "name": "my_csv_asset_with_default_connect_options",
                    "type": "csv",
                },
            },
            "name": "my_pandas_abs_ds",
            "type": "pandas_abs",
        },
        "my_pandas_fs_ds": {
            "assets": {
                "my_csv_asset_w_custom_connect_options": {
                    "connect_options": {"glob_directive": "**/*.csv"},
                    "sep": ",",
                    "type": "csv",
                },
                "my_csv_asset_with_default_connect_options": {
                    "sep": ",",
                    "type": "csv",
                },
            },
            "base_directory": ".",
            "name": "my_pandas_fs_ds",
            "type": "pandas_filesystem",
        },
    }
}

But for json/dict it's like this...

{
    "fluent_datasources": {
        "my_pandas_abs_ds": {
            "assets": [
                {
                    "connect_options": {
                        "abs_container": "test",
                        "abs_delimiter": "/",
                        "abs_name_starts_with": "",
                    },
                    "delimiter": ",",
                    "name": "my_csv_asset_w_custom_connect_options",
                    "type": "csv",
                },
                {
                    "connect_options": {"abs_container": "this_is_always_required"},
                    "delimiter": ",",
                    "name": "my_csv_asset_with_default_connect_options",
                    "type": "csv",
                },
            ],
            "name": "my_pandas_abs_ds",
            "type": "pandas_abs",
        },
        "my_pandas_fs_ds": {
            "assets": [
                {
                    "connect_options": {"glob_directive": "**/*.csv"},
                    "name": "my_csv_asset_w_custom_connect_options",
                    "sep": ",",
                    "type": "csv",
                },
                {
                    "name": "my_csv_asset_with_default_connect_options",
                    "sep": ",",
                    "type": "csv",
                },
            ],
            "base_directory": ".",
            "name": "my_pandas_fs_ds",
            "type": "pandas_filesystem",
        },
    }
}

[alexsherstinsky]

@Kilo59 One of the requirements is that YAML is special: YAML should read the way it has been thus far, while JSON and Dict are more object-like. Thanks.

[Kilo59]

I think this warrants further discussion (as a team) before merging.

[alexsherstinsky]

@Kilo59 We had the discussion about this at the Architecture Review meeting earlier today. While everybody understands the problem and the examples, the Team does not have a strong opinion on the format; so we agreed to add @donaldheppner as a reviewer, since @donaldheppner was an originator of the requirement for the YAML and JSON / Dictionary serialization formats to be different for product considerations. Thank you!

[Kilo59]

Why is this changing?

[alexsherstinsky]

@Kilo59 Style -- nothing special here. Just indicating the keys are String-valued.

[Kilo59]

This is a clever way of finding the right datasources to update.

[alexsherstinsky]

@Kilo59 I tried using set() first, but our Datasource and DataAsset structures are non-hashable, so this did not work (that is all right -- I do not think that we should rush to implement dunder hash as using dictionary works well). Thanks.

[donaldheppner]

blocking this PR pending a full review