-
-
Notifications
You must be signed in to change notification settings - Fork 87
Fix(jupyter):use non-blocking stdin check to prevent hanging #380
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
base: main
Are you sure you want to change the base?
Conversation
Better to have some tests inserting data with Jupyter Notebook |
int flags = fcntl(fd, F_GETFL); | ||
if (flags != -1) | ||
{ | ||
fcntl(fd, F_SETFL, flags | O_NONBLOCK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changed the behavior of stdin empty check. The orginal one will block wait for data if not eol found.
I still think this mod is risky while processing large chunk of input data with slow IO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon confirmation:
-
The
std_in
check here is only used for INSERT statements on the clickhouse-local client side; engines like CSV read do not use it for reading. -
The interaction code between Python and C++ does not use the
std_in
fd
tests/test_data_insertion.ipynb
Outdated
"\n", | ||
"# Insert from INFILE and VALUES got stuck either\n", | ||
"# chs.query(\"INSERT INTO embeddings FROM INFILE 'movie_embeddings.csv' FORMAT CSV\")\n", | ||
"chs.query(\"INSERT INTO embeddings VALUES (1, [1,2,3,4,5,6,7,8,9,10])\")\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe gen some bigger CSV to test it is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
complete add more tests
It's worth noting the following code doesn't work correctly (we can't accept and insert data from standard input like clickhouse-local).
terminal run
Receive exception:
This issue has already existed in our main branch, introduced by commit 6123ace. This feature is mutually exclusive with the non-blocking Jupyter Notebook, so we can confirm that it is not supported. |
bca90c6
to
7974d23
Compare
use non-blocking stdin check to prevent hanging
close #260 and #152
Changelog category (leave one):
Documentation entry for user-facing changes
CI Settings
NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step
Run these jobs only (required builds will be added automatically):
Deny these jobs:
Extra options:
Only specified batches in multi-batch jobs: