Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing SESSION_STORE_ARGS default from starters #1951

Closed
1 of 2 tasks
noklam opened this issue Jun 20, 2024 · 8 comments · Fixed by kedro-org/kedro-starters#225
Closed
1 of 2 tasks

Removing SESSION_STORE_ARGS default from starters #1951

noklam opened this issue Jun 20, 2024 · 8 comments · Fixed by kedro-org/kedro-starters#225
Assignees
Labels
Python Pull requests that update Python code

Comments

@noklam
Copy link
Contributor

noklam commented Jun 20, 2024

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 #1891 (comment)

@rashidakanchwala
Copy link
Contributor

@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.

@ravi-kumar-pilla
Copy link
Contributor

ravi-kumar-pilla commented Jun 26, 2024

@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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

noklam commented Jul 2, 2024

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

@ravi-kumar-pilla ravi-kumar-pilla self-assigned this Jul 2, 2024
@ravi-kumar-pilla ravi-kumar-pilla added the Python Pull requests that update Python code label Jul 2, 2024
@ravi-kumar-pilla ravi-kumar-pilla moved this to Todo in Kedro-Viz Jul 2, 2024
@ravi-kumar-pilla
Copy link
Contributor

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
Copy link
Member

LGTM 👍🏼

@rashidakanchwala
Copy link
Contributor

LGTM. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python Pull requests that update Python code
Projects
Archived in project
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants