Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs clarification culling behavior and configs #2267

Merged
merged 9 commits into from
Jul 20, 2021

Conversation

cdibble
Copy link
Contributor

@cdibble cdibble commented Jun 21, 2021

This addresses #2244. The goal is simply to clarify some of the documentation around the jupyterhub-idle-culler to differentiate it from culling behavior in Jupyter Notebook and make it easier to understand the definition of "activity" when users are customizing their Helm deployments.

@welcome

This comment has been minimized.

@cdibble
Copy link
Contributor Author

cdibble commented Jun 21, 2021

@consideRatio - Happy to revise as needed, but I think this makes sense.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent first pass!! Some smaller changes and I'm happy to merge!

@cdibble cdibble force-pushed the docs_clarification_culling branch from b04694c to f87179f Compare June 22, 2021 03:16
@cdibble
Copy link
Contributor Author

cdibble commented Jun 22, 2021

Thanks for the quick review and thoughtful comments @consideRatio - back to you. Happy to make further revisions as needed!

@consideRatio
Copy link
Member

@cdibble I decided to pick this up myself at this point. I figured that it makes sense to not document so much details in this repo but instead reference the jupyterhub-idle-culler repo with documentation so that it can be reused and maintained more centralized manner.

I've started writing a text but its not yet complete, I'll open a draft on that soon and look to push a commit to this PR referencing that documentation I write for the jupyterhub-idle-culler repo instead.

Does this sound okay to you?

@cdibble
Copy link
Contributor Author

cdibble commented Jun 24, 2021

@cdibble I decided to pick this up myself at this point. I figured that it makes sense to not document so much details in this repo but instead reference the jupyterhub-idle-culler repo with documentation so that it can be reused and maintained more centralized manner.

I've started writing a text but its not yet complete, I'll open a draft on that soon and look to push a commit to this PR referencing that documentation I write for the jupyterhub-idle-culler repo instead.

Does this sound okay to you?

Yep! Sounds great. I agree that this is a little over the top and am totally fine with paring it back a little bit for the sake of keeping docs organized under the appropriate repo. Feel free to ping me if you want me to make any updates/changes as well.

@consideRatio
Copy link
Member

@cdibble what do you think about these changes?


If you want to configure the culling of kernels that can help stop long running
code on the user servers, it can be useful to use
[`singleuser.extraFiles`](schema_singleuser.extraFiles).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate that this is referencing your solution here for providing the jupyter_notebook_config.py file, which is very useful. I think as written, though, it is hard to understand how singleuser.extraFiles can help with culling.

I might rewrite this as:

If you want to configure the culling of kernels using, for example, a kernel manager that can help stop long running
code on the user servers, it can be useful to use
[`singleuser.extraFiles`](schema_singleuser.extraFiles) to pass a `jupyter_notebook_config.py` file to `singleUser` servers with those configuration values like those documented in the [jupyterhub-idle-culler docs[(https://github.com/jupyterhub/jupyterhub-idle-culler#how-it-works) 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried addressing this in 6b8cef8! Thanks for your input!

@cdibble
Copy link
Contributor Author

cdibble commented Jun 25, 2021

Thanks for this effort @consideRatio! It's looking good. I made a few very minor typographical edits. I also made two slightly more substantive comments.

I would love to wrap my head around this issue of last_activity updates from the KubeSpawner class (or the base Spawner) with the update_last_activity call. Here's my comment on that. I think if I can understand where that last_activity value is updated for the server/spawner objects, it would help me patch the jupyterhub-idle-culler to avoid terminating kernels that are running processes but do not have open web sockets. As it stands, I can't seem to figure out how the last_activity value returned with the proxy.get_all_routes() call in update_last_activity is generated for each individual route.

I'd love to discuss this more, but I'll let you decide whether this is the right venue for such a discussion. If not, please let me know if you have any interest in exploring this elsewhere.

@cdibble
Copy link
Contributor Author

cdibble commented Jun 25, 2021

Naturally I found what I was looking for right after asking...

This should help. I'll keep digging.

@consideRatio
Copy link
Member

consideRatio commented Jul 18, 2021

Thanks for your review and effort on this @cdibble!

I'd love to discuss this more, but I'll let you decide whether this is the right venue for such a discussion. If not, please let me know if you have any interest in exploring this elsewhere.

Great that you found the notifiy_activity part! I suggest jupyter.discourse.org under the jupyterhub topic someplace perhaps.

@cdibble could you review the changes as they are now? If you agree, then I think we should go for a merge on this. I hope to cut a release with documentation in it this upcoming week.

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments, reviewing with @consideRatio . 👍

doc/source/jupyterhub/customizing/user-management.md Outdated Show resolved Hide resolved
doc/source/jupyterhub/customizing/user-management.md Outdated Show resolved Hide resolved
doc/source/jupyterhub/customizing/user-management.md Outdated Show resolved Hide resolved
consideRatio and others added 2 commits July 20, 2021 03:20
Co-authored-by: Min RK <benjaminrk@gmail.com>
@consideRatio
Copy link
Member

consideRatio commented Jul 20, 2021

Thanks @cdibble and @minrk!! I learned a lot from this collaborative work to describe how the culler system works!

@consideRatio consideRatio merged commit 6295a19 into jupyterhub:main Jul 20, 2021
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jul 20, 2021
jupyterhub/zero-to-jupyterhub-k8s#2267 Merge pull request #2267 from cdibble/docs_clarification_culling
@viktoriaas
Copy link

@consideRatio
Thanks for the great extension in docs. Culling is now much clearer than before. I got here from #2244 and to add my tiny bit, I think it would be also useful to add a comment about not enabling culling.

Many people want to ensure that their computations are not stopped after the browser tab is closed but there's still some mystery around the functionality. I myself am not sure about that, I just know the functionality is not implemented in core code in terms of checking whether the kernel is busy and not the connection to the browser. However, I think when jupyterhub-idle-culler is disabled

cull:
  enabled: false

and the MappingKernelManager options are left default (nothing is added to values.yaml), the notebook should stay "open" forever and even after closing the tab, the process is running.
I checked it with a simple python code where I count to 10000 with 1s sleep and print the number to file - after closing the tab and reopening an hour later, the numbers were being printed to the file continuously.

In my opinion, this is a very important part of the documentation, to clarify how to enable long-running computations and state the options.

@consideRatio
Copy link
Member

consideRatio commented Jul 26, 2021

@viktoriaas can you repost this docs improvement request as a dedicated issue? It makes a difference to me as I don't end up feeling i must do it or it will be forgotten in this PR.

Note that z2jh deploys a lot of software, but it can't be responsible for documenting everything from here. So when it comes to the user servers configuration, we are to some degree leaving the domain of what I think should be documented in z2jh - but I think its fine still to clarify that by setting cull.enabled=false JupyterHub won't interfere with long running computations, and that by default a jupyter server won't cull long running computations either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants