kedro-viz icon indicating copy to clipboard operation
kedro-viz copied to clipboard

Removing `SESSION_STORE_ARGS` default from starters

Open noklam opened this issue 1 year ago • 8 comments

Context

Related:

  • [x] https://github.com/kedro-org/kedro-viz/issues/1891
  • [ ] https://github.com/kedro-org/kedro-viz/pull/1915

As Kedro-viz is introducing a default "SESSION_STORE_ARGS", we should moving away from setting SESSION_STORE_ARGS in kedro-starters and leave that for kedro-viz. Noted this is necessary as we currently have a different default:

SESSION_STORE_ARGS = {"path": str(Path(__file__).parents[2] / )}

but we want to change to

SESSION_STORE_ARGS = {"path": str(Path(__file__).parents[2] / .viz)}

Development Notes:

We need to upgrade the lower pin of kedro-viz to 9.2.0 to make sure the default will be .viz as well.

@ravi-kumar-pilla @rashidakanchwala Can we make it so that Kedro-Viz assumes a default SESSION_STORE_ARGS of

SESSION_STORE_ARGS = {"path": str(Path(__file__).parents[2] / ".viz")}

?

That would be the first step towards fixing the issue. The second one would be to update the starters to actually remove the SESSION_STORE_ARGS from them, so that Viz uses its default.

We are also overloading "SESSION_STORE_ARGS" as "VIZ_METADATA_ARGS" here, the stats.json has nothing to do with session store.

@noklam Could you open a new issue about this describing it in a bit more detail?

Originally posted by @astrojuanlu in https://github.com/kedro-org/kedro-viz/issues/1891#issuecomment-2132963984

noklam avatar Jun 20 '24 04:06 noklam

@noklam, @astrojuanlu , @ravi-kumar-pilla -

  1. I was thinking we either add this to settings.py for starters
# Setup for collaborative experiment tracking.
# The SQLite DB required for experiment tracking is stored by default in the .viz folder of your project.
# To store it in another directory, provide the path: SESSION_STORE_ARGS = {"path": "<Your PATH>"}
  1. or, we could omit Session Store setup entirely from settings.py and include all the details in the documentation.

rashidakanchwala avatar Jun 25 '24 08:06 rashidakanchwala

@noklam, @astrojuanlu , @ravi-kumar-pilla -

  1. I was thinking we either add this to settings.py for starters
# Setup for collaborative experiment tracking.
# The SQLite DB required for experiment tracking is stored by default in the .viz folder of your project.
# To store it in another directory, provide the path: SESSION_STORE_ARGS = {"path": "<Your PATH>"}

Just to make sure we are on the same page, the PR only provides a default session store db path and stats path. The users still need to provide SESSION_STORE_CLASS in settings.py to customize how session data is stored. So we cannot omit the entire Session Store setup. i.e.,

from kedro_viz.integrations.kedro.sqlite_store import SQLiteStore

SESSION_STORE_CLASS = SQLiteStore

So, I think it is better we add comments in settings.py as you suggested in (1 - # Setup for experiment tracking). We can also document this in the project settings doc. Let me know your thoughts.

Thank you

ravi-kumar-pilla avatar Jun 26 '24 02:06 ravi-kumar-pilla

The users still need to provide SESSION_STORE_CLASS in settings.py to customize how session data is stored.

Can't we also provide a default for that one? In the same way we don't have to make users declare their DATA_CATALOG_CLASS for instance

astrojuanlu avatar Jul 01 '24 14:07 astrojuanlu

The users still need to provide SESSION_STORE_CLASS in settings.py to customize how session data is stored.

Can't we also provide a default for that one? In the same way we don't have to make users declare their DATA_CATALOG_CLASS for instance

I am not sure if that is something we can do from viz. So the changes to move SESSION_STORE_ARGS are made in this class which is assigned in settings.py as SESSION_STORE_CLASS. This gets initialized in _init_store of KedroSession.

Thank you

ravi-kumar-pilla avatar Jul 02 '24 02:07 ravi-kumar-pilla

I think SESSION_STORE_CLASS is still needed. SESSION_STORE_CLASS_ARGS can be left as a comment for discoverability

noklam avatar Jul 02 '24 11:07 noklam

Hi Team,

Should I update the settings.py in each starter with the below block ?

Before -

# Keyword arguments to pass to the `SESSION_STORE_CLASS` constructor.
# SESSION_STORE_ARGS = {
#     "path": "./sessions"
# }

After -

# Setup for Experiment Tracking
# The SQLite DB required for experiment tracking is stored by default in the .viz folder of your project.
# To store it in another directory, provide the keyword argument `SESSION_STORE_ARGS` to pass to the `SESSION_STORE_CLASS` constructor.
# SESSION_STORE_ARGS = {"path": str(Path(__file__).parents[2] / .viz)}

Since this is a repeat block for all the starters, I wanted to get your feedback before making changes in all the starters. @astrojuanlu @noklam @rashidakanchwala

Thank you

ravi-kumar-pilla avatar Jul 02 '24 14:07 ravi-kumar-pilla

LGTM 👍🏼

astrojuanlu avatar Jul 02 '24 14:07 astrojuanlu

LGTM. thanks

rashidakanchwala avatar Jul 02 '24 15:07 rashidakanchwala