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

feat(debug): make it possible to connect a remote ipython shell #900

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

jansegre
Copy link
Member

@jansegre jansegre commented Dec 20, 2023

Motivation

In many situations we don't have an API/sysctl to control or adjust what's happening on the fullnode. The ipython shell is a very useful escape hatch to freely tinker with the code. Currently there is a shell subcommand, which in practice is helpful to interact with the storage and consensus, but it doesn't really allow the node to keep running.

Embedding an IPython kernel (aka ipykernel or ipkernel), allows a running node to be a server for a jupyter console (basically a remote ipython, either in a terminal or GUI), which basically provides a shell where all of the runtime can be accessed without stopping the node. This could even be used for debugging situations that are really hard to reproduce, but we could open a shell into a node where the case is happening.

Acceptance Criteria

  • Add an unsafe cli param --x-ipython-kernel (that currently depends on --x-asyncio-reactor) which enables ipython kernel in the fullnode using the mechanism expected by jupyter/ipython generating a kernel.json on the data dir, which then allows a client to connect/disconnect freely to the shell using jupyter console --existing /path/to/data/kernel.json

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@jansegre jansegre self-assigned this Dec 20, 2023
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (9b1bbcc) 85.26% compared to head (2410024) 85.19%.

Files Patch % Lines
hathor/ipykernel.py 36.66% 19 Missing ⚠️
hathor/builder/cli_builder.py 14.28% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #900      +/-   ##
==========================================
- Coverage   85.26%   85.19%   -0.07%     
==========================================
  Files         284      285       +1     
  Lines       22332    22369      +37     
  Branches     3368     3371       +3     
==========================================
+ Hits        19042    19058      +16     
- Misses       2613     2637      +24     
+ Partials      677      674       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jansegre jansegre force-pushed the feat/shell-ipykernel branch 2 times, most recently from 30205ef to 79e13f4 Compare January 5, 2024 00:00
@jansegre jansegre requested review from msbrogli and glevco January 5, 2024 20:30
@jansegre jansegre force-pushed the feat/shell-ipykernel branch from 79e13f4 to 049a33a Compare January 5, 2024 20:31
@jansegre jansegre marked this pull request as ready for review January 5, 2024 20:31
@jansegre jansegre force-pushed the feat/shell-ipykernel branch 3 times, most recently from 1c12222 to c75a085 Compare January 8, 2024 23:05
hathor/cli/run_node.py Outdated Show resolved Hide resolved
hathor/cli/run_node.py Outdated Show resolved Hide resolved
hathor/cli/run_node.py Outdated Show resolved Hide resolved
hathor/cli/util.py Outdated Show resolved Hide resolved
hathor/manager.py Outdated Show resolved Hide resolved
hathor/ipykernel.py Show resolved Hide resolved
hathor/ipykernel.py Show resolved Hide resolved
hathor/ipykernel.py Show resolved Hide resolved
@jansegre jansegre force-pushed the feat/shell-ipykernel branch from c75a085 to c85ef3e Compare January 12, 2024 01:52
msbrogli
msbrogli previously approved these changes Jan 12, 2024
hathor/builder/cli_builder.py Outdated Show resolved Hide resolved
hathor/ipykernel.py Show resolved Hide resolved
hathor/ipykernel.py Show resolved Hide resolved
hathor/cli/run_node.py Outdated Show resolved Hide resolved
hathor/cli/run_node.py Outdated Show resolved Hide resolved
hathor/cli/run_node.py Outdated Show resolved Hide resolved
hathor/cli/util.py Outdated Show resolved Hide resolved
hathor/manager.py Outdated Show resolved Hide resolved
hathor/ipykernel.py Show resolved Hide resolved
@@ -120,6 +121,9 @@ def create_parser(cls) -> ArgumentParser:
help=f'Signal not support for a feature. One of {possible_features}')
parser.add_argument('--x-asyncio-reactor', action='store_true',
help='Use asyncio reactor instead of Twisted\'s default.')
# XXX: this is temporary, should be added as a sysctl instead before merging
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we changing it in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but in a future PR. On my initial conversation with @msbrogli we thought it would be better to do it in sysctl, allowing it to be turned on/off. However since turning it in/off requires somewhat more careful management of the IPKernelApp, and since there's a dependency on the asyncio-reactor which is easier to make explicit as a CLI parameter. I went with the current implementation as the initial proof of concept of this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok! Maybe we should update this comment, then? Just to prevent future confusion

@jansegre jansegre merged commit 6c95fad into master Jan 15, 2024
8 of 9 checks passed
@jansegre jansegre deleted the feat/shell-ipykernel branch January 15, 2024 16:05
@jansegre jansegre mentioned this pull request Jan 30, 2024
2 tasks
@jansegre jansegre mentioned this pull request Feb 26, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants