Skip to content

Conversation

@thopp-tudelft
Copy link
Contributor

all renderaccess functions that use the requests library did so by using a "session" parameter in the function declaration, this parameter was given a default value like this: session=requests.session()
doing this executed this code on import, that is, when the library is imported and these functions declared each function will call requests.session() and store this object in the default parameter list.
this is quite annoying to deal with and causes unexpected behavior, like each function setting up its own connection persistence.
this behavior also contradicted the docstrings on for example renderapi/stack.py which says the default behavior is to start a new session, even though that was not true. (with the exception of clone_stack)

to fix this problem this pr removes all instances of session=requests.session() in declarations and brings its handling of requests more in line with expectations

I've divided this pr in separate commits:

  1. replace all of the malformed function declarations and instead use the renderaccess decorator to create a new session when not provided
  2. remove the unused requests imports and formatting changes
  3. add the session object to the Render class, allowing for easier use
  4. have the Render class default to creating a new session in order to allow reusing the session, restoring the performance lost when removing the reused session on import
  5. have the clone_stack function not create a new session anymore, this is redundant with the new behavior

most important is the first commit, the others can be split off into separate prs if there is a concern that these changes would impact existing use of the library.

thopp-tudelft added a commit to hoogenboom-group/scripted-render-pipeline that referenced this pull request Oct 31, 2025
@thopp-tudelft
Copy link
Contributor Author

hi @RussTorres please take a look at this, it'd be greatly appreciated!

@RussTorres RussTorres changed the base branch from master to develop November 24, 2025 20:43
@RussTorres RussTorres self-requested a review November 24, 2025 20:48
@RussTorres
Copy link
Collaborator

@thopp-tudelft I really appreciate this PR and it looks great! Not sure how this historically came about but this change is definitely a better implementation.
I am going to run this through the new integration test workflow I am working on, but don't foresee any issues. After that I will accept this and it will go dev -> master/main and I will publish the new version to pypi. I should get through this in the next day or so.

Thank you!

Copy link
Collaborator

@RussTorres RussTorres left a comment

Choose a reason for hiding this comment

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

With the change in thopp-tudelft#1

This passes its own tests and integration tests with downstream repos. As soon as that commit makes it in I will merge this PR.

Thanks!

@RussTorres RussTorres self-requested a review December 2, 2025 22:45
@RussTorres RussTorres merged commit 9d742cf into AllenInstitute:develop Dec 2, 2025
@thopp-tudelft thopp-tudelft deleted the session branch December 4, 2025 12:25
@thopp-tudelft
Copy link
Contributor Author

thanks! can you give me an estimate how long it'll take before the develop branch will be included in releases?

@RussTorres
Copy link
Collaborator

@thopp-tudelft I just updated the release strategy and deployed v2.4.0 to pypi -- seems like it's ready to use.

@thopp-tudelft
Copy link
Contributor Author

thopp-tudelft commented Dec 11, 2025

thank you for notifying me!

I noticed however that your latest release does not support python 3.13 even though it was released a year ago. Is there a specific reason to restrict it from running on python 3.13? I have been using the previous version on python 3.13 so far.

I've also been trying to install it but no matter the version of python I can't actually install it from pypi. when I pip install the package only the dist-info folder is created and not the actual library. there might be a mistake in your new pyproject.toml file possibly?

@RussTorres
Copy link
Collaborator

Yes, there was a typo in the pyproject that I had already fixed on another deployment -- v2.4.1 has that change and works for me with a fresh 3.11 environment. I don't have the new automated testing action set up for this repo, but I manually tested on 3.12.12 and 3.13.11. Changed to <3.14 for now and I will release that as v2.4.2

@thopp-tudelft
Copy link
Contributor Author

I tested with the 2.4.1 version and it works! thank you!

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.

2 participants