Console stdout stderr #148

tigercosmos · 2022-11-09T17:45:34Z

TODO checklist:

yungyuc · 2022-11-09T23:14:15Z

Is it correct that the PR is still in draft mode and not ready for review?

yungyuc · 2022-11-09T23:25:37Z

I tested the code a little bit. Some issues:

print(module) does not show anything.
Mouse double-clicking in the history box selects either a single word (good; that's what we want) or the whole box (not helpful). I would like double-clicking selects either a single words or a whole line but excluding the prompt. Selecting only an output section may be useful too.
The history text box does not scroll to the bottom when new command is added beyond the text area.
Conventionally the Python interactive command prompt is >>> , not $ , which looks strange to a Python user.

cpp/modmesh/python/common.hpp

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

tigercosmos · 2022-11-12T17:54:07Z

modmesh/apputil.py

        # Each run of the application appends a new environment.
-        environ[name] = self
+        environ[config['appEnvName']] = self


IMO, this line should live in new_appenv

Do not mingle the non-GUI construct environ with GUi widget. Avoid taking the name from config. The config object should be removed too.

modmesh/apputil.py

modmesh/system.py

cpp/modmesh/view/RPythonConsoleDockWidget.hpp

tigercosmos · 2022-11-12T18:00:32Z

modmesh/system.py

+        'redirectStdErrFile': 'stderr_viewer.txt',
+    }
+
+    # setup new app environment


Python Console shares the same config with AppEnvironment

currently, only one AppEnvironment is created at the beginning.

in the future, if there is new AppEnvironment created, Python Console should also update its config

modmesh/apputil.py

yungyuc

I think most of the change in apputil.py and system.py should be reverted. You added too much Python code and it does not look right. Most of the I/O redirection should be done in C++, not Python. The change in Python should simply changing the underneath file descriptors.

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

cpp/modmesh/view/RPythonConsoleDockWidget.hpp

cpp/modmesh/view/wrap_view.cpp

yungyuc · 2022-11-12T23:48:34Z

cpp/modmesh/view/wrap_view.cpp

-                    return self.command().toStdString();
-                },
+                "command", [](wrapped_type const & self)
+                { return self.command().toStdString(); },


Do not change the style.

yungyuc · 2022-11-12T23:50:24Z

modmesh/apputil.py

        # Each run of the application appends a new environment.
-        environ[name] = self
+        environ[config['appEnvName']] = self


Do not mingle the non-GUI construct environ with GUi widget. Avoid taking the name from config. The config object should be removed too.

modmesh/apputil.py

yungyuc · 2022-11-13T00:24:42Z

modmesh/system.py

@@ -83,6 +82,19 @@ def _run_viewer(argv):
    wm.windowTitle = "Modmesh Viewer"
    wm.resize(w=1000, h=600)
    wm.show()
+
+    config = {


Do not use a Python dictionary or C++ hash or tree to keep configurations. They are slow for HPC.

There are cases that we may tolerate the slowness of a hash/tree, but I do not want to make it enter modmesh so soon. It delivers a wrong impression to devs and users.

modmesh/system.py

ezio-melotti · 2022-11-16T16:53:58Z

modmesh/apputil.py

+        with open(self._config['redirectStdOutFile'], 'w') as f1:
+            with contextlib.redirect_stdout(f1):
+                with open(self._config['redirectStdErrFile'], 'w') as f2:
+                    with contextlib.redirect_stderr(f2):


Here you can use (parenthesized context managers)[https://docs.python.org/3.10/whatsnew/3.10.html#parenthesized-context-managers) or ExitStack to avoid the multiple levels of indentation.

modmesh/apputil.py

ezio-melotti · 2022-11-16T17:37:47Z

modmesh/apputil.py

+                        sys.stdout.flush()
+                        sys.stderr.flush()


You should be able to do print(..., flush=True) directly if you only have one print. If you have multiple, doing it once at the end might be better.

ezio-melotti · 2022-11-16T17:38:01Z

modmesh/apputil.py

+
+def new_appenv(config=None):
+    name = config.get('appEnvName', None)
+    if None is name or name == '':


Suggested change

if None is name or name == '':

if name is None or name == '':

For making the two conditions in the line use consistent style, changing the order is good for reading.

If not in this scenario, (like a single condition), I prefer putting constant in the first place in the conditional checking. It is the same as a convention commonplace in C code, which is to reduce the chance of typo to treat equality == as assignment =, because

if (nullptr = pointer)

is an error compiler must complain about.

With the aid of modern compilers or clang-tidy, the trick is not absolutely necessary, but good to have.

My preference may change if a convincing argument is provided.

Three reasons:

With a single = in the condition Python already complains, so there is no risk of accidental assignment:

>>> name = 'foo' >>> if name = None: pass File "<stdin>", line 1 if name = None: pass ^^^^^^^^^^^ SyntaxError: invalid syntax. Maybe you meant '==' or ':=' instead of '='?

In this case we are using is instead of ==, so there is no risk of typos and accidentally missing an =.

variable == constant reads better and is the convention followed in Python (and many other languages), and in this specific situation it is also symmetric with the second part of the condition

modmesh/apputil.py

ezio-melotti · 2022-11-16T17:43:43Z

modmesh/apputil.py

        i = 0
        name = 'anonymous%d' % i
        while name in environ:
            i += 1
            name = 'anonymous%d' % i
    app = environ.get(name, None)
+
    if None is app:


Suggested change

if None is app:

if app is None:

This is where constant should be put in the first position of the condition.

ezio-melotti · 2022-11-16T17:44:02Z

modmesh/apputil.py

-        raise KeyError("No AppEnviron is available")
-    aenv = environ[k]
-    aenv.run_code(code)
+    if None is app:


Suggested change

if None is app:

if app is None:

tigercosmos · 2022-11-21T17:08:12Z

modmesh/apputil.py

+            if self._stderrFd <= 0:
+                raise Exception("wrong stderr fd:", self._stderrFd)
+
+            sys.stdout = msvcrt.get_osfhandle(self._stdoutFd)


failed here, although self._stdoutFd could be 3 from C++ fopen, and msvcrt.get_osfhandle translates it to Python's handle, but it seems python cannot take this FD, probably cannot share the same FD for different processes.

OSError: [Errno 9] Bad file descriptortraceback: File "C:\Users\tiger\modmesh\modmesh\system.py", line 105, in enter_main ret = _run_viewer(argv) File "C:\Users\tiger\modmesh\modmesh\system.py", line 80, in _run_viewer return view.launch() File "C:\Users\tiger\modmesh\modmesh\view.py", line 95, in launch return app.exec() File "C:\Users\tiger\modmesh\modmesh\system.py", line 117, in exec_code apputil.run_code(code, redirectStdOutFd, redirectStdErrFd) File "C:\Users\tiger\modmesh\modmesh\apputil.py", line 129, in run_code aenv.run_code(code) File "C:\Users\tiger\modmesh\modmesh\apputil.py", line 99, in run_code traceback.print_stack()

Is there a reason that you have been trying to deal with the fd in Python, but not directly in C/C++? Wouldn't it be easier to simply handle it in CPython API?

I haven't tried if I can get a python file object in C++, I could try.

modmesh/apputil.py

ezio-melotti · 2022-11-21T22:50:05Z

modmesh/apputil.py

+
+            sys.stdout = oldStdoutFd
+            sys.stderr = oldStderrFd
+        except Exception as e:


Here you could either use ValueError or leave Exception if you want to catch other types of exception too.

ezio-melotti · 2022-11-21T22:51:19Z

modmesh/apputil.py

+        except Exception as e:
+            sys.stdout = oldStdoutFd
+            sys.stderr = oldStderrFd
+            sys.stderr.write(("{}: {}".format(type(e).__name__, str(e))))


Suggested change

sys.stderr.write(("{}: {}".format(type(e).__name__, str(e))))

sys.stderr.write(f"{type(e).__name__}: {e}")

There might be something in the traceback module that does this for you already.

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

yungyuc · 2022-11-21T23:25:05Z

modmesh/apputil.py

-        exec(code, self.globals, self.locals)
+        import msvcrt
+
+        oldStdoutFd = sys.stdout


The names baffled me. I think you intended to mean old_stdout_fobj?

modmesh/apputil.py

yungyuc · 2022-11-21T23:31:00Z

modmesh/apputil.py

+            if self._stderrFd <= 0:
+                raise Exception("wrong stderr fd:", self._stderrFd)
+
+            sys.stdout = msvcrt.get_osfhandle(self._stdoutFd)


Is there a reason that you have been trying to deal with the fd in Python, but not directly in C/C++? Wouldn't it be easier to simply handle it in CPython API?

yungyuc · 2022-11-21T23:34:14Z

modmesh/apputil.py

@@ -88,14 +114,17 @@ def get_appenv(name=None):
 get_appenv(name='master')


-def run_code(code):
+def run_code(code, redirectStdOutFd=-1, redirectStdErrFd=-1):


In my imagination, the code handling fd would be more straight-forward if they are in C/C++. In Python, the fd should be wrapped by the Python file object, which is used almost everywhere in Python.

tigercosmos · 2022-11-22T16:35:55Z

modmesh/system.py

    try:
+        sys.stdout = msvcrt.get_osfhandle(redirectStdOutFd)


still OSError: [Errno 9] Bad file descriptor

@yungyuc I feel that we cannot open fds in C++, because Python cannot access those fds.
I also tried opening fds in Python and share to C++, but also not work.
C++ and Python should be the same process, so fd should be able to share?

Is there a reason that you have been trying to deal with the fd in Python, but not directly in C/C++? Wouldn't it be easier to simply handle it in CPython API?

maybe I should try this

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

tigercosmos · 2022-11-22T17:56:25Z

I also tried to add the following in system.py

r_stdout, w_stdout = os.pipe()
r_stderr, w_stderr = os.pipe()

sys.stdout = os.fdopen(r_stdout, "w+")
sys.stderr = os.fdopen(r_stderr, "w+")

def get_stdout_redirect_fd():
    return w_stdout

def get_stderr_redirect_fd():
    return w_stderr

and in the C++ code, call interp.get_stdout_redirect_fd() to get the fd, and then call _read(fd) ; however, this way doesn't work as well.

tychuang1211 · 2022-11-22T21:35:34Z

I was testing the code and it seemed that not only did print(module) not show anything, but everything enclosed in angle brackets also didn't show. And I also checked the stream before and after redirection, both were correct.

After this I found that the inserted Html in RPythonConsoleDockWidget::printCommandStdout and RPythonConsoleDockWidget::printCommandStderr were not escaped. We could simply change QString::fromStdString(message) to QString::fromStdString(message).toHtmlEscaped() in both function to fix the problem.

tychuang1211 · 2022-11-22T21:42:41Z

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

+    QTextCursor cursor = m_history_edit->textCursor();
+    cursor.movePosition(QTextCursor::End);
+    m_history_edit->setTextCursor(cursor);
+    m_history_edit->insertHtml(m_stdoutHtml + QString::fromStdString(stdout_message) + m_endHtml);


Suggested change

m_history_edit->insertHtml(m_stdoutHtml + QString::fromStdString(stdout_message) + m_endHtml);

m_history_edit->insertHtml(m_stdoutHtml + QString::fromStdString(stdout_message).toHtmlEscaped() + m_endHtml);

I think this is good! But perhaps for simple coloring, we may also consider to directly use Qt API for changing color? I am curious which way works better for the terminal.

thanks @tychuang1211, I didn't think about toHtmlEscaped. btw, which code did you test, I think it's not the latest commit?
@yungyuc, it's inconvenient to set different colors for text if we use QT way. let's just use HTML way.

@tigercosmos I am OK to use HTML there for now, but convenience should not be the major factor for deciding which way to go. The more indirect the longer runtime it takes. To some point we will need to evaluate and pick a "better" way, of which the metrics are not defined at the moment.

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

Co-authored-by: tychuang1211 <33109379+tychuang1211@users.noreply.github.com>

Console stdout

This reverts commit a9d5f7d.

This reverts commit a637592.

Redirect Python output to Qt Widget

yungyuc

With some cosmetic improvements we are good to check in!

yungyuc · 2022-11-24T08:55:51Z

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

+    PyStdErrOutStreamRedirect()
+    {
+        auto sysm = pybind11::module::import("sys");
+        _stdout = sysm.attr("stdout");


Please avoid using underscore _ in the beginning of a variable, because the standard reserves them: https://softwareengineering.stackexchange.com/a/191833

yungyuc · 2022-11-24T08:56:34Z

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

@@ -33,6 +33,43 @@
 namespace modmesh
 {

+class PyStdErrOutStreamRedirect
+{
+    pybind11::object _stdout;


Member data names should start with m_.

yungyuc · 2022-11-24T08:58:12Z

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

+        sysm.attr("stdout") = _stdout_buffer;
+        sysm.attr("stderr") = _stderr_buffer;
+    }
+    std::string stdoutString()


It's not a Qt function, and the name should follow the snake_case convention.

yungyuc · 2022-11-24T09:34:16Z

cpp/modmesh/view/RPythonConsoleDockWidget.cpp

    m_command_edit->setPlainText("");
    m_command_string = "";
    m_current_command_index = static_cast<int>(m_past_command_strings.size());
    auto & interp = modmesh::python::Interpreter::instance();
+    PyStdErrOutStreamRedirect pyOutputRedirect{};


snake_case for local variables.

This is elegant. Not even need a line of comment.

tigercosmos · 2022-11-24T10:19:01Z

@yungyuc ready for merge

tigercosmos · 2022-11-24T10:54:38Z

/home/runner/work/modmesh/modmesh/cpp/modmesh/python/common.cpp:208:28: error: an exception may be thrown in function '~PyStdErrOutStreamRedirect' which should not throw exceptions [bugprone-exception-escape,-warnings-as-errors]

any idea?

yungyuc · 2022-11-24T11:02:55Z

/home/runner/work/modmesh/modmesh/cpp/modmesh/python/common.cpp:208:28: error: an exception may be thrown in function '~PyStdErrOutStreamRedirect' which should not throw exceptions [bugprone-exception-escape,-warnings-as-errors]

any idea?

https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-exception-escape.html

tychuang1211 · 2022-11-24T11:17:10Z

/home/runner/work/modmesh/modmesh/cpp/modmesh/python/common.cpp:208:28: error: an exception may be thrown in function '~PyStdErrOutStreamRedirect' which should not throw exceptions [bugprone-exception-escape,-warnings-as-errors]

any idea?

the pybind11::module::import("sys") in ~PyStdErrOutStreamRedirect may throw an exception (pybind11::error_already_set)

yungyuc · 2022-11-24T12:01:45Z

cpp/modmesh/python/common.cpp

+    return pybind11::str(m_stderr_buffer.attr("read")());
+}
+
+PyStdErrOutStreamRedirect::~PyStdErrOutStreamRedirect() noexcept(true)


Because you still may throw an exception, it should be noexcept(false). Since no specifying noexcept means noexcept(false), it's not really needed.

because I only catch pybind11::error_already_set here?

also, if I use try catch(...) for the whole deconstruction function, will it throw?

I do not fully understand your questions. Could you be more specific?

Because you still may throw an exception, it should be noexcept(false)

is it because I only catch pybind11::error_already_set in the code?

if I write:

try { // .. } catch (...) // capture all exception { // ... }

will it still throw exception?

I do not know. noexcept means the function does not throw exception. I am not sure it means no exception handling is allowed in the function, or no except is thrown outside the function. I guess it's the latter, but if it's the former, a try catch block in noexcept would be problematic.

I would be interested in the behavior too.

it seems no difference for the behavior when noexcept is true or false if we catch all errors

struct Foo { Foo(){} ~Foo() noexcept(true){ try { throw "123"; } catch(...) { // } } }; int main() { Foo foo; }

the program can run without panic with both conditions.

however, this panic

struct Foo { Foo(){} ~Foo() noexcept(true){ try { throw "123"; } catch(int a) { std::cerr << "XX" ; } } }; int main() { Foo foo; }

https://en.cppreference.com/w/cpp/language/noexcept_spec

so I think the answer is, the current code might need to use noexcept(false) because it only capture error_already_set.
if it is catch(...) then we can set it noexcept(true)

yungyuc added the enhancement New feature or request label Nov 10, 2022

yungyuc mentioned this pull request Nov 10, 2022

Turn the "python" widget into a REPL interactive shell #104

Closed

3 tasks

tigercosmos force-pushed the console-stdout branch 5 times, most recently from ee49c58 to 1e65c98 Compare November 10, 2022 17:01

tigercosmos changed the title ~~WIP: Console stdout stderr~~ Console stdout stderr Nov 12, 2022

tigercosmos commented Nov 12, 2022

View reviewed changes

cpp/modmesh/python/common.hpp Outdated Show resolved Hide resolved

tigercosmos commented Nov 12, 2022

View reviewed changes

cpp/modmesh/view/RPythonConsoleDockWidget.cpp Show resolved Hide resolved

tigercosmos commented Nov 12, 2022

View reviewed changes

cpp/modmesh/view/RPythonConsoleDockWidget.cpp Outdated Show resolved Hide resolved

tigercosmos commented Nov 12, 2022

View reviewed changes

modmesh/system.py Outdated Show resolved Hide resolved

tigercosmos commented Nov 12, 2022

View reviewed changes

cpp/modmesh/view/RPythonConsoleDockWidget.hpp Outdated Show resolved Hide resolved

tigercosmos commented Nov 12, 2022

View reviewed changes

modmesh/apputil.py Outdated Show resolved Hide resolved

yungyuc requested changes Nov 13, 2022

View reviewed changes

yungyuc reviewed Nov 13, 2022

View reviewed changes

modmesh/system.py Outdated Show resolved Hide resolved

ezio-melotti reviewed Nov 16, 2022

View reviewed changes

tigercosmos force-pushed the console-stdout branch from af83c05 to a935773 Compare November 21, 2022 07:03

yungyuc mentioned this pull request Nov 21, 2022

Limit the number of anonymous environments in modmesh.apputil #153

Merged

tigercosmos commented Nov 21, 2022

View reviewed changes

ezio-melotti reviewed Nov 21, 2022

View reviewed changes

yungyuc requested changes Nov 21, 2022

View reviewed changes

tigercosmos added 4 commits November 22, 2022 23:56

open cmd for debug mode for windows

9161ec7

stdout & stderr in console

4b7845c

use ">>>" as command beginning

9c8a3cf

redirect exception to stderr

878e6de

tigercosmos commented Nov 22, 2022

View reviewed changes

cpp/modmesh/view/RPythonConsoleDockWidget.cpp Outdated Show resolved Hide resolved

tychuang1211 reviewed Nov 22, 2022

View reviewed changes

cpp/modmesh/view/RPythonConsoleDockWidget.cpp Outdated Show resolved Hide resolved

tigercosmos and others added 10 commits November 23, 2022 13:39

toHtmlEscaped

839fe21

Co-authored-by: tychuang1211 <33109379+tychuang1211@users.noreply.github.com>

toHtmlEscaped

3c02148

Co-authored-by: tychuang1211 <33109379+tychuang1211@users.noreply.github.com>

Merge pull request #1 from tigercosmos/console-stdout

5d2b26a

Console stdout

Revert "open pipe"

f42743f

This reverts commit a9d5f7d.

Revert "try fd, not work"

6b5d092

This reverts commit a637592.

Revert changes in python & Interpreter::exec_code

c3ae031

Use pre-formatted paragraph to show newline

1a42671

Capture python output and redirect it to Qt widget

a5e2255

Merge pull request #1 from tychuang1211/148-console-stdout-stderr

896b803

Redirect Python output to Qt Widget

fix format

3da056a

yungyuc requested changes Nov 24, 2022

View reviewed changes

refactor code

2b9c9f9

Suppress the warning "greater visibility than the type of its field"

e95ded1

exception for PyStdErrOutStreamRedirect

5200df0

catch exception in deconstructor

cb19799

yungyuc requested changes Nov 24, 2022

View reviewed changes

noexcept false

00a53c4

yungyuc approved these changes Nov 24, 2022

View reviewed changes

yungyuc merged commit b485565 into solvcon:master Nov 24, 2022

tigercosmos deleted the console-stdout branch November 24, 2022 13:54

	if None is name or name == '':
	if name is None or name == '':

	sys.stderr.write(("{}: {}".format(type(e).__name__, str(e))))
	sys.stderr.write(f"{type(e).__name__}: {e}")

	m_history_edit->insertHtml(m_stdoutHtml + QString::fromStdString(stdout_message) + m_endHtml);
	m_history_edit->insertHtml(m_stdoutHtml + QString::fromStdString(stdout_message).toHtmlEscaped() + m_endHtml);

Console stdout stderr #148

Console stdout stderr #148

Conversation

tigercosmos commented Nov 9, 2022 • edited by yungyuc Loading

yungyuc commented Nov 9, 2022

yungyuc commented Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezio-melotti Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos commented Nov 22, 2022

tychuang1211 commented Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos commented Nov 24, 2022

tigercosmos commented Nov 24, 2022

yungyuc commented Nov 24, 2022

tychuang1211 commented Nov 24, 2022

yungyuc Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos commented Nov 9, 2022 •

edited by yungyuc

Loading

yungyuc commented Nov 9, 2022 •

edited

Loading

tigercosmos Nov 12, 2022 •

edited

Loading

ezio-melotti Nov 16, 2022 •

edited

Loading

yungyuc Nov 16, 2022 •

edited

Loading

tigercosmos Nov 22, 2022 •

edited

Loading

tychuang1211 commented Nov 22, 2022 •

edited

Loading

yungyuc Nov 24, 2022 •

edited

Loading

tigercosmos Nov 24, 2022 •

edited

Loading