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

Introduce GAP-Level StandardInput, StandardOutput, StandardError Objects #1815

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

markuspf
Copy link
Member

@markuspf markuspf commented Oct 26, 2017

This introduces the InputStream StandardInput and the OutputStreams
StandardOutput and StandardError at the GAP level, and attaches

  • InputTextFile("*stdin*") to StandardInput,
  • OutputTextFile("*stdout*", true) to StandardOutput
  • OutputTextFile("*errout*", true) to StandardError

This small change already enables refactoring error.g to PrintTo
the StandardError stream, which can be replaced by an
OutputTextString (or a different output stream) to capture error messages.

One obvious use case of this is to capture output for alternative
REPL implementations such as Jupyter.

This introduces the InputStream StandardInput and the OutputStreams
StandardOutput and StandardError at the GAP level, and attaches
 * InputTextFile("*stdin*") to StandardInput,
 * OutputTextFile("*stdout*", true) to StandardOutput
 * OutputTextFile("*errout*", true) to StandardError

This small change already enables refactoring error.g to PrintTo
the StandardError stream, and it can be replaced by an
OutputTextString to capture error messages.

One obvious use case of this is to capture output for alternative
REPL implementations such as Jupyter.
Obj StandardInput = 0;
Obj StandardOutput = 0;
Obj StandardError = 0;

Copy link
Member Author

Choose a reason for hiding this comment

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

These are currently unused, I seem to remeber imagining having to access these streams from the kernel level.

Copy link
Member

Choose a reason for hiding this comment

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

So, do you mean you want to keep them "in case", or that you will update this PR to drop them?!?

If you intend to keep them, please add at least a tiny comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

The best outcome would be to not need them at the C level at all, so maybe I remove them right now, and only add them if there's a really good reason to have them.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

So, I have no actual concerns about this PR, but am a bit mystified by the motivation for it.

You say that it enables refactoring Error to send its output to a stream, by redeeming StandardError, which then is useful for Jupyter. But I would have thought that things like Jupyter have to catch GAP's stdout and stderr anyway? Am I then wrong in that believe? And if so, does that mean it is OK for you to capture only output sent to StandardError, but not any remaining output sent to *errout*?

On a different point, note that hpcgap/lib/error.g exists; this PR increases the diffs between the two copies of error.g again.

I also wonder if these new objects work corrctly in HPC-GAP.

@@ -1797,6 +1797,10 @@ InstallGlobalFunction( "UnInstallCharReadHookFunc",
Unbind(OnCharReadHookActive);


InstallValue(StandardInput, InputTextFile("*stdin*"));
InstallValue(StandardOutput, OutputTextFile("*stdout*", true));
Copy link
Member

Choose a reason for hiding this comment

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

Note that colorprompt.g also declares STDOut as BindGlobal("STDOut", OutputTextUser());, which also binds it to stdout; so perhaps change that to use StandardOutput, too?

Also, Exec in process.gi uses stdin and stdout.

Obj StandardInput = 0;
Obj StandardOutput = 0;
Obj StandardError = 0;

Copy link
Member

Choose a reason for hiding this comment

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

So, do you mean you want to keep them "in case", or that you will update this PR to drop them?!?

If you intend to keep them, please add at least a tiny comment.

@markuspf
Copy link
Member Author

My main motivation is to have a well-defined GAP level interface for output and input that can just be re-bound to well-defined to GAP level OutputStreams and InputStreams, and in the end to get rid of the file-and-stream-juggling exercises in scanner.c (which will, hopefully, also enable clean implementation of a replacement to READ_ALL_COMMANDS in the end!).

At the moment its fine with me for Jupyter if someone uses PrintTo or AppendTo on *errout* or *stdout*, it just means that this output will not be visible in a Jupyter front-end (hence I would consider it a bug-to-be-fixed, but there are still so many things "wrong" with Jupyter output that this is merely noise).

On the matter of HPC-GAP: while this change will not immediately work with it, the multi-threaded console-ui does something very similar to the approach used here: it defines a DEFAULT_INPUT_STREAM and a DEFAULT_OUTPUT_STREAM for each thread.

Once we managed to make input and output go through a well-defined GAP level object, the console ui of HPC-GAP can also be simplified.

@fingolfin
Copy link
Member

What I don't understand is why we need to define new global objects for this, and then change all code to print to them; note that nothing would stop new code from again printing to e.g. *errout*.

Wouldn't it be less invasive to do the reverse, and change the code which accepts *errout* etc. as input to use that as a special hint to print to respective streams?

@markuspf markuspf added this to the GAP 4.10.0 milestone Nov 3, 2017
@markuspf
Copy link
Member Author

markuspf commented Nov 6, 2017

We can add functionality to re-route stuff that uses the "magic strings" through these streams.

Since I would quite like to clean things up a bit (and that would include chasing up the definition of STDOut in colorprompt.g, so clearly there has been a desire to have the output stream around in the past, and adding documentation!), adding an indirection is of course complicating the input/output path.

Another usecase I imagine for this PR is getting rid of all the specific LogTo streams, since we can implement all the logging through LogStreams that are implemented on the GAP level.

@frankluebeck
Copy link
Member

I'm wondering if some abstraction layer would be useful here. So, e.g., StandardOutput is not directly connected to the stdout stream. Instead there is some data structure that contains the information about one or several streams where this output should be sent to (and the set of these output streams can be changed at runtime, e.g., adding or removing a log file at runtime). So, printing to the StandardOutput is done to distinguish this output from output for StandardError (and maybe we want to distinguish further cases).

ssiccha added a commit to ssiccha/JupyterKernel that referenced this pull request Jul 6, 2018
Uses the new command line option --alwaystrace. This lets GAP
also print tracebacks when the break loop is disabled by -T.

This still overwrites the library functions Where, WHERE, and
ErrorInner. This can be avoided once there is a central GAP
error stream where these functions send their output, see
gap-system/gap#1815.

Fixes gap-packages#38.
ssiccha added a commit to ssiccha/JupyterKernel that referenced this pull request Jul 7, 2018
Uses the new command line option --alwaystrace. This lets GAP
also print tracebacks when the break loop is disabled by -T.

This still overwrites the library functions Where, WHERE, and
ErrorInner. This can be avoided once there is a central GAP
error stream where these functions send their output, see
gap-system/gap#1815.

For versions older than 4.10 a backwards compatible version of
gap/JupyterError.gi is loaded.

Fixes gap-packages#38.
ssiccha added a commit to ssiccha/JupyterKernel that referenced this pull request Jul 10, 2018
Uses the new command line option --alwaystrace. This lets GAP
also print tracebacks when the break loop is disabled by -T.

This still overwrites the library functions Where, WHERE, and
ErrorInner. This can be avoided once there is a central GAP
error stream where these functions send their output, see
gap-system/gap#1815.

For versions older than 4.10 a backwards compatible version of
gap/JupyterError.gi is loaded.

Fixes gap-packages#38.
ssiccha added a commit to ssiccha/JupyterKernel that referenced this pull request Sep 17, 2018
Uses the new command line option --alwaystrace. This lets GAP
also print tracebacks when the break loop is disabled by -T.

This still overwrites the library functions Where, WHERE, and
ErrorInner. This can be avoided once there is a central GAP
error stream where these functions send their output, see
gap-system/gap#1815.

Since redirecting error messages was not available in 4.9
this also sets the required GAP version to >= 4.10.

Fixes gap-packages#38.
ssiccha added a commit to ssiccha/JupyterKernel that referenced this pull request Sep 17, 2018
Uses the new command line option --alwaystrace. This lets GAP
also print tracebacks when the break loop is disabled by -T.

This still overwrites the library functions Where, WHERE, and
ErrorInner. This can be avoided once there is a central GAP
error stream where these functions send their output, see
gap-system/gap#1815.

Since redirecting error messages was not available in 4.9
this also sets the required GAP version to >= 4.10.

Fixes gap-packages#38.
markuspf pushed a commit to gap-packages/JupyterKernel that referenced this pull request Sep 18, 2018
Uses the new command line option --alwaystrace. This lets GAP
also print tracebacks when the break loop is disabled by -T.

This still overwrites the library functions Where, WHERE, and
ErrorInner. This can be avoided once there is a central GAP
error stream where these functions send their output, see
gap-system/gap#1815.

Since redirecting error messages was not available in 4.9
this also sets the required GAP version to >= 4.10.

Fixes #38.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 18, 2018
This commit introduces new global variables StandardInput,
StandardOutput, ErrorOutput.

ADD_SET is used since the new global variables are
initialized relatively early in the start-up process,
AddSet is not yet available at this point.

The lib/colorprompt.g was changed to use StandardOutput
instead of defining its own STDOut.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 18, 2018
This commit introduces new global variables StandardInput,
StandardOutput, ErrorOutput.

ADD_SET is used since the new global variables are
initialized relatively early in the start-up process,
AddSet is not yet available at this point.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 18, 2018
This commit introduces a new global variable ErrorOutput.

ADD_SET is used since the new global variables are
initialized relatively early in the start-up process,
AddSet is not yet available at this point.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 18, 2018
This commit introduces a new global variable ErrorOutput.

ADD_SET is used since the new global variables are
initialized relatively early in the start-up process,
AddSet is not yet available at this point.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 18, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 18, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 20, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 20, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 20, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 20, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 21, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
ssiccha added a commit to ssiccha/gap that referenced this pull request Sep 21, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
fingolfin pushed a commit that referenced this pull request Sep 21, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in #1815. See #2822.
markuspf pushed a commit that referenced this pull request Sep 22, 2018
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in #1815. See #2822.
@fingolfin fingolfin removed this from the GAP 4.10.0 milestone Sep 28, 2018
ssiccha added a commit to ssiccha/gap that referenced this pull request Mar 27, 2019
This commit introduces a new global variable ERROR_OUTPUT.

As a next step one can make it possible to change the targets
of the magic strings "*stdout*", "*stdin*", and "*errout*",
as suggested by @fingolfin in gap-system#1815. See gap-system#2822.
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