Remove KedroSession activate/deactivate mechanism

[noklam]

Signed-off-by: noklam [email protected]

Development Notes

• Remove the activate/deactivate & global _active_session all together in favor of the related after_context_created hook.

Description

Close #1431 (Copied from #1431)

I checked the repository the _activate_session() and the global _active_session are only used by ipython.py and the KedroSession's context manager. It was also used by get_current_session() but it no longer exists now.

The activate/deactivate session mechanism doesn't look necessary to me, in fact, if you just do session.run() today it runs perfectly fine. This whole activate/deactivate thing only stops you if you are using context manager.

Based on our discussion, it looks like we only want to do 1., but we should just do 2. unless there are some use cases we missed.

1. Update the logic so a session can be created with the existing active session (this was already possible before, it just doesn't work with the Context Manager style of session. However, this essentially means we do not care if there are pre-exist _active_session.

2. Remove the activate/deactivate altogether, since the global _active_session is not used by anyone (was used by get_current_session). But since we allow multiple sessions and there is no practical use to keep track of "active".

Development notes

• Add test for making sure multiple session can be created when we use context manager.
• Remove activate/deactivate and global _active_session

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 a description of this change in the RELEASE.md file
• Added tests to cover my changes

[Galileo-Galilei]

Hi @noklam , can you please check my comment in #1431 and tell me what you think before you merge this PR?

[noklam]

@Galileo-Galilei I am checking on the comments and older Github issue. Your concerns are valid and this won't be merged until further discussed. I can see why this is important for plugins. Thank you for your detailed comments.

[Galileo-Galilei]

Thank you very much!

[noklam]

Notes:

1. The scope of this PR will be limited to an ad-hoc fix so it doesn't block new session in Jupyter/Databricks environment when an active session is not closed.
2. The discussion of removing global _active_session will be deferred. Particularly, we will need more information about how the plugins developers are using it and do we need to expose more. Potentially adding new hooks/changing hookspec or some more thinking about the responsibility of KedroSession & KedroContext.

[noklam]

Ready for review again, but we need to make sure it is merged only after #1458 and release them both in 0.18.1

[antonymilne]

Given that #1458 is a prerequisite for this, will we need any of this stuff at all any more?

I'm a bit confused by whether the plan is to simultaneously do both of these:

1. introduce after_context_created hook
2. remove all the _active_session stuff

Or whether we want to do:

1. remove _activate_session stuff but leave _active_session global there
2. introduce after_context_created hook
3. remove all the _active_session stuff

i.e. do we do a straight swap in 0.18.1 (replace all _active_session things with the new hook) or do we do a phased introduction (in 0.18.1 you can use _active_session OR the new hook; then in 0.18.2 you can only use the new hook)?

Would be good to know if this affects @Galileo-Galilei 🙂

[noklam]

I think the idea is to do 1&2 together in 0.18.1. Doing 1 only would leave the _active_session None only? Sorry I found that there are commits in my local repository but never pushed to the remote once as I was switching between these related branches.

[antonymilne]

Ah ok, that makes sense! It looks much more like I was expecting after your push 😀

[Galileo-Galilei]

For what it's worth, I am completely ok with scenario (1), i.e. shipping the new hook and removing the session activation mechanism simultaneously in kedro==0.18.1. This is a private method and I think we can safely assume that people using my dirty hack of importing the global variable in their own plugin know what they are doing and accept the inherent risk of doing so.

There is always a possibility that we miss something and that releasing everything at the same time will eventually break the plugin with no workaround at the plugin level. I'll come back complaining in a new issue if it were to happen 😄. I have some concerns about the current proposal to make the context "frozen", but I'll let a comment in the other PR.

[antonymilne]

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?

[antonymilne]

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!

[merelcht]

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

[antonymilne]

Just a small comment on the tests but definitely happy to see this change!

[antonymilne]

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!

[noklam]

@AntonyMilneQB With the global _active_session it makes sense to test if it gets activated. Maybe what worth testing is KedroContext should be identical using the non-contextmanger or contextmanager version? They exhibit different behavior before (I think that was just a bug)

(Edited: I think we can now only do this after the attrs change otherwise it will be just comparing the hash of the two objects and that's not we want)

[merelcht]

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