jupyterhub / kubespawner

Kubernetes spawner for JupyterHub
https://jupyterhub-kubespawner.readthedocs.io
BSD 3-Clause "New" or "Revised" License
541 stars 303 forks source link

Allow building more complex profile_list templates #724

Closed yuvipanda closed 1 year ago

yuvipanda commented 1 year ago

The profile_list HTML / CSS was a single string embedded into the python code, and modifications were allowed as a single traitlet that had to be a fully complete jinja2 template.

This becomes difficult to manage very quickly, and means we lose out on all the compositional benefits of using jinja2 templates.

This PR will:

manics commented 1 year ago

https://setuptools.pypa.io/en/latest/userguide/datafiles.html#accessing-data-files-at-runtime recommends using importlib.resources instead of relying on direct filesystem access.

yuvipanda commented 1 year ago

@manics it looks like that would only work on python 3.10+ without adding an external dependency. From https://setuptools.pypa.io/en/latest/userguide/datafiles.html#accessing-data-files-at-runtime

importlib.resources was added to Python 3.7. However, the API illustrated in this code (using files()) was added only in Python 3.9, [3] and support for accessing data files via namespace packages was added only in Python 3.10 [4] (the data subdirectory is a namespace package under the root package mypkg). Therefore, you may find this code to work only in Python 3.10 (and above). For other versions of Python, you are recommended to use the importlib-resources backport which provides the latest version of this library.

I'd say it's ok to let this be, maybe add a note to remove it once we are python 3.10+ only?

yuvipanda commented 1 year ago

@manics the alternative is to add the additional dependency, which we currently don't have. I just checked to see if any jupyterhub or kubespawner dependency transitively includes that - turns out the answer is no. If you strongly prefer, I will add that dependency?

yuvipanda commented 1 year ago

(am working through the pre-commit situation now)

manics commented 1 year ago

If there's no easy fix then leave it as it is unless anyone else has thoughts

yuvipanda commented 1 year ago

hmm, not sure if the test failures are related.

yuvipanda commented 1 year ago

Per @consideRatio the test failures are unrelated https://github.com/jupyterhub/kubespawner/pull/721#issue-1673545073****

consideRatio commented 1 year ago

Per @consideRatio the test failures are unrelated https://github.com/jupyterhub/kubespawner/pull/721#issue-1673545073****

@yuvipanda the failures I meant would have shown up during the action-k3s-helm step, not in pytest. It seems that the failure seen here is recurring for the py37 env with oldest dependencies.

But, they seem to be indepent of this PR as when I triggered the test workflow in the main branch, I saw the failures as well: https://github.com/jupyterhub/kubespawner/actions/runs/4824271639

yuvipanda commented 1 year ago

@consideRatio makes sense. Let's investigate that separately from this PR?

consideRatio commented 1 year ago

@yuvipanda yes I agree, separate investigation.

I'm considering an conditional dependency for importlib_resources. I've seen Min do similar things in several places.

Python version constrained requirement, that is conditionally imported.

Reading this not confident on the implications or meaning, I would feel at ease knowing we do that.

It is strongly recommended that, if you are using data files, you should use importlib.resources to access them.

yuvipanda commented 1 year ago

@consideRatio ok, I've expanded the scope of this PR a little bit, and in the process got rid of the manual template loading :)

yuvipanda commented 1 year ago

Alright, I've completely removed the usage of __file__ from here!

yuvipanda commented 1 year ago

@consideRatio I have not changed the contents of the templates, so there should be no visual differences. Just extracted and autoformatted.

yuvipanda commented 1 year ago

@consideRatio how about now for the PR title & description?

manics commented 1 year ago

I haven't reviewed the latest code but I'm happy to go with @consideRatio

Are we happy that the wheel package correctly contains these extra files (from python -mbuild .), or can we rely on pyproject.toml/the build system to just "do the right thing"?

yuvipanda commented 1 year ago

@manics hmm, I see it in the sdist but not wheel! Investigating.

yuvipanda commented 1 year ago

@manics turns out hatch doesn't read MANIFEST.in, so I've included it separately in the wheel now. Good catch!

consideRatio commented 1 year ago

Nice work @yuvipanda !!!

yuvipanda commented 1 year ago

THANKS A LOT, @consideRatio!