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

Feature/remove session store #2219

Merged

Conversation

Huongg
Copy link
Contributor

@Huongg Huongg commented Dec 2, 2024

Description

Fixes #2208
This PR is part of the effort to remove experiment tracking by removing the session store from the back-end.

Main Changes:

  • Completely removed database.py.
  • Completely removed sqlite_store.py.
  • Removed session_store references from:
    • data_loader.py
    • server.py
  • Deleted all tests related to the session store.

QA Notes:
Please pull the changes locally and check that the app runs without errors,

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Huong Nguyen added 3 commits December 2, 2024 21:28
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
@Huongg Huongg changed the title Feature/remove session store [DRAFT] Feature/remove session store Dec 2, 2024
Huong Nguyen added 7 commits December 3, 2024 10:01
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
@Huongg Huongg requested a review from merelcht December 3, 2024 15:24
# SESSION_STORE_ARGS = {"path": str(Path(__file__).parents[2] / "data"),
# "remote_path": "s3://{path-to-session_store}" }

# Define custom context class. Defaults to `KedroContext`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can keep the code from line 18-23 as is, as this is not related to ET

@@ -21,21 +21,3 @@ def configure_wal_for_azure(engine):
if is_azure_ml:
with engine.connect() as conn:
conn.execute(text("PRAGMA journal_mode=WAL;"))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is for handling SQLite which is not needed anymore, so I guess we can delete the file entirely

CONTRIBUTING.md Outdated
```python
from kedro_viz.integrations.kedro.sqlite_store import SQLiteStore
SESSION_STORE_ARGS = {"path": str(Path(__file__).parents[2] / "data")}
```
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make changes to all the starter templates (may be a note saying kedro-viz v11 does not support ET) as a follow up - https://github.com/kedro-org/kedro-starters/blob/main/spaceflights-pandas-viz/%7B%7B%20cookiecutter.repo_name%20%7D%7D/src/%7B%7B%20cookiecutter.python_package%20%7D%7D/settings.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, @merelcht has already raised this concern, and I’ll create a ticket for it. For now, we’ve added a deprecation warning for Kedro datasets to alert users about this change coming in the next release.

@ravi-kumar-pilla
Copy link
Contributor

Hi @Huongg ,

I know this PR is related to removing only Session store. I had a brief look and it looks fine for me. In general, I prepared a checklist as reference (you may add to this, but overall this would suffice) -

  • Remove graphql folder from api
  • Remove graphql_router from apps.py (line 66)
  • Remove “/et” from apps.py (line 84)
  • Remove SQLite Store from integrations/kedro/sqlite_store (This might also require changes to kedro starters as we say in the docs for ET, configure SQLiteStore in settings.py)
  • Remove docs related to ET

I hope this helps. I will also have a look at this PR once it is ready for review. Thank you

Huong Nguyen added 7 commits December 3, 2024 16:36
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
@Huongg Huongg marked this pull request as ready for review December 4, 2024 13:50
@Huongg Huongg changed the title [DRAFT] Feature/remove session store Feature/remove session store Dec 4, 2024
@Huongg
Copy link
Contributor Author

Huongg commented Dec 4, 2024

Thanks, @ravi-kumar-pilla, for the helpful guidance! I’ve implemented your suggestion to update the code related to removing session_store. The changes related to GraphQL and the router will be addressed in a separate PR to keep this one more focused and easier to review. Please take a look and let me know your thoughts.

Currently, the test coverage isn’t at 100% because I’ve deleted all the graphql_tests. Since the GraphQL-related code will be removed soon, I’m wondering if it's okay to ignore this in the PR for now. Here's the link to the error for reference: https://github.com/kedro-org/kedro-viz/actions/runs/12160128272/job/33911739972?pr=2219

@Huongg Huongg removed the request for review from astrojuanlu December 4, 2024 13:57
@Huongg Huongg requested review from SajidAlamQB and removed request for rashidakanchwala December 4, 2024 13:57
@tynandebold
Copy link
Member

Is the plan to remove all of the graphql code, or only that which relates to experiment tracking?

I'm in favor of removing it entirely. There's one graphql request that's used on the flowchart page though, and that's the getVersion one. That could definitely be moved to a REST endpoint. Bringing up here so we don't forget about it.

@Huongg
Copy link
Contributor Author

Huongg commented Dec 5, 2024

Is the plan to remove all of the graphql code, or only that which relates to experiment tracking?

I'm in favor of removing it entirely. There's one graphql request that's used on the flowchart page though, and that's the getVersion one. That could definitely be moved to a REST endpoint. Bringing up here so we don't forget about it.

hey so I'm thinking we're removing the graphql code related to ET in this PR, and I've opened a new issue separately to update the getVersion here

Copy link
Contributor

@jitu5 jitu5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Huongg

Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @Huongg, I've looked through most of the changes and it LGTM!

@Huongg Huongg changed the base branch from main to feature_branch/remove-experiment-tracking December 6, 2024 10:45
…/remove-session-store

Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
@Huongg Huongg merged commit 7ca0bd0 into feature_branch/remove-experiment-tracking Dec 9, 2024
46 of 56 checks passed
@Huongg Huongg deleted the feature/remove-session-store branch December 9, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ET back-end components
5 participants