feat: Add metadata attribute to datasets

[AhdraMeraliQB]

Description

Resolves kedro-org/kedro#2440

This is connected to kedro-org/kedro#2537 which adds the metadata attribute to the datasets within kedro.io (MemoryDataSet, LambdaDataSet, PartitionedDataSet)

Also addresses some changes made in #184

Development notes

The metadata attribute is defined solely within self.metadata. Some datasets make use of a _describe method to return a dictionary of the dataset's attributes. I have not included the metadata in these methods as in some instances it would necessitate defining it twice, and I find the use redundant - however I would like to hear the reviewer's opinions on this matter.

Metadata is accessible through dataset_name.metadata.Depending on the implementation this is at times inconsistent within the dataset, but it remains consistent across all datasets.

These changes have been tested manually using both the Python api and through catalog.yml using hooks. The hook I used to access the dataset's metadata is as follows:

from kedro.framework.hooks import hook_impl
from typing import Any, Dict
from kedro.io import DataCatalog, MemoryDataSet

class MetadataHooks:
    @hook_impl
    def after_catalog_created(self,
        catalog: DataCatalog,
        conf_catalog: Dict[str, Any],
        conf_creds: Dict[str, Any],
        feed_dict: Dict[str, Any],
        save_version: str,
        load_versions: Dict[str, str],
    ):
        for k,v in catalog.datasets.__dict__.items():
            print(k + "metadata: \n" + str(v.metadata))

Checklist

• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the relevant RELEASE.md file
• Added tests to cover my changes

[noklam]

Just thinking top of my head, do we need to change this in all the dataset? How was layer being used before without being an argument in DataSet? I think it should be yes.

It would be also great to think about how metadata could be access via hooks, i.e. how will viz using this new field. after_caterlog_create hook I guess?

[AhdraMeraliQB]

How was layer being used before without being an argument in DataSet? I think it should be yes.

layer is currently consumed by the DataCatalog and the general consensus is that this is not ideal. Previously it was defined on every dataset as in this PR

It would be also great to think about how metadata could be access via hooks, i.e. how will viz using this new field. after_caterlog_create hook I guess?

metadata is accessible in the dummy hook implementation that I used to test the datasets (in PR description) , for the specifics on how viz (and other plugins) would consume and use this however, I am not sure

[merelcht]

I think it would be good to mention here that Kedro doesn't do anything with this metadata, but that it can be consumed by plugins or directly by the user.

[rashidakanchwala]

How was layer being used before without being an argument in DataSet? I think it should be yes.

layer is currently consumed by the DataCatalog and the general consensus is that this is not ideal. Previously it was defined on every dataset as in this PR

It would be also great to think about how metadata could be access via hooks, i.e. how will viz using this new field. after_caterlog_create hook I guess?

metadata is accessible in the dummy hook implementation that I used to test the datasets (in PR description) , for the specifics on how viz (and other plugins) would consume and use this however, I am not sure

Hey @AhdraMeraliQB -- i think this is very important because currently we get layer information when we context.catalog -- would the metadata information also be in that way?

[antonymilne]

We would need to change how it's consumed on kedro-viz so that instead of looking at catalog.layers we look at get_dataset(dataset_name).metadata["kedro-viz"]["layer"] (where get_dataset is defined in CatalogRepository).

[merelcht]

Maybe we should just ignore this on the repo level, it's not really a relevant check for the datasets.

[noklam]

@AhdraMeraliQB @merelcht Not sure if we have a ticket already, but we should have docs explaining how this metadata should be consume i.e. via the hook

The implemention in the description is hacky as we are accessing the internal dict in weird way, I think this is related to the discussion we have about the datasets and frozen datasets.

class MetadataHooks:
    @hook_impl
    def after_catalog_created(self,
        catalog: DataCatalog,
        conf_catalog: Dict[str, Any],
        conf_creds: Dict[str, Any],
        feed_dict: Dict[str, Any],
        save_version: str,
        load_versions: Dict[str, str],
    ):
        for k,v in catalog.datasets.__dict__.items():
            print(k + "metadata: \n" + str(v.metadata))

[noklam]

Changed my thought Originally I think 3 is better but then it will always load the full catalog - in case of viz, you only want to load the data that is associated with the target pipeline. I just wrote down my reasoning here, I think 1 & 2 are equally bad but we don't have better option, so I am think with 1.

1. catalog.datasets.__dict__ as @AhdraMeraliQB did, which used private variable
2. catalog._data_sets - still using internal variable
3. conf_catalog - this doesn't use any internal variable, but it may contains the _ entries?

[antonymilne]

@noklam you're right that going through catalog.list() would only show those that are explicitly defined in catalog.yml and would not cover patterned ones. If you want all the datasets actually used in a pipeline then you should use pipelines.data_sets instead. Either way, catalog._get_dataset(dataset_name) will work for both explicitly defined and pattern-matched dataset.

[antonymilne]

As for the major change here: I think I may disagree @merelcht here and wonder whether we should actually use metadata rather than _metadata. It looks inconsistent with other attribute, but since metadata will by definition not get used anywhere in kedro we don't really need to mark it as protected. It's a dataset implementation attribute and not part of AbstractDataSet so we don't currently have any sort of interface associated with exposing _metadata. Hence there's currently no way for consumers (e.g. plugins) to use metadata through a public interface, which is not very encouraging for people who want to actually use it.

In theory we could have a @property for metadata to show the underlying _metadata attribute but I don't see any particular need for that when we could just make metadata public in the first place.

Curious what @idanov thinks here...

Aside from that, this and the other PR LGTM.

The metadata attribute is defined solely within self.metadata. Some datasets make use of a _describe method to return a dictionary of the dataset's attributes. I have not included the metadata in these methods as in some instances it would necessitate defining it twice, and I find the use redundant - however I would like to hear the reviewer's opinions on this matter.

Fine by me to not put it in _describe, but I don't understand what you mean by defining it twice here - can you give an example?

Metadata is accessible through dataset_name.metadata.Depending on the implementation this is at times inconsistent within the dataset, but it remains consistent across all datasets.

Also curious what you mean by this. Where is the inconsistency?

[AhdraMeraliQB]

@antonymilne

As for the major change here: I think I may disagree @merelcht here and wonder whether we should actually use metadata rather than _metadata. It looks inconsistent with other attribute, but since metadata will by definition not get used anywhere in kedro we don't really need to mark it as protected.

I did a little digging and it looks like the _preview function added by Viz is the same, in that it's not used by Kedro itself. For this reason, I'd argue to keep _metadata and maintain consistency.

However I also agree that that marking it as protected implies that accessing it is somewhat hacky. In this case I'd actually vouch for introducing a metadata property that accesses self._metadata.

Fine by me to not put it in _describe, but I don't understand what you mean by defining it twice here - can you give an example?

This really only came up in APIDataSet - the dataset has an instance variable _request_args which stores most of the arguments provided to the class. It's this instance variable that is then accessed by _describe(), so to include the metadata in _describe() it would need to be stored in two separate places - self._metadata and self._request_args.

Also curious what you mean by this. Where is the inconsistency?

Another example born from APIDataSet: it mostly doesn't make use of instance variables, instead having everything stored within the one variable self._request_args. This implementation ignores that and defines always metadata as an instance variable for every dataset, regardless of any variance in the dataset implementations.

[antonymilne]

I did a little digging and it looks like the _preview function added by Viz is the same, in that it's not used by Kedro itself. For this reason, I'd argue to keep _metadata and maintain consistency.

However I also agree that that marking it as protected implies that accessing it is somewhat hacky. In this case I'd actually vouch for introducing a metadata property that accesses self._metadata.

I don't think the comparison with _preview is quite accurate here for various reasons, but I do agree that it looks a little odd to have one public attribute when everything else is protected.

Having a metadata property is ok, but I think it should probably live in AbstractDataSet. It's then maybe a bit weird then that the _metadata attribute is defined in implementations but not the abstract dataset, but I guess it's consistent with how e.g. public AbstractDataSet.exists function wraps the implementation's _exists function (this type of template method pattern I'm also not a fan of, but that's for another time).

So yeah, if you and others think the property is a good idea then fine by me 👍 It does at least make metadata read-only after dataset instantiation, which is probably a good thing, and also provide a public interface for users.

Thanks for explaining about APIDataSet. I think what you've done here is good.

[AhdraMeraliQB]

I agree that the metadata property should live in the AbstractDataSet - I'll make those changes in #2537. I suppose I'll have to open a separate PR to add pass them through the datasets that can then be merged in after the changes on Kedro are released.

@merelcht do you have any thoughts on this?

[merelcht]

I agree that the metadata property should live in the AbstractDataSet - I'll make those changes in #2537. I suppose I'll have to open a separate PR to add pass them through the datasets that can then be merged in after the changes on Kedro are released.

@merelcht do you have any thoughts on this?

I think the comments and observations @antonymilne made are very valid. I hadn't properly thought it through. So I'd prefer to just make metadata public then and not add a property. If we add the property, it would be a breaking change to remove it again in future and we don't know yet how this metadata feature is going to be used so I'd prefer to not tie ourselves into this additional property.

[AhdraMeraliQB]

If we add the property, it would be a breaking change to remove it again in future and we don't know yet how this metadata feature is going to be used so I'd prefer to not tie ourselves into this additional property.

Wouldn't it be breaking either way?

[noklam]

The discussion has been mainly about how we should implement this new attributes. There are 3 options discussed in this PR and we go for 2 in this PR

1. self._metadata
2. self.metadata as a instance attribute
3. self.metadata (as a property)

If we want to change 2/3 in the future, it will be a breaking change since they are both public method. I do think this should goes into the Abstract Class, in future if a new dataset PR comes in and it doesn't have the metadata field, we will reject it. For this reason we should just define it explicit in the contract (abstract class).

3. seem to address the point about contract better, but it actually didn't enforce what we want. What we really want is enforcing metadata is part of the signature in the Class Constructor. I think the right way to do it is to enforce it in the abstract class constructor, i.e. super().__init__(xxx, xxx)

Follow up actions:

• We should add example hooks for how to consume this new metadata. Viz will likely be the first consumer.
• Remove of the layer attribute for 0.19.0 in favor of the metadata attribute

[antonymilne]

@noklam agree with everything you say here, but I believe that adding metadata to the AbstractDataSet constructor was difficult for some reason. I don't know the details (I think @merelcht does) but maybe comes down to the things discussed here and related issue about where AbstractDataSet lives: kedro-org/kedro#1076 (comment).

If we want to change 2/3 in the future, it will be a breaking change since they are both public method.

Agree, although a breaking change to a dataset is less awkward to deal with than to framework.

[antonymilne]

🌟