Check whether DataCatalog changes are reflected in the session

[astrojuanlu]

Today I wanted to apply an "advanced" hook use case: storing the catalog and then injecting datasets on the fly. However, it doesn't work:

class MissingDatasetHooks:
    @hook_impl
    def after_catalog_created(self, catalog: DataCatalog):
        self._catalog = catalog

    @hook_impl
    def before_dataset_loaded(self, dataset_name):
        dataset = self._catalog._get_dataset(dataset_name)
        try:
            dataset.load()
        except DataSetError:
            # Create EmptyDataset on the fly
            logger.warning("Attempted to load dataset %s which doesn't exist yet, injecting it", dataset_name)
            missing_dataset = MissingDataSet(dataset=dataset)
            self._catalog.add(dataset_name, missing_dataset, replace=True)

the self._catalog that gets saved receives the .add(..., replace=True) correctly, but the catalog.load that comes immediately after the before_dataset_loaded hook still has the old dataset:

kedro/kedro/runner/runner.py

Lines 403 to 404 in fd8162d

	hook_manager.hook.before_dataset_loaded(dataset_name=name, node=node)
	inputs[name] = catalog.load(name)

Using after_context_created gets the same result.

Context: I was trying to give a workaround for https://stackoverflow.com/q/76557758/554319.

Is this behavior expected?

Originally posted by @astrojuanlu in #2690 (comment)

[gitgud5000]

I've been trying to achieve this aswell. It would be a great feature to add.

[astrojuanlu]

@gitgud5000 Can you detail a bit more what are you trying to achieve and why? We are trying to come up with use cases for this change.

[gitgud5000]

@astrojuanlu I'm trying to pass additional save_args based on the output of the node that creates a pandas.SQLTableDataSet as output.

My plan was to modify the catalog in a before_dataset_saved hook.

Similar to this: #910
This is also related: #898

[gitgud5000]

What I ended up doing was implementing a subclass of pandas.SQLTableDataSet

[noklam]

I don't think mutating DataCatalog is a good thing to do. I believe the hook system was designed in a way to avoid this exactly (correct me if I am wrong).

I am unsure if this example correct though because dataset.load() just load the dataset but not returning anything, how would it work? Maybe need an example to play with to investigate this.

There are however use cases that is very useful.
i.e. If you developing in a notebook environment, it is very convenient if you can inject/change data to test your pipeline without re-running the whole pipeline

[astrojuanlu]

I don't think mutating DataCatalog is a good thing to do.

That's fair enough, but not the impression that I got when I saw a DataCatalog.add method existed 🤔 How could we reduce confusion here?

[noklam]

That's fair enough, but not the impression that I got when I saw a DataCatalog.add method existed 🤔 How could we reduce confusion here?

I think we need to clarify what's not working here. DataCatalog.add should work in a standalone mode? What doesn't work here is because catalog is not stored in KedroContext but rather hot-reload everytime it gets called.

• Removing KedroContext params and catalog hot-reload #1460

I think there are two conflicting features here and we need to think of it carefully.

• Hot-reload feature, which is useful for notebook because it saves you from keep creating new catalog when you are editing catalog.yml on the side. This because less important with %reload_kedro, thus Removing KedroContext params and catalog hot-reload #1460. However, if we are now steering to using components separately. We may need to re-think what should be done here.
  • Make it easier to use the Config Loader #2819
• Immutable KedroContext - Revisit: Make KedroContext a dataclass and add config_loader as a property #1459, I re-open this issue recently.

  • This was merged already, but as we approaching 0.19, I want to revise if it's a good idea to freeze KedroContext.
    We should also consider how easy for plugin developer to use KedroContext. In the past we were more negative about storing context and states in Hooks for following hooks. However in Document more advanced hook use cases #2690, we may want to document this. If this is not a bad idea, do we need to re-visit the decision we made?

In any case, we should list out why we want to make this a singleton. I don't have the full context why it was designed that way, may be good to dig out the old issues and PR, but I don't have it now.

[astrojuanlu]

Another user was confused by this but found a workaround that worked for their use case: using runner.run(..., catalog=new_catalog) https://linen-slack.kedro.org/t/16062852/hi-all-how-can-i-update-replace-catalog-entries-from-an-exis#e163f246-de25-405d-9462-7fbc757bf927

[noklam]

Maybe the action item for this ticket is:

• Create documentation to explain DataCatalog immutability

Is there a good reason Kedro should start supporting this?

[astrojuanlu]

@ElenaKhaustova to check again if this happens with DataCatalog and/or with KedroDataCatalog, should be quick

[ElenaKhaustova]

I double-checked that this behaviour is only relevant for DataCatalog and for KedroDataCatalog, modification of catalog object will be reflected in the further hooks.

This change is explained by the removing shallow_copy() method for KedroDataCatalog and the execution order:

1. First, we create catalog in the session
   

   kedro/kedro/framework/session/session.py

   Line 382 in a1fae50

   catalog = context._get_catalog(

2. Then after_catalog_created hook is called where we save catalog object for further use
   

   kedro/kedro/framework/context/context.py

   Line 244 in a1fae50

   self._hook_manager.hook.after_catalog_created(

3. Then AbstractRunner.run is called where we make a shallow copy of catalog (for DataCatalog)
   

   kedro/kedro/runner/runner.py

   Line 93 in a1fae50

   catalog = catalog.shallow_copy(

4. Then AbstractRunner._run is called based on the runner set
   

   kedro/kedro/runner/runner.py

   Line 107 in a1fae50

   self._run(pipeline, catalog, hook_or_null_manager, session_id) # type: ignore[arg-type]

5. Then before_dataset_loaded hook is called, so we modify the catalog that we saved but not the catalog used in the run
   

   kedro/kedro/runner/task.py

   Line 151 in a1fae50

   hook_manager.hook.before_dataset_loaded(dataset_name=name, node=node)

Note: For DataCatalog, we used the shallow copy method to add runtime patterns to the catalog before the run. Now, we have a dedicated method to add just patterns for KedroDataCatalog, so shallow copy is not done anymore

kedro/kedro/io/kedro_data_catalog.py

Line 577 in a1fae50

def shallow_copy(

@astrojuanlu, @merelcht, @ankatiyar, @noklam, @lrcouto, @DimedS, based on the above, I suggest closing the ticket. It works as expected for the new catalog, which will soon replace the old one.

[astrojuanlu]

Thanks @ElenaKhaustova ! Closing