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

Python Script progress log #4276

Closed
Hrovatin opened this issue Dec 18, 2019 · 9 comments
Closed

Python Script progress log #4276

Hrovatin opened this issue Dec 18, 2019 · 9 comments
Labels
feast This may require a few weeks of work wish

Comments

@Hrovatin
Copy link
Contributor

Currently the Python Script widget prints to the console only after it has finished running and there is no indicator of whether it is running or not.
It should be somehow indicated whether the script is currently running or not. Otherwise it is not clear whether 1.) the script even started running and 2.) why Orange is unresponsive.

@irgolic
Copy link
Member

irgolic commented Dec 18, 2019

Something akin to a radio button next to the Run button, that's red when the script is running. Maybe this should be standardized across all widgets to show running status not only on canvas, but also in the widget's interface.

I think Orange would benefit a lot from a strong Python script widget, but I appreciate it's a relatively big project.

Maybe after fixing this issue, we could separate out some smaller tasks, like:

  • Add line numbers
  • Rework the Library view (e.g., changing Update to Save),
  • Run script in parallel thread/process, so stdout shows as the script runs

Also (the biggest task), maybe we could improve the data table (in_data/out_data) interface in some way. A user in the Discord support channel was confused why in_data['Date',]= str(datetime.datetime.strptime(date, "%d.%m.%Y")) wasn't working. The closer the exposed interface is to something users are familiar with, the flatter the widget's learning curve.

(sorry for hijacking your issue)

@ajdapretnar
Copy link
Contributor

A user in the Discord support channel was confused why in_data['Date',]= str(datetime.datetime.strptime(date, "%d.%m.%Y")) wasn't working.

This is because most people are used to working with pandas, but Orange's data structures are not like that. That said, this is something only @janezd can answer.

@pavlin-policar
Copy link
Collaborator

pavlin-policar commented Dec 18, 2019

While the script widget can be improved in lots of ways, the thing that annoys me the most is that stdout is not flushed while running. In this simple example,

import time
time.sleep(2)
print('Hello world')
time.sleep(2)
print('Hello world')
time.sleep(2)
print('Hello world')

hello world is printed three times when everything finishes, not every two seconds. The fix for this is probably just adding sys.stdout.flush() after every line and probably wouldn't require a major rewrite.

Also, in addition to printing actual stdout, I would suggest outputting the commands, like ipython does. So the output for the simple example above would be:

>>> import time
>>> time.sleep(2)
>>> print('Hello world')
Hello world
>>> time.sleep(2)
>>> print('Hello world')
Hello world
>>> time.sleep(2)
>>> print('Hello world')
Hello world

instead of just

Running script
Hello world
Hello world
Hello world

Also, the most Orange-y way to indicate that things are running would be to add a progress bar.

@janezd
Copy link
Contributor

janezd commented Dec 19, 2019

The widget uses PythonConsole, which does patch('sys.stdout', self), and calling sys.stdout.flush:

    def flush(self):
        pass

PythonConsole.write already adds show the output without any flushing, but Qt doesn't update the window until the script finishes because, if I understand correctly, the script runs in the main thread, just as signal processing. One way of achieving the update is to put

        qApp.processEvents()

to the end of PythonConsole.write.

@ales-erjavec should say whether this is correct and safe. A better way could be to run the console in a separate thread.

@janezd
Copy link
Contributor

janezd commented Dec 19, 2019

When does ipython output commands?

@janezd janezd added the needs discussion Core developers need to discuss the issue label Dec 19, 2019
@ales-erjavec
Copy link
Contributor

processEvents must not be called when the event loop is already running.

@janezd
Copy link
Contributor

janezd commented Dec 27, 2019

The script must be executed in a separate thread to allow the main thread to update? But PythonConsole, which is derived from QWidget, must remain in the main thread? That is, class PythonConsole(QPlainTextEdit, code.InteractiveConsole) should be separated into two classes and the latter moved into a worker thread?

@janezd janezd removed needs discussion Core developers need to discuss the issue suggestion labels Jan 1, 2020
@janezd
Copy link
Contributor

janezd commented Feb 24, 2020

I moved code.InteractiveConsole to another thread. I pass commands through signals. Do I understand correctly that these signals are then processed in the other thread's event loop, from which it is safe to call qApp.processEvents() to process GUI updates, because these are different event loops?

Why is it prohibited (or just not recommended?) to call processEvents while signals are being processed? Infinite recursion? If so, even my initial solution (without an extra thread) isn't problematic because it cannot lead to recursion (assuming we disable the Run button when the script is being executed). Or are there other reasons?

@ales-erjavec
Copy link
Contributor

Do I understand correctly that these signals are then processed in the other thread's event loop ...

Assuming correct setup yes.

... from which it is safe to call qApp.processEvents() to process GUI updates, because these are different event loops?

GUI updates cannot be processed from other threads. Calling processEvents() can only process events enqueued to the calling thread's event loop.

Or are there other reasons?

The most problematic is the execution (completion) ordering inversion due to reentry.

Most of https://stackoverflow.com/questions/35561182/why-should-nesting-of-qeventloops-be-avoided applies (processEvents is just the worst kind of event loop nesting).

Also https://lists.qt-project.org/pipermail/interest/2019-September/033855.html

@janezd janezd self-assigned this Mar 6, 2020
@janezd janezd removed their assignment Mar 13, 2020
@janezd janezd added feast This may require a few weeks of work wish labels Oct 22, 2021
@janezd janezd added needs discussion Core developers need to discuss the issue and removed needs discussion Core developers need to discuss the issue labels Jan 10, 2023
@janezd janezd closed this as completed Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feast This may require a few weeks of work wish
Projects
None yet
Development

No branches or pull requests

6 participants