Fix common_traits calculation

[rcthomas]

With traitlets 5, a trait_values() method was added to HasTraits that seems like a better fit here. Indeed it seems to resolve problems like #41 and others.

Without this change, but with traitlets 5 installed, a number of traits in self.child_spawner._trait_values.keys() are omitted relative to with traitlets 4. Switching this to self.child_spawner.trait_values().keys() returns more of these but other ones as well. This may be the consequence of some kind of effective **metadata argument "applied" in traitlets 4, I haven't looked.

I've marked this WIP because while it appears to fix the reproducer in #41, there may be other consequences and I'd like to try it out on my dev deployment before asking for a merge. I would also encourage others who have seen traitlets 5 break their JupyterHub+wrapspawner deployments to try this out and report back.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[rcthomas]

So this doesn't magically fix #44 but the fix there is interesting, might be related. FWIW I see the same thing as in #44 on my deployment when using traitlets 5. I'll keep looking...

[minrk]

It looks like you are only interested in the trait names, so maybe the .trait_names() method is right for you? I don't think that changed in traitlets 5. .trait_values() goes through and does getattr for every trait, which isn't used since you call .keys() on the dict anyway.

._trait_values, unlike .trait_values() is a private cache of currently set values, and will exclude anything that hasn't been populated yet (e.g. traits that have not been configured or accessed yet). Implementation details can affect when that happens, which might be why you see a difference with traitlets 5. Whereas .trait_values() gets the values of all traits. .trait_names() gives you the names of all traits (i.e. is equivalent to list(obj.trait_values().keys())).

[rcthomas]

Thanks @minrk, right, what .trait_values() does is just this one liner:

    return {name: getattr(self, name) for name in self.trait_names(**metadata)}

Since we're not doing any metadata filtering on the traits I think this would turn into basically the same answer as .trait_names() but it does seem more clean, correct, and compatible with traitlets 4 and 5 to use .trait_names(). I'll go with that then.

It's been a while but maybe @cmd-ntrf has a reason he was trying to use ._trait_values (the private cache of current values). If there's no problem switching this might clear up more than just one issue, a few of us have found ourselves having to manually link traits.

[rcthomas]

Gives identical answers for common_traits now regardless of traitlets 4 vs 5

[rcthomas]

I've taken off the WIP: prefix and retitled the PR. I think this is worth considering for merge at this point.

[cmd-ntrf]

It's been a while but maybe @cmd-ntrf has a reason he was trying to use ._trait_values (the private cache of current values). If there's no problem switching this might clear up more than just one issue, a few of us have found ourselves having to manually link traits.

No specific reason on my part, I probably found out about the existence of _trait_values by digging through traitlets code or doing some live digging on traitlets object with dir() and, my bad, did not look for the right method to access the attribute once I found what I was looking for to solve the issue.

[minrk]

ex post facto justification: ._trait_values() would have allowed you to only look at values that have been set explicitly already. This is conceivably an optimization to avoid looking at traits that haven't been set and thus would necessarily use their default values. But unless looking at traits that happen to have default values (which could still be derived on access from other values) is a problem, that's probably not worth it.

[ftorradeflot]

I was encountering #41 also after upgrading the environment we use to provide the jupyterhub service. It took a while to find out that, due to the update of the traitlets package, the port attribute was not being propagated from the parent ProfileSpawner to the child BatchSpawner. I tried the version in this MR and fixed the bug for me. Moreover, it relies on the public API instead of private methods so I think it should me merged. Maybe a explicit dependency on traitlets>=5 should be set, because wrapspawner won't work anymore with traitlets<5.

[rvalieris]

thanks a lot for this PR @rcthomas, I ran into both #41 and #44 issues and applying this patch makes everything works as expected on traitlets==5.0.5

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[kgutwin]

Can this be released as a new version? It fixed my problem with batchspawner/profilesspawner on SLURM which must have been caused by the upgrade to traitlets version 5, and I'd rather not have to specifically install from Git in my build template.

[katringoogoo]

same here, just ran into this problem today and it cost a lot of time :/ ... releasing a 1.0.1 seems justified to me