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

PR: Wait until console It's ready before executing code. #5127

Closed
wants to merge 4 commits into from

Conversation

rlaverde
Copy link
Member

@rlaverde rlaverde commented Sep 4, 2017

Fix 1 and 2 of #4856

Fix error of banner and prompt being deleted when executing in a dedicated console.

Now It looks as expected when running in a dedicated console:
spectacle lh4836

- Fix error of banner and prompt being deleted when executing in a
dedicated console.
@rlaverde rlaverde added this to the v3.2.2 milestone Sep 4, 2017
@rlaverde rlaverde self-assigned this Sep 4, 2017
sw.silent_execute(
'get_ipython().kernel.close_all_mpl_figures()')
sw.reset_namespace(force=True)
elif current_client and clear_variables:
sw.reset_namespace(force=True)
# wait until console it's ready before executing code
self.wait_shell_is_ready(sw)
Copy link
Member

Choose a reason for hiding this comment

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

Please move this line to the if not current_client block.

@goanpeca
Copy link
Member

goanpeca commented Sep 4, 2017

@rlaverde does this mean that the code execution is now slower somehow, because of this timeout? or we take the same time, but simply display info at a later step?

loop = QEventLoop()
timer = QTimer()
timer.timeout.connect(loop.quit)
timer.start(300)
Copy link
Member

Choose a reason for hiding this comment

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

Are we waiting only 300 ms for the prompt to be ready?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I saw above that timeout=3000, but this should be higher. I think we should set it to 15000 at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are waiting a maximum of 3seconds, and checking every 300ms, testing in my machine It takes ~1.5 seconds to be ready

@ccordoba12 ccordoba12 modified the milestones: v3.2.2, v3.2.3 Sep 4, 2017
@rlaverde
Copy link
Member Author

rlaverde commented Sep 4, 2017

does this mean that the code execution is now slower somehow, because of this timeout?

It's a little slow the first time when executing in a dedicated console, It'll wait until console is ready before executing the script, the timeout is to avoid waiting too much time.

When running in an already open console, wait_shell_is_ready will return almost immediately, It won't enter the loop

@ccordoba12
Copy link
Member

ccordoba12 commented Sep 4, 2017

the timeout It's to avoid waiting too much time.

What do you mean by this? What happens if the timeout is finished?

@rlaverde
Copy link
Member Author

rlaverde commented Sep 4, 2017

What do you mean by this?

It the console is never ready (maybe for an error in the initialization), continuing this infinite loop could hang spyder, the timeout is the maximum time to wait for the console to be ready

What happens if the timeout is finished?

It'll return and the code will executed, maybe deleting the prompt, or mixing the output with the prompt

sw.silent_execute(
'get_ipython().kernel.close_all_mpl_figures()')
sw.reset_namespace(force=True)
# wait until console it's ready before executing code
self.wait_shell_is_ready(sw)
Copy link
Member

Choose a reason for hiding this comment

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

This has to be before the other two previous calls (i.e. sw.silent_execute and sw.reset_namespace) because those require a running kernel to work.

@rlaverde
Copy link
Member Author

rlaverde commented Sep 7, 2017

@ccordoba12 I moved the code, although I don't think it will be a difference, because the wait (for the banner) is necessary for commands that print something.

@ccordoba12 ccordoba12 modified the milestones: v3.2.3, v3.2.4 Sep 8, 2017
@ccordoba12 ccordoba12 removed this from the v3.2.4 milestone Sep 21, 2017
@ccordoba12
Copy link
Member

I was not too happy with this approach because I found it a bit too complicated. So I developed #5301, which I find simpler to understand.

@ccordoba12 ccordoba12 closed this Sep 21, 2017
@rlaverde rlaverde deleted the banner-ipyconsole branch September 21, 2017 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants