kedro-org / kedro-viz

Visualise your Kedro data and machine-learning pipelines and track your experiments.
https://demo.kedro.org
Apache License 2.0
646 stars 106 forks source link

Removing `SESSION_STORE_ARGS` default from starters #1951

Open noklam opened 1 week ago

noklam commented 1 week ago

Context

Related:

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

rashidakanchwala commented 1 week ago

@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>"}
  2. or, we could omit Session Store setup entirely from settings.py and include all the details in the documentation.

ravi-kumar-pilla commented 1 week ago

@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

astrojuanlu commented 1 day ago

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

ravi-kumar-pilla commented 1 day ago

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

noklam commented 15 hours ago

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

ravi-kumar-pilla commented 11 hours ago

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

astrojuanlu commented 11 hours ago

LGTM 👍🏼

rashidakanchwala commented 11 hours ago

LGTM. thanks