Fix MemoryDataSet not displaying on metadata panel
#1113
Conversation
There was a problem hiding this comment.
This feels like the right fix to me but is exactly what @deepyaman tried here and apparently didn't solve it? #1093 (comment)
modifying https://github.com/kedro-org/kedro-viz/blob/v5.1.1/package/kedro_viz/data_access/repositories/catalog.py#L59 to
dataset_obj = MemoryDataSet()causes the flicker regardless of whether or not theMemoryDataSetis explicitly defined in the catalog.
| dataset_obj = None | ||
| # if dataset has no catalog entry, it is a MemoryDataSet | ||
| if not dataset_obj: | ||
| dataset_obj = MemoryDataSet() |
There was a problem hiding this comment.
Is this any different from the below? Can dataset_obj ever evaluate to false if the exception doesn't get thrown?
| dataset_obj = None | |
| # if dataset has no catalog entry, it is a MemoryDataSet | |
| if not dataset_obj: | |
| dataset_obj = MemoryDataSet() | |
| dataset_obj = MemoryDataSet() |
There was a problem hiding this comment.
it work's if you just add it in the exception. the reason why it flickered was because filepath is none for MemoryDataSets and it is a required field in DataNodeMetaDataResponseAPI which now I have made Optional.
I will make the above change :)
There was a problem hiding this comment.
I'm happy with this. Yeah, I just didn't understand/get to finding what was happening with the validation error, but this solves it.
| @@ -2,8 +2,9 @@ | |||
| centralise access to Kedro data catalog.""" | |||
| # pylint: disable=missing-class-docstring,missing-function-docstring,protected-access | |||
| from typing import Dict, Optional | |||
| from joblib import Memory | |||
There was a problem hiding this comment.
| from joblib import Memory |
|
|
||
| X_train: | ||
| type: MemoryDataSet |
There was a problem hiding this comment.
| X_train: | |
| type: MemoryDataSet |
I guess we can revert it?
| @@ -55,8 +56,9 @@ def get_dataset(self, dataset_name: str) -> Optional[AbstractDataSet]: | |||
| dataset_obj = self._catalog._get_dataset(dataset_name, suggest=False) | |||
| else: # pragma: no cover | |||
| dataset_obj = self._catalog._get_dataset(dataset_name) | |||
| # if dataset has no catalog entry, it is a MemoryDataSet | |||
| except DataSetNotFoundError: # pragma: no cover | |||
There was a problem hiding this comment.
Wonder if this should be covered in tests, given that it does make a functional difference.
There was a problem hiding this comment.
yes, i will write the tests now !
| @@ -55,8 +56,9 @@ def get_dataset(self, dataset_name: str) -> Optional[AbstractDataSet]: | |||
| dataset_obj = self._catalog._get_dataset(dataset_name, suggest=False) | |||
| else: # pragma: no cover | |||
| dataset_obj = self._catalog._get_dataset(dataset_name) | |||
| # if dataset has no catalog entry, it is a MemoryDataSet | |||
There was a problem hiding this comment.
Nit: I think this is bordering on a redundant comment (the code seems really self-explanatory), but I suppose one could argue that it adds a bit of value. Not strong feelings, just a hunch.
deepyaman
changed the title
MemoryDataSet not displaying on metadata panel
Description
Resolves #1093
Development notes
@deepyaman , @tynandebold -- I have made an attempt to fix this issue at the backend. Let me know if this is good approach.
Basically, in the kedro-viz backend we add datasets to the node based on the catalog entries. If there's no catalog entry for a dataset, we just leave it blank. I have fixed this part by adding MemoryDataSet() to a dataset without any catalog entry.
Also MemoryDataSets don't have any file path which was a sort of required field in the ResponseAPI for DataNodes (which is why u got the pydantic error), so I have made that optional.
QA notes
I will add the necessary tests once everyone's ok with this fix.
Checklist
RELEASE.mdfile