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

Remove KedroSession activate/deactivate mechanism #1434

Merged
merged 23 commits into from
May 9, 2022
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
957e8e4
Removed nested session test and add tests to make sure multiple sessi…
noklam Apr 12, 2022
dc422f3
Remove _active_session global and activate/deactivate mechanism.
noklam Apr 12, 2022
276c451
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam Apr 12, 2022
2d5cafb
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam Apr 12, 2022
06d793b
Merge branch 'feat/activate-deactivate-kedrosession' of https://githu…
noklam Apr 12, 2022
1d90518
Revert "Remove _active_session global and activate/deactivate mechani…
noklam Apr 14, 2022
36206c5
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam Apr 14, 2022
bb2ca8a
Merge branch 'feat/activate-deactivate-kedrosession' of https://githu…
noklam Apr 19, 2022
d533bfb
remove session checking
noklam Apr 19, 2022
3dd3bf0
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam Apr 19, 2022
7314320
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam Apr 20, 2022
5fdae60
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam Apr 27, 2022
c542255
Clean up activate deactivate session
noklam Apr 27, 2022
6e5da8c
Update RELEASE.md
noklam May 5, 2022
d974796
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam May 5, 2022
d9fe519
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam May 6, 2022
a9276f8
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam May 6, 2022
5bbbc7d
Fix merged conflict
noklam May 9, 2022
da70fb4
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam May 9, 2022
084a65f
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam May 9, 2022
2be1e22
Replace the ellipsis syntax with pass
noklam May 9, 2022
81f9a92
Merge branch 'feat/activate-deactivate-kedrosession' of https://githu…
noklam May 9, 2022
7c39e57
Merge branch 'main' into feat/activate-deactivate-kedrosession
noklam May 9, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
* Introduced `after_command_run` CLI hook.
* Update sections on visualisation, namespacing, and experiment tracking in the spaceflight tutorial to correspond to the complete spaceflights starter.
* Fixed `Jinja2` syntax loading with `TemplatedConfigLoader` using `globals.yml`.
* Removed global `_active_session`, `_activate_session` and `_deactivate_session`. Plugins that need to access objects such as the config loader should now do so through `context` in the new `after_context_created` hook.

## Upcoming deprecations for Kedro 0.19.0

Expand Down
2 changes: 0 additions & 2 deletions kedro/extras/extensions/ipython.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ def reload_kedro(
from kedro.framework.cli import load_entry_points
from kedro.framework.project import configure_project, pipelines
from kedro.framework.session import KedroSession
from kedro.framework.session.session import _activate_session
from kedro.framework.startup import bootstrap_project

# If a path is provided, set it as default for subsequent calls
Expand All @@ -63,7 +62,6 @@ def reload_kedro(
session = KedroSession.create(
metadata.package_name, default_project_path, env=env, extra_params=extra_params
)
_activate_session(session, force=True)
logging.debug("Loading the context from %s", default_project_path)
context = session.load_context()
catalog = context.catalog
Expand Down
23 changes: 0 additions & 23 deletions kedro/framework/session/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,24 +28,6 @@
from kedro.io.core import generate_timestamp
from kedro.runner import AbstractRunner, SequentialRunner

_active_session = None


def _activate_session(session: "KedroSession", force: bool = False) -> None:
global _active_session

if _active_session and not force and session is not _active_session:
raise RuntimeError(
"Cannot activate the session as another active session already exists."
)

_active_session = session


def _deactivate_session() -> None:
global _active_session
_active_session = None


def _describe_git(project_path: Path) -> Dict[str, Dict[str, Any]]:
project_path = str(project_path)
Expand Down Expand Up @@ -282,12 +264,7 @@ def close(self):
if self.save_on_close:
self._store.save()

if _active_session is self:
_deactivate_session()

def __enter__(self):
if _active_session is not self:
_activate_session(self)
return self

def __exit__(self, exc_type, exc_value, tb_):
Expand Down
2 changes: 0 additions & 2 deletions tests/extras/extensions/test_ipython.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import pytest

from kedro.extras.extensions.ipython import load_ipython_extension, reload_kedro
from kedro.framework.session.session import _deactivate_session
from kedro.framework.startup import ProjectMetadata
from kedro.pipeline import Pipeline

Expand All @@ -23,7 +22,6 @@ def mocked_logging(mocker):
@pytest.fixture(autouse=True)
def cleanup_session():
yield
_deactivate_session()


@pytest.fixture()
Expand Down
20 changes: 6 additions & 14 deletions tests/framework/session/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
_HasSharedParentClassValidator,
_IsSubclassValidator,
_ProjectSettings,
configure_project,
)
from kedro.framework.session import KedroSession
from kedro.framework.session.store import BaseSessionStore, ShelveStore
Expand Down Expand Up @@ -285,6 +284,12 @@ def test_create(
assert session.load_context() is mock_context_class.return_value
assert isinstance(session._get_config_loader(), ConfigLoader)

@pytest.mark.usefixtures("mock_settings")
def test_create_multiple_sessions(self, fake_project, mock_package_name):
with KedroSession.create(mock_package_name, fake_project):
with KedroSession.create(mock_package_name, fake_project):
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to do the context manager of this just to check it works the same way?

with KedroSession.create() as first_session:
    assert kedro.framework.session.session._active_session is first_session
    with KedroSession.create() as new_session:
        assert kedro.framework.session.session._active_session is new_session
    assert kedro.framework.session.session._active_session is new_session ## is that right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see that now that _active_session no longer exists it's not possible to test as you had before and in my above comment. But would it be possible to make this a little more explicit somehow? Something like this maybe?

@pytest.mark.usefixtures("mock_settings")
    def test_create_multiple_sessions(self, fake_project, mock_package_name):
        with KedroSession.create(mock_package_name, fake_project) as session1:
            with KedroSession.create(mock_package_name, fake_project) as session2:
                assert session1 is not session2

Also I quite liked having the separate test you had before for the non-context manager version like this:

@pytest.mark.usefixtures("mock_settings")
    def test_create_multiple_sessions(self, fake_project, mock_package_name):
        session1 = KedroSession.create(mock_package_name, fake_project)
        session2 = KedroSession.create(mock_package_name, fake_project)
        assert session1 is not session2

But maybe there's no point doing that - up to you!

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to check that it's now possible to create two sessions without it failing, like it did previously.

@pytest.mark.usefixtures("mock_settings_context_class")
def test_create_no_env_extra_params(
self,
Expand Down Expand Up @@ -519,19 +524,6 @@ def test_log_error(self, fake_project, mock_package_name):
"raise FakeException" in tb_line for tb_line in exception["traceback"]
)

@pytest.mark.usefixtures("mock_settings")
def test_nested_sessions(self, fake_project, mock_package_name):
configure_project(mock_package_name)
session1 = KedroSession.create(mock_package_name, fake_project)
session2 = KedroSession.create(mock_package_name, fake_project)

with session1:
pattern = (
"Cannot activate the session as another active session already exists"
)
with pytest.raises(RuntimeError, match=pattern), session2:
pass # pragma: no cover

@pytest.mark.usefixtures("mock_settings_context_class")
@pytest.mark.parametrize("fake_pipeline_name", [None, _FAKE_PIPELINE_NAME])
def test_run(
Expand Down