Trigger child spawner's server property getter to make it exist

[rcthomas]

This addresses problems like #44 where the child_spawner.server attribute doesn't exist because it needs to be provoked into existing by hitting its getter. This part of the Spawner code shows how server comes into existence or not.

Per discussion at the HPC call Tuesday, we'll proceed by

• Update the PR to comment the rather odd-looking line added here (done)
• Add references to issues that are closed by merging this PR
• Open a new issue to track a better fix longer term upon closing this

[rcthomas]

@mbmilligan I've re-made the PR here so it's a bit cleaner and addresses a little disarray my fork had with it, sorry for any confusion

[rcthomas]

For this PR specifically, the main discussion about the problem and its "resolution" is in #44. There are hints about it in #24, but several of the open issues are mainly resolved by (closed) PR #45 such as #41.

Perhaps we can merge this PR and create an RC incorporating it and the work done in #45, and folks involved in those issues to test it and see if the issues reported there go away?

[jbeal-work]

"Per discussion at the HPC call Tuesday", Is this due to @rcthomas being at Berkley and Berkley being industrial partners ?

[rcthomas]

Hi @jbeal-work, there's a monthly JupyterHub in HPC call, first Wednesday of each month, you can find it and other similar events on the Jupyter Community calendar, https://discourse.jupyter.org/t/jupyter-community-calendar/2485, please feel free to join us in February!

Have you been able to test this branch? If so, can you let us know how it went?

[jbeal-work]

I have added the event in Febuarry to our groups diary. I will do my best to attend.

With the combination of this patch and the api one I have a web interface based around docker being able to submit jobs to our HPC clusters working so thank you very much.

                if isinstance(spawner, ProfilesSpawner):
                    spawner = spawner.child_spawner

I am a bit confused why the system built with out docker did not need the patches but I suspect they are so disimular.

( I am pretty sure the hacks I have put in batchspawner should be config however I am not a python expert ).

( I am going to try converting the data notebooker docker image workin ( I understand I will need to inject batchspawner in to the image )).

[minrk]

FWIW, I tried @manics config and see no errors both with and without this patch. My env:

pip freeze ``` alembic==1.7.6 anyio==3.5.0 appnope==0.1.2 argon2-cffi==21.3.0 argon2-cffi-bindings==21.2.0 asttokens==2.0.5 async-generator==1.10 attrs==21.4.0 Babel==2.9.1 backcall==0.2.0 bleach==4.1.0 certifi==2021.10.8 certipy==0.1.3 cffi==1.15.0 charset-normalizer==2.0.12 cryptography==36.0.1 debugpy==1.5.1 decorator==5.1.1 defusedxml==0.7.1 entrypoints==0.4 executing==0.8.3 greenlet==1.1.2 idna==3.3 ipykernel==6.9.1 ipython==8.1.0 ipython-genutils==0.2.0 jedi==0.18.1 Jinja2==3.0.3 json5==0.9.6 jsonschema==4.4.0 jupyter-client==7.1.2 jupyter-core==4.9.2 jupyter-server==1.13.5 jupyter-telemetry==0.1.0 jupyterhub==2.1.1 jupyterlab==3.2.9 jupyterlab-pygments==0.1.2 jupyterlab-server==2.10.3 Mako==1.1.6 MarkupSafe==2.1.0 matplotlib-inline==0.1.3 mistune==0.8.4 nbclassic==0.3.5 nbclient==0.5.11 nbconvert==6.4.2 nbformat==5.1.3 nest-asyncio==1.5.4 notebook==6.4.8 oauthlib==3.2.0 packaging==21.3 pamela==1.0.0 pandocfilters==1.5.0 parso==0.8.3 pexpect==4.8.0 pickleshare==0.7.5 prometheus-client==0.13.1 prompt-toolkit==3.0.28 ptyprocess==0.7.0 pure-eval==0.2.2 pycparser==2.21 Pygments==2.11.2 pyOpenSSL==22.0.0 pyparsing==3.0.7 pyrsistent==0.18.1 python-dateutil==2.8.2 python-json-logger==2.0.2 pytz==2021.3 pyzmq==22.3.0 requests==2.27.1 ruamel.yaml==0.17.21 ruamel.yaml.clib==0.2.6 Send2Trash==1.8.0 six==1.16.0 sniffio==1.2.0 SQLAlchemy==1.4.31 stack-data==0.2.0 terminado==0.13.1 testpath==0.6.0 tornado==6.1 traitlets==5.1.1 urllib3==1.26.8 wcwidth==0.2.5 webencodings==0.5.1 websocket-client==1.3.1 -e git+ssh://git@github.com/jupyterhub/wrapspawner.git@af05ff447c9d8abec6d08fa1e42fbc5852850bfa#egg=wrapspawner ```

But I suspect the root is that .server is an unnecessarily complex, cached property (not a trait) referencing .orm_spawner.server, so it's possible a cached value is being stored as None, losing its connection to the underlying/wrapper Server object.

A big part is that server is a property, not a trait. That means it can't be set via the constructor, as is attempted here. That line has no effect, though it should trigger this warning from traitlets:

# run with python -Wall test.py
from jupyterhub.spawner import SimpleLocalProcessSpawner

s = SimpleLocalProcessSpawner(server="xxx")
print(f"server={s.server}")

produces:

$PREFIX/lib/python3.9/site-packages/traitlets/config/configurable.py:85: DeprecationWarning: Passing unrecognized arguments to super(SimpleLocalProcessSpawner).__init__(server='xxx').
object.__init__() takes exactly one argument (the instance to initialize)
This is deprecated in traitlets 4.2.This error will be raised in a future release of traitlets.
  super(Configurable, self).__init__(**kwargs)
server=None

The lack of environment variable is this retrieval in get_env(). I don't quite understand why triggering the property getter would result in the right behavior, when it's the exact same property getter that seems to return the wrong value in one test and not in the other (at least for @manics).

The fix is perhaps to redefine the .server property on one class or the other to be a more direct link to the parent. However, redefining how properties work isn't something that's easy to do on instances - that's usually done on classes themselves. You could do:

class WrappedChild(self.child_class):

    # keep our server property in sync with our wrapper
    @property
    def server(self):
        return self.parent.server

    @server.setter(self):
    def server(self, new_server):
        self.parent.server = new_server

self.child_spawner = WrappedChild(parent=self, ...)

Alternately, the new property could be implemented in WrapSpawner, but I think that's actually more complicated, since there isn't easy access to super() in properties, so you'd have to duplicate private details of the .server property.

I think it would also work to clear the cached private ._server value:

delattr(child_spawner, '_server')

but that's reaching into some pretty private details.

If we rewrote .server in the base Spawner class as a trait instead of a property, the issue ought to go away, as the existing directional_link code would pick it up.

[minrk]

Now that I think of it, it's possible that this simple link on WrapSpawner:

@property
def server(self):
    return self.construct_child().server

@server.setter
def server(self, server):
    self.construct_child().server = server

ought to work, relying on the 'canonical' implementation residing in the child.

[rcthomas]

Agreed this is mainly because Spawner.server is not a trait and changing it to be such would fix this case. Thanks for verifying the proof of concept @manics.

The proposed property fix from @minrk is worth trying because it's much less mysterious looking than the vivification line. It could be documented in such a way that if an implementor comes up with yet another Spawner attribute they need that isn't a trait and needs similar handling (e.g. manual directional linkage) they'd have a fighting chance of knowing what to do. It would be much better if everything could be handled without that but I imagine there are reasons not everything is a traitlet on Spawner.

I'll set up and test this proposal out today.

[rcthomas]

See #51 for an alternative, based on Min's suggestion

[minrk]

I think this would also be fixed by https://github.com/jupyterhub/jupyterhub/pull/3810 which more rigorously keeps spawner.server in sync with spawner.orm_spawner.server. That PR has some explanation of why server isn't a trait (mainly that we can't observe secondary attributes like orm_spawner.server, so we have to check on every access, which traitlets doesn't do).

[mbmilligan]

Thanks @rcthomas and @minrk for thinking this through, I really like the alternative solution you've arrived at in #51. It sounds then like this PR should be closed in favor of merging that one.

[mbmilligan]

Improved solution merged from PR #51