Quell async warning, and POST with body for jupyterhub 3.0

[ryanlovett]

I upgraded a development hub to jupyterhub 3.0.0b1 from 1.5.x and ran into a couple of issues. I'm using slurm and these issues were present with the latest release and latest in git:

Initially I was seeing the following warning:

/usr/local/linux/anaconda3.8/envs/jupyterhub-3.0/lib/python3.8/site-packages/batchspawner/singleuser.py:16: RuntimeWarning: coroutine 'HubAuth._api_request' was never awaited
  hub_auth._api_request(method='POST',
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
[I 2022-08-19 23:32:20.510 SingleUserLabApp mixins:610] Starting jupyterhub single-user server version 3.0.0b1
...

It did start the server, however the hub wouldn't redirect me to it.

This patch quells the warning, but more importantly causes hub to redirect me to the server properly. I don't know if anyone else has been able to use batchspawner as-is with jupyterhub 3.0.0b1.

This also updates the api request call to post data via the body parameter which seems like the right way to do that now according to tornado HTTPRequest docs.

[ryanlovett]

This fails tests for python 3.5, but the latest jupyterhub has a test framework starting at python 3.7. Two years ago the oldest was python 3.6. Thoughts?

[rcthomas]

Odd, I haven't had this problem with 3.0.0b1 (Python 3.9). My notebook server starts and I'm redirected without needing to make a change to singleuser.

[ryanlovett]

@rcthomas Okay, thanks. Do you see the warning message that HubAuth._api_request was never awaited?

I'm on python 3.8.

[rcthomas]

I do not see that warning message, no...

[ryanlovett]

I deployed a new singleuser venv and a new hub container, both based on python 3.10. I still had the issue without the patch, and it went away with the patch. So if something in my environment is causing it, it seems like it isn't some version-specific async-y thing in python.

Apparently the _api_request method became async only four months ago.

[rcthomas]

Just realized that while my hub is running JupyterHub 3.0.0b1, the separate environment where singleuser is executing isn't (they're separate deployments). When I upgrade that test environment I bet I see the same problem you're talking about.

[rcthomas]

I now confirm the bug with the same warning and failure to redirect, sorry about the earlier false negative! I can at least confirm this doesn't appear through JupyterHub 2.3.1 but starts at 3.x.

/global/common/software/nersc/current/jupyter/cgpu/test/lib/python3.9/site-packages/batchspawner/singleuser.py:16: RuntimeWarning: corout
ine 'HubAuth._api_request' was never awaited
  hub_auth._api_request(method='POST',
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

@mbmilligan thoughts on the test matrix?

[ryanlovett]

@rcthomas No worries, and thanks for testing! I'm relieved that it is reproducible.

[mbmilligan]

Thanks for chasing this down! Regarding the version matrix, two years ago takes you back to Hub 1.3.0. Digging further, it appears that 1.2 is the last Hub that included Python 3.5 in the test matrix, although later versions do claim to support Python 3.5 (earlier pythons don't work because of the new async syntax iirc). I would be very surprised if anyone upgrading batchspawner is still using a Hub that old.

Coming from another angle, the oldest widely used distribution among our userbase is probably RHEL/CentOS 7. Python 3.6 is available there (formerly via scl, in newer point releases it's in the base distro).

So yes, I think I'm okay with dropping testing support for Python 3.5. I wouldn't drop 3.6 support for a while yet though. As that's currently the newest Python that can easily be installed on those CentOS 7 systems, I think we're stuck with it until those systems go EOL in 2024.

[ryanlovett]

@mbmilligan Okay, thanks! I'll make a new issue about the matrix because I'm curious about the coverage you'd like there.

I think I'll need to update this PR to account for the case (<jh3) when the call is not async.

[mbmilligan]

Discussed in monthly meeting, let's discuss with JHub upstream to see if there's a better way to do this.

[ryanlovett]

Hi @minrk, there was some discussion at the last JupyterHub HPC meeting about how batchspawner currently invokes HubAuth's _api_request method. Is there a better way of doing this now?

It came up because _api_request recently became async which affected a test deployment running JH 3.0.0b1 and batchspawner. We could patch it via some variation of this PR, but should batchspawner use some other mechanism?

[minrk]

There should be a better way! A stable public hub_api_request method probably makes sense.

~all of the code in _api_request is informative error handling, though. You can make an API request without any private APIs, e.g. with requests (which was used in hub 2.x):

import requests
url = url_path_join(hub_auth.api_url, "batchspawner")
headers = {"Authorization": f"token {hub_auth.api_token}"}

# internal_ssl kwargs
kwargs = {}
if hub_auth.certfile and hub_auth.keyfile:
    kwargs["cert"] = (hub_auth.certfile, hub_auth.keyfile)
if hub_auth.client_ca:
    kwargs["verify"] = hub_auth.client_ca

r = requests.post(url, headers={"Authorization": f"token {hub_auth.api_token}"}, json=..., **kwargs)

[ryanlovett]

Okay, thanks @minrk ! I'll adjust this PR.

[rcthomas]

Hi @mbmilligan I don't think we discussed this PR at the recent HPC call directly but I was wondering what we need to do to move this to a release? I think updaring the test matrix was one element.

[jbeal-work]

There is another suggestion here #250

[jbeal-work]

And another which looks as if it should work nicely for both cases #251

[mawigh]

Is there any update on this pull request yet? I also find this solution more elegant.

We have a JupyterHub > v.3.0 in our HPC environment and have the possibility to start Jupyter notebook within custom Singularity containers. The changes would help to solve the problem communicating to the Hubs API.

[ryanlovett]

Hi @mawigh , this may check cleanly once 3.5 is no longer in the test matrix. Once that PR is merged we'll see if all of the tests in this PR will pass. People may be less available this time of year however so you might check in in January.

[mbmilligan]

Hi, just letting you know that #252 is merged now so you might want to try the tests on this again.

[ryanlovett]

Thanks @mbmilligan ! It reran checks after I merged master into this branch, but it is failing on what looks like unrelated code. (ORM, sqlalchemy)

[mbmilligan]

Okay my strong suspicion is that this is working correctly. I'm merging this so that people working from the main branch have this solution for 3.0 hubs while we work to sort out the test situation.

[cmd-ntrf]

The tests fail because the "install dependencies" step installs SQLAlchemy 2.x which is incompatible with JupyterHub < 3.1.1.

[jaescartin1]

Hi, I have the same issue using JupyterHub 4.0.0 with the last Batchspawner release 1.2 using pip/conda.
The merge in git master solved my issue in the DEV environment, but in our PROD system we only use pypi / conda-forge packages.

Is there any schedule date to launch another release that contain the patch?