search

jupyterhub / batchspawner

Custom Spawner for Jupyterhub to start servers in batch scheduled systems
BSD 3-Clause "New" or "Revised" License
190 stars 134 forks source link

maint: req py36+ and jh 1.5.1+, fix tests, add RELEASE.md, add pre-commit hooks, add dependabot #273

Closed consideRatio closed 1 year ago

consideRatio commented 1 year ago

This PR is making some progress towards general maintenance needs tracked in #270, doing things done for other repos in the jupyterhub org.

It is a large PR that combines various things that could be made into at least some separate PRs, but has for now been combined to avoid merge conflicts. If its too much work to review in this form or needs various updates, I'll try to extract a few PRs to make it easier to review going onwards.

consideRatio commented 1 year ago

Ping also to @rkdarst who has contributed a lot to this repo!

mbmilligan commented 1 year ago

@consideRatio Hi Erik, just to note that this is much needed work and I'm really happy to see you taking it on! I'm on vacation through July 5 so I probably won't have time to dig into a review before then.

consideRatio commented 1 year ago

Hi @mbmilligan I hope you are well!

Do you think you will find time to review this PR and others to get us towards a release? I also wondered if you feel that its fine if someone else in the jupyterhub team reviewed and merged some PRs?

mbmilligan commented 1 year ago

I would absolutely welcome other folks familiar with the codebase helping out with reviews and testing of PRs. I prefer to have a single person actually pushing the merges, but I could do that quickly if given good documentation that proposed changes are okay to merge.

On Tue, Sep 19, 2023, 6:24 AM Erik Sundell @.***> wrote:

Hi @mbmilligan https://github.com/mbmilligan I hope you are well!

Do you think you will find time to review this PR and others to get us towards a release? I also wondered if you feel that its fine if someone else in the jupyterhub team reviewed and merged some PRs?

— Reply to this email directly, view it on GitHub https://github.com/jupyterhub/batchspawner/pull/273#issuecomment-1725313711, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACR7QZ5S322FUCHDY4XH3P3X3F6GPANCNFSM6AAAAAAZLSWLDY . You are receiving this because you were mentioned.Message ID: @.***>

yuvipanda commented 1 year ago

Upon re-reading @mbmilligan's comment in https://github.com/jupyterhub/batchspawner/pull/273#issuecomment-1725817717, let me amend to say that I'm very happy to approve all the changes except for 3.6 support dropping. My interpretation of the comment is that @mbmilligan would still prefer to be the one pushing the merge button so am happy to leave that up to them :)

consideRatio commented 1 year ago

Thanks for reviewing @yuvipanda and @mbmilligan!

I've pushed a commit reverting dropping support for python 3.6. For reference note that jupyterhub 3 requires python 3.7, so users with python 3.6 are stuck with jupyterhub 2 for now also.

I also pushed a code coverage related config omitting coverage of tests.

yuvipanda commented 1 year ago

I've reviewed and approved this, @mbmilligan! I am not 100% sure if your comment meant you want to be the person hitting merge, so I'll just give this a few days before I hit merge :)

yuvipanda commented 1 year ago

@consideRatio I think you can also have the 3.6 drop as a separate PR! I personally think it's ok to drop that, but I'd like someone from the HPC community to make that choice. We can definitely 100% do so once CentOS EOLs in 2024, but hopefully someone from the community chimes in and says it's ok to do now :D

mahendrapaipuri commented 1 year ago

We use batchspawner on our HPC platform and well, we dont really rely on python distribution shipped out of OS. We install all Jupyter related stack in a separate conda/mamba environment. Moreover JupyterHub 4+ supports only Python 3.8+, so, I do not really see a point on supporting Python 3.6+ for batchspawner.

Saying so, I agree with @yuvipanda that we should drop support for Python 3.6 in a separate PR. This is due to the fact that there is no release of batchspawner that supports JupyterHub 3. (#277) which supports Python 3.7. So, I think it makes sense to make a release that is compatible with JupyterHub 3. and then drop support for the next releases that are meant for JupyterHub 4.*.

yuvipanda commented 1 year ago

@mbmilligan are you ok with me merging this PR now?

mbmilligan commented 1 year ago

@mbmilligan are you ok with me merging this PR now?

Thanks for the ping and for the work pushing this forward. Merging now.

mbmilligan commented 1 year ago

We use batchspawner on our HPC platform and well, we dont really rely on python distribution shipped out of OS. We install all Jupyter related stack in a separate conda/mamba environment. Moreover JupyterHub 4+ supports only Python 3.8+, so, I do not really see a point on supporting Python 3.6+ for batchspawner.

Saying so, I agree with @yuvipanda that we should drop support for Python 3.6 in a separate PR. This is due to the fact that there is no release of batchspawner that supports JupyterHub 3. (#277) which supports Python 3.7. So, I think it makes sense to make a release that is compatible with JupyterHub 3. and then drop support for the next releases that are meant for JupyterHub 4.*.

That was my thinking as well. I'll merge this PR now and set up a release, and after that release we can drop additional support lines.

yuvipanda commented 1 year ago

Thank you for the merge, @mbmilligan!

And thank you for doing all this work, @consideRatio!