Dataset factories

[ankatiyar]

Description

Resolve #2423 This PR is introduces the dataset factories feature. Look at #2670 for the documentation on how to use it.

Development notes

Kedro run workflow

-->kedro run in terminal
--> Session is created and run
--> Context created which loads all the config from the conf_source
--> DataCatalog.fromconfig(dict:catalog) which initialises the DataCatalog object

• At this point the DataCatalog is created with explicitly mentioned datasets in DataCatalog._data_sets
• Sort the patterns and save them to DataCatalog._dataset_patterns
• Also save load_version and save_version to self._load_version and self._save_version

--> runner.run(pipeline, catalog, .., ..)

• loop through all datasets used by the pipeline and save all that exist (either as explicit datasets or patterns) to registered_ds
• The check for existence will materialise and add the datasets to the catalog (in DataCatalog.__contains__())

--> Pipeline is run and catalog._get_dataset(dataset_name)-> AbstractDataSet : No major change because datasets should already be materialised at the check for existence

• Add datasets that exist as patterns as materialised datasets to the catalog

NOTE : The tests are not updated to reflect the latest changes (TODO)

Toy project to test with : https://github.com/ankatiyar/space-patterns

Follow up tasks to do

• Update tests
• Documentation Add documentation for the dataset factories feature #2666
• Use dataset factories to register default datasets #2668
• Update kedro catalog list + Add new catalog CLI commands for dataset factories #2603

Checklist

• Read the contributing guidelines
• 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 RELEASE.md file
• Added tests to cover my changes

[merelcht]

I've done a first review of the code in data_catalog.py and left some questions. My main question is what the purpose of the _pattern_name_matches_cache is and the methods that update it.

[ankatiyar]

@merelcht The reason I added self._pattern_name_matches_cache is because the first thing we need to do in the runner is check if all the datasets used in the pipeline exist in the catalog using catalog.exists_in_catalog(). At this step if we find a pattern that matches, it's added to the cache dict which stores dataset name -> pattern match. (Just so when we do catalog._get_dataset(dataset_name) we don't have to iterate over all the patterns again).

This might be overkill and I can revert to what the prototype was doing but I thought for large catalogs, this would avoid doing the same iteration twice. What do you think?

The workflow for then fetching a dataset is ->

1. If the dataset is in self._data_set - go to step 3
2. If not, I reuse the self.exists_in_catalog() to check if it is a pattern.
   i. If this is the first time we're checking for the dataset's existence( e.g when someone is using DataCatalog as a standalone and the runner hasn't populated the cache), add "the name -> pattern" entry in the self._pattern_name_matches_cache.
   ii. get the factory pattern for the dataset name from _pattern_name_matches_cache()
   iii. get resolved dataset using _get_resolved_dataset() -> resolves the config and then loads the instance of AbstractDataSet
   iv. Add the resolved dataset to self._data_sets
3. Load dataset from self._data_sets and return

[merelcht]

Thanks for clarifying the use of the cache @ankatiyar, it makes more sense now! I think we can keep it, but perhaps give it a slightly shorter name e.g. _pattern_matches_cache.
In general, I struggled with following the code because some of the names of the methods. I tried giving suggestions on how to improve, but it's quite personal so it would be good to get some other opinions as well 🙂

One thing every single user in the interviews mentioned is a warning about the catch-all pattern, so we must really implement that, including tests.

Another thing that was mentioned several times was a warning about when multiple patterns match a dataset, it would be good to have a test for that to see how that would happen and if we can also warn about that.

[ankatiyar]

@merelcht I'll leave the warning about catch-all patterns and multiple matches out of this PR but do it as a part of #2668

[noklam]

Drop some comments for 1st round of review.

[noklam]

This should be refactor as a method and have its own unit tests

[noklam]

Is this the cache that add matched pattern into catalog? If so why do we need to keep it separately instead of just creating the entry in catalog?

[ankatiyar]

This fn just adds the dataset name -> pattern in the self._pattern_matches_cache does not create an entry in the catalog. This is because this function is also called in the runner to check for existence of datasets from the pipeline in the catalog. The actual adding of a dataset to the catalog happens in self._get_dataset()
This is to avoid doing the pattern matching multiple times when it's called inside a Session run but the actual resolving and adding is still at _get_dataset time to allow for DataCatalog standalone usage

[idanov]

This isn't a good name for a public method, it's way too descriptive. I would suggest that you change it to def __contains__(self, dataset_name):. This will allow us to do a test like this:
if dataset_name in catalog:..., which is way more intuitive and nicer than catalog.exists_in_catalog_config(dataset_name).

[merelcht]

I reviewed both this PR and #2743 and I definitely prefer the approach here with adding load_versions and save_version arguments instead of raw_catalog. I left some more comments, but in general I think this is nearly finished.

[merelcht]

For completeness this doc string should also include the return type/value.

[ankatiyar]

It's a private method, the Args: part is leftover from when it was a helper fn outside DataCatalog I've removed it

[merelcht]

I left one more minor comment, but apart from that I'm very happy with how this looks! It would be good to get another review from @noklam or @idanov, but from my side this is good to go 👍

[noklam]

I have some comments here, but I figured it is easier if I drop all my comments in one draft PR, it's not ready yet. I will ping you when it's ready.

Review PR since it is easier.

[noklam]

Maybe it is good to introduce keyword-only arguments here, similar to the proposal to get us more freedom to re-arrange the arguments later without breaking changes. The current argument lists does not make too much sense

[noklam]

This still confuse me a lot, is there any difference? Isn't dataset_patterns a dict already?

Suggested change

	self._dataset_patterns = dict(dataset_patterns or {})
	self._dataset_patterns =dataset_patterns or {}

[noklam]

Same as self._dataset_patterns

[noklam]

Same argument, good to have keywords-only argument here even we are going to remove layers soon

[noklam]

Suggested change

	if "{" in pattern:
	return True
	return False
	return "{" in pattern

[noklam]

As discussed, let's separate the Type Alias proposed in #2770.
I will close #2770 and approve this PR now. :)

[noklam]

LGTM now! Let's ship it and do the type alias separately.

[idanov]

This could be nicely rewritten to:

matches = (parse(pattern, data_set_name) for pattern in data_set_patterns.keys())
return next(filter(None, matches), None)

[idanov]

This test should be reordered, since key in catalog is easier to test and probably will match more often. No need to go through _match_pattern for all the non-patterned datasets.

[idanov]

Shorter and neater.

return {key: data_set_patterns[key] for key in sorted_keys}