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

Do we remember why utils.io.IOStream exists? #9141

Open
takluyver opened this issue Jan 19, 2016 · 14 comments
Open

Do we remember why utils.io.IOStream exists? #9141

takluyver opened this issue Jan 19, 2016 · 14 comments

Comments

@takluyver
Copy link
Member

IPython.utils.io has stdin, stdout and stderr attributes which wrap the corresponding stream objects from sys in an IOStream instance. Then certain odd places around the codebase, such as oinspect, do print(..., file=io.stdout), while other places don't.

I assume that this was originally supposed to provide an indirection layer so stdout/stderr could easily be redirected. However:

  • Things that redirect stdout/stderr typically replace them directly in sys - they have to, because not all printing is directed through the IPython.utils.io objects. There are backup copies of the original objects as e.g. sys.__stdout__, so replacing sys.stdout is not destructive.
  • When sys.std* is replaced, io.std* still has a reference to the original sys.std* objects, so it doesn't respect the redirection. See issue Support changing sys.std* streams #8669, and I also just ran into this trying to use colorama in Terminal interface based on prompt_toolkit #9118.

I'm proposing that we deprecate io.std*, and move all uses of it in our own code to sys.std*. Does anyone know of a reason not to do this?

Ping @fperez, because I'm pretty sure this code has been around longer than I've been on the project.

@takluyver
Copy link
Member Author

I may have partially answered my own question 2 minutes after asking it. 😑

On Windows, we plug in pyreadline's ANSI-escape convertor at io.std*, but we don't replace sys.std*. So any coloured output, like oinspect and tracebacks, must go through io.std*.

Now, is there a reason why we can't just do sys.stdout = self.readline._outputfile?

@takluyver
Copy link
Member Author

colorama, which I'm using in #9118, installs ansi->win converting streams to sys.stdout/stderr. So we should be able to get rid of these once that work lands, at least.

@Carreau Carreau added this to the 5.0 milestone Jan 19, 2016
@fperez
Copy link
Member

fperez commented Jan 20, 2016

I think you're right, in that we introduced that indirection layer to assist with Windows issues. I honestly don't 100% remember, though: I wrote most of that code, but it was a very long time ago, and I don't recall the specific rationale.

Going through the cobwebs in my brain, though, I'm pretty sure that it gave us hooks we needed to play nice with the windows PyReadline machinery.

It's perfectly conceivable that we could do without all that today, given changes to the surrounding tools. If that's the case, it would be great to get rid of that layer. We should, however, test pretty extensively on Windows before jettisoning it.

@Carreau
Copy link
Member

Carreau commented May 21, 2016

May I propose a crazy idea.
I'm sure you won't like it.

Instead of actually replacing sys.stdout/sys.stderr, we replace sys itself, and mae stdin/stderr properties where we can actually watch for changes and dynamically wrap.

Not even sure it is possible, and not even sure it is a good idea.

@Carreau
Copy link
Member

Carreau commented May 21, 2016

Well, that's definitively a bad idea, but it works.

from types import ModuleType
class Sys(ModuleType):

    def __init__(self, mod):
        setattr(self,'_mod', mod)
        super().__init__(mod.__name__, mod.__doc__)


    def __getattr__(self, name):
        if name in {'stdin','stdout','stderr'}:
            print('So we are getting to that?')
        # you dont' want to recurse on `_mod` do you?
        return object.__getattribute__(object.__getattribute__(self,'_mod'), name)

    def __setattr__(self, name, value):
        if name in {'stdin', 'stdout','stderr'}:
            print('Or so you think...')
            return
        if name == '_mod':
            object.__setattr__(self, name, value)
        setattr(self._mod, name, value)

    def __dir__(self):
        return dir(self._mod)

def patchsys(sys):
    if isinstance(sys, Sys):
        print('Nop, not twice')
        return
    import sys as _sys
    _sys.modules['sys'] = Sys(_sys)

@takluyver
Copy link
Member Author

You're quite right, I do not like it. ;-) The sys module is pretty fundamental to Python, and I don't want to find out what we'd break by doing that. I also don't think it would work reliably - anything that had done import sys before you put that shim in would still have a reference to the real sys module.

Kudos for working out that it's possible, though. For more entertainment, have a look at this code that James Powell showed off at PyData London: https://github.com/dutc/rwatch

@fperez
Copy link
Member

fperez commented May 23, 2016

+1 to not messing around with sys. Neat as the above code may be, I concur with Thomas above. Too dangerous to be worth the trouble.

@Carreau
Copy link
Member

Carreau commented May 23, 2016

+1 to not messing around with sys. Neat as the above code may be, I concur with Thomas above. Too dangerous to be worth the trouble.

Yes, I was aware that was a bad idea.

Sidenote, we have that in globalipapp:

class StreamProxy(io.IOStream):
    """Proxy for sys.stdout/err.  This will request the stream *at call time*
    allowing for nose's Capture plugin's redirection of sys.stdout/err.

    Parameters
    ----------
    name : str
        The name of the stream. This will be requested anew at every call
    """

So it seem that iostream does have a useful usage.

@takluyver
Copy link
Member Author

I think that's more working around our use of IOStream than a reason for it ;-)

@Carreau Carreau modified the milestones: 6.0, 5.0 Jun 2, 2016
@Carreau
Copy link
Member

Carreau commented Jun 2, 2016

IOStream has been deprecated pushing that for 6.0 for potential removal.

@Carreau
Copy link
Member

Carreau commented Feb 9, 2017

@takluyver do you want to have a look at it, 6.0 material or bump to 7+ ?

@takluyver takluyver modified the milestones: 7.0, 6.0 Feb 9, 2017
@takluyver
Copy link
Member Author

Bumped. It's not hard to leave the code there for another release cycle.

@Carreau Carreau removed this from the 7.0 milestone Aug 28, 2018
@fperez
Copy link
Member

fperez commented May 1, 2021

BTW, I suspect this could be done now and all that cleaned up... Whatever we were doing to help Windows ~20y ago isn't needed anymore, I'm nearly certain :)

@Carreau
Copy link
Member

Carreau commented May 1, 2021

Marked as Good First issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants