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

Add documentation about how it works in README.md #28

Merged
merged 3 commits into from
Jul 21, 2021

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Jun 24, 2021

@cdibble started documenting details on how jupyterhub-idle-culler works in jupyterhub/zero-to-jupyterhub-k8s#2267, but I think such documentation should go here and be linked to from a distribution of jupyterhub like z2jh or tljh.

This PR is meant to help users understand jupyterhub-idle-culler a bit better so that they can better figure out how to configure it and better understand what to expect it can accomplish and how a kernel culler can work together with it.

A rendered preview of the README can be found here. I consider this ready for final review and merge at this point.

@consideRatio consideRatio added the documentation Improvements or additions to documentation label Jun 24, 2021
@consideRatio consideRatio requested review from minrk and yuvipanda June 24, 2021 19:10
@consideRatio consideRatio force-pushed the pr/add-docs-on-how-it-works branch from 711a6cc to caf8dca Compare June 24, 2021 19:34
@consideRatio consideRatio force-pushed the pr/add-docs-on-how-it-works branch from de82d8e to 236e20f Compare July 18, 2021 02:31
@consideRatio
Copy link
Member Author

consideRatio commented Jul 18, 2021

@cdibble commented the following on a commit i force-pushed away, making it disappear. Here it is again.

I can see that the update_last_activity method does look at individual spawner instances for each user where spawner = user.orm_spawners.get(route_data['server_name']). It looks like spawner.last_activity gets updated, but it's being updated with spawner.last_activity = max(spawner.last_activity, dt) where dt = parse_date(route_data['last_activity']). So update_last_activity can tell if a spawner instance has had more recent activity than the user model reflects, and update the latter.

But, how is the last_activity of the spawner instance (aka, route_data['last_activity']) set? I've looked at KubeSpawner and the base Spawner class, but the poll methods don't seem to actually update any time stamps. Is it set at the db level on commit?

I'm trying to understand how that last_activity timestamp is updated, because right now the behavior described here does not seem to match up with my experience. What I (and others) have seen is that Hub cannot see activity from spawner routes except when the browser connection is active.

I updated this PR and I think it now answers these kinds of questions @cdibble! Could you contribute with a review of this text perhaps?

@consideRatio
Copy link
Member Author

Thanks a lot for your insights about this during our session yesterday @minrk! I've made another pass now, what do you think?

@consideRatio consideRatio force-pushed the pr/add-docs-on-how-it-works branch from c793bdc to 1f31a56 Compare July 20, 2021 03:10
Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I've left some comments about detail, but I've a suggestion for different structure.

  1. What factors are taken into consideration for a notebook to be 'culled'. This would talk about CHP activity and when it is triggered, and same for the reports from the user server. This would be a high level description, and helps answer the question of 'What causes a notebook to be culled or not culled?'
  2. How these factors are calculated technically. This would point to code and add more details as you have done.

I hope this restructuring makes sense?

@consideRatio
Copy link
Member Author

Thanks @yuvipanda for your feedback!

I hope this restructuring makes sense?

It does, but I'm not eager to put in the work to get it done, feeling low on energy to iterate even further on this beyond tweaks =/

My work on this started out by trying to help a PR land in z2jh documenting the idle culler there, but as I felt most content should be upstreamed here, I started making this to assist that PR from landing in a sustainable way within z2jh.

Would you consider accepting this PR as it is without such restructuring and open an issue about the structure improvement? I pushed a commit to cover your other comments.

@yuvipanda
Copy link
Collaborator

It does, but I'm not eager to put in the work to get it done, feeling low on energy to iterate even further on this beyond tweaks =/

That is absolutely understandable. Thank you for being so open about it! I think I'll accept this PR as is. Do you feel comfortable opening an issue?

@yuvipanda yuvipanda merged commit 2fff987 into jupyterhub:master Jul 21, 2021
@consideRatio
Copy link
Member Author

Thanks @yuvipanda! I opened #29

@yuvipanda
Copy link
Collaborator

Thanks for making the ecosystem better documented, @consideRatio!

@cdibble
Copy link

cdibble commented Jul 22, 2021

Thanks for all of these efforts @yuvipanda and @consideRatio - sorry for starting us into the morass. But the docs are that much better now and that's great! Thanks again.

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

Successfully merging this pull request may close these issues.

3 participants