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

Fix Kedro in Azure ML with SQLiteSessionStore #2131

Merged
merged 16 commits into from
Oct 14, 2024

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Oct 9, 2024

Description

Related to: #1991

This PR addresses issues with SQLite database locking in Azure Machine Learning (AML).

Reading online other users have also had this problem. It stems from the fact that in Azure ML the /home directory is mounted as CIFS filesystem which can't deal with SQLite3 lock. Read more here: https://stackoverflow.com/a/54051016

To resolve this, we can enable Write-Ahead Logging (WAL) mode to fix these locking issues.

We can either:

  1. Adopt WAL for all environments: If we go with this option the session_store.db file will have a small change in its internal structure to indicate that it is now using WAL mode. This change is stored in the database's header.

  2. Enable WAL mode conditionally for Azure ML: So we activate WAL mode only when the code detects that it is running in an AML environment. (Currently implemented in this PR).

Development notes

  • Included PRAGMA journal_mode=WAL; to enable WAL mode.

  • Implemented a condition to detect when it is running in an Azure Machine Learning environment using environment variables specific to Azure:AZUREML_ARM_SUBSCRIPTION, AZUREML_ARM_RESOURCEGROUP, or AZUREML_RUN_ID.

Provided two options:

  1. Apply WAL mode universally across all environments.
  2. Apply WAL mode only when the AML environment is detected, using the is_azure_ml flag.

QA notes

I tested this in Azure Machine Learning studio and confirmed that the database operates in WAL mode without the locking issues.

Tested locally as well it works as expected with and without enabling WAL mode.

image

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

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB SajidAlamQB changed the title Fix Kedro in Azure with SQLiteSessionStore Fix Kedro in Azure ML with SQLiteSessionStore Oct 9, 2024
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
SajidAlamQB and others added 2 commits October 10, 2024 11:45
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM. thanks

Signed-off-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com>
Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

thanks @SajidAlamQB , LGTM too 👍

@SajidAlamQB SajidAlamQB merged commit 9996c99 into main Oct 14, 2024
40 checks passed
@SajidAlamQB SajidAlamQB deleted the bugfix/azure-and-SQLiteStore branch October 14, 2024 11:51
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.

4 participants