Skip to content

Conversation

@schloerke
Copy link
Collaborator

@schloerke schloerke commented Mar 17, 2025

This reverts commit 14b92d4 via #1923

From conversations with @jcheng5 , the proxy sessions should have a single "parent" object which is "Root" session, they should not recurse up the session family tree during calls.

In a followup PR, I'll rename ._parent to ._root_session to avoid confusion.

From chat w/ @jcheng5

If there are reasons we want nested session scopes to actually be related (and there are reasons we might--like we could add .destroy() methods that recursively kill their child scopes), we can do that, but we’d have to make further changes I think.

Off the top of my head, we’d have to make sure that make_scope:

  1. Always returns the same object when make_scope is passed with the same id, much like Python does with loaded modules. We’d need to do this with weakrefs.
  2. If make_scope is passed with an id more than one level deep, we’d need to create/fetch a separate session proxy object for each level.

…on when making a scoped session (#1923)"

This reverts commit 14b92d4.
@schloerke schloerke changed the title revert: fix(session): Always have calling session be the parent session when making scoped session fix: Revert - fix(session): Always have calling session be the parent session when making scoped session Mar 17, 2025
@schloerke schloerke requested review from Copilot and jcheng5 March 17, 2025 17:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reverts a previous change to how scoped sessions are created so that the calling session is always used as the parent session. Key changes include:

  • Reverting the use of SessionProxy in favor of delegating the make_scope call to the parent session.
  • Updating the CHANGELOG to remove the entry for the previous behavior.
  • Removing a test skip that previously disabled recursive session tests.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
shiny/session/_session.py Reverts make_scope to call parent's make_scope method
CHANGELOG.md Removes outdated changelog entry for the reverted fix
tests/playwright/shiny/bookmark/modules/test_bookmark_modules.py Removes skip for recursive test
Comments suppressed due to low confidence (2)

shiny/session/_session.py:1237

  • Verify that deferring make_scope entirely to the parent session does not bypass necessary logic from the previously used SessionProxy.
return self._parent.make_scope(self.ns(id))

tests/playwright/shiny/bookmark/modules/test_bookmark_modules.py:28

  • Ensure that the removal of the recursive test skip is intentional and that recursive session behavior is now correctly handled to avoid potential test failures.
if "recursive" in app_name:

@schloerke schloerke marked this pull request as ready for review March 17, 2025 17:33
@jcheng5
Copy link
Collaborator

jcheng5 commented Mar 17, 2025

LGTM if @wch approves

@schloerke schloerke requested a review from wch March 17, 2025 17:37
@schloerke
Copy link
Collaborator Author

Approval received via chat:

In a followup PR, I'll rename ._parent to ._root_session to avoid confusion.

@wch: That is a better name 👍

Merging

@schloerke schloerke merged commit 381e851 into main Mar 17, 2025
63 of 64 checks passed
@schloerke schloerke deleted the revert_session_parent branch March 17, 2025 17:42
schloerke added a commit that referenced this pull request Mar 17, 2025
* main:
  fix: Revert - fix(session): Always have calling session be the parent session when making scoped session (#1924)
  fix(session): Always have calling session be the parent session when making a scoped session (#1923)
schloerke added a commit that referenced this pull request Mar 17, 2025
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.

3 participants