[KED-2565] Register DataCatalog in settings instead of through the registration hook.

[merelcht]

Description

DataCatalog is currently registered via hooks - however we would like this to be via settings.py instead.

Development notes

• Followed the same refactoring the DataCatalog registration as done for the ConfigLoader
• Kept the DataCatalog as part of the KedroContext for this PR. The removal will happen seperately.
• Default register_catalog hook_impl has been removed
• DATA_CATALOG_CLASS has been added to _ProjectSettings class impl
• DATA_CATALOG_CLASS has been added (commented) to settings.py
• Default DATA_CATALOG_CLASS is set to kedro.io.DataCatalog
• Left the ProjectHooks in place for now as we have a separate task in the backlog to get rid of the registration hooks fully.

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

[lorenabalan]

Thank you for picking this up and making it such a clean PR to review. Made a few suggestions, let me know what you think. Everything looks good otherwise.

[lorenabalan]

Awesome stuff! 💥

[antonymilne]

General 👍 👍 👍 from me but with a few questions...

• Do we want to be exposing DATA_CATALOG_ARGS as we do CONFIG_LOADER_ARGS? I don't know if this has been considered already. Personally I have no strong feelings on it - we can always add it on in a separate PR or some time in the future if it turns out it's needed. I suspect @lorenabalan or @idanov would know the answer to whether we want to do this anyway
• Maybe I missed it, but I couldn't see any tests to make sure this is actually working ok? You have the BadCatalog but I didn't see any happy path tests analogous to the config loader tests that use MyConfigLoader

Completely separate from this PR, but before we release 0.18 we should really go through the release notes and reorganise them. e.g. to put all the registration hook change stuff together. Same goes for settings.py actually, to make sure that it's all nicely ordered and clear and consistent.

[merelcht]

• Do we want to be exposing DATA_CATALOG_ARGS as we do CONFIG_LOADER_ARGS? I don't know if this has been considered already. Personally I have no strong feelings on it - we can always add it on in a separate PR or some time in the future if it turns out it's needed. I suspect @lorenabalan or @idanov would know the answer to whether we want to do this anyway

Good question. I'm not sure if we've discussed this or not 🤔 The DataCatalog is constructed slightly differently from the ConfigLoader though, it calls from_config which takes specific parameters so we probably need to add some input validation. I haven't really looked at it in depth though so maybe I'm talking nonsense.

• Maybe I missed it, but I couldn't see any tests to make sure this is actually working ok? You have the BadCatalog but I didn't see any happy path tests analogous to the config loader tests that use MyConfigLoader

The happy paths are the already existing tests. Follow up to this task will be to remove the catalog from KedroContext, so I think it makes sense to revisit/rewrite the tests as part of that as well.

[antonymilne]

Good question. I'm not sure if we've discussed this or not 🤔 The DataCatalog is constructed slightly differently from the ConfigLoader though, it calls from_config which takes specific parameters so we probably need to add some input validation. I haven't really looked at it in depth though so maybe I'm talking nonsense.

I don't think the fact that it's constructed from from_config would make a difference here. We could do

        catalog = settings.DATA_CATALOG_CLASS.from_config(
            catalog=conf_catalog,
            credentials=conf_creds,
            load_versions=load_versions,
            save_version=save_version,
            **settings.DATA_CATALOG_ARGS
        )

...analogous to how we do

 return config_loader_class(
                conf_source=str(self._project_path / settings.CONF_SOURCE),
                env=env,
                runtime_params=extra_params,
                **settings.CONFIG_LOADER_ARGS,
            )

Overall to me, at a glance it makes sense that we should provide DATA_CATALOG_ARGS since, as I understand it, data catalog is on an equivalent footing to config loader and hence should have the same options for configurability and extensibility. This is in contrast to CONTEXT_CLASS, for which I'm guessing we are deliberately not enabling CONTEXT_ARGS. But I guess others may argue that we should not enable this functionality until there's a proven user need for it.

The happy paths are the already existing tests. Follow up to this task will be to remove the catalog from KedroContext, so I think it makes sense to revisit/rewrite the tests as part of that as well.

Do these test what happens if I overwrite DATA_CATALOG_CLASS with my own custom class? I couldn't see that but could definitely just be missing it!

[lorenabalan]

My personal preference would be to leave it as is for now because I don't think we know enough information about that workflow, if it even exists. With the ConfigLoader we at least could look at two classes (templated vs regular) and infer a pattern, but there's no feasible option for an alternative for DataCatalog out of the box - only potentially user-defined options which we have no visibility over.

If the workflow exists, I would rather make the catalog go through the same exercise as the config loader, and see if we can define a clearer interface, rather than force it now and feel patchy. We can add it later as a feature addition, not a breaking change.

[merelcht]

Thanks for your input @lorenabalan and @AntonyMilneQB! I will merge this PR now as is and if we find that users need to pass arguments to the catalog we can create a follow up task to investigate and add this functionality.