Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enable writing to python stdio streams #3920
base: main
Are you sure you want to change the base?
Enable writing to python stdio streams #3920
Changes from 8 commits
d360b44
b0217de
19bb492
421fb03
de7d4de
7e0d188
7a8f468
0d5e21d
5ad567b
6fa04aa
41d600a
e90f220
f328a20
e2b8e53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the Rust-side line buffering here? From the Python documentation
I would infer that the line buffering is happening inside the
sys.stdout/err
Python objects (if it is desired/enabled) to whichPySys_WriteStdout/err
eventually defer. Meaning that we actually should not make a buffering decision here and also defer to whatever these Python objects decide for the buffer strategy.Finally, this also makes me wonder whether we should go through the formatting machinery of
PySys_WriteStdout/err
at all instead of callingwrite
onpystream
same as we callflush
. The code athttps://github.com/python/cpython/blob/5dc8c84d397110f9edfa56793ad8887b1f176d79/Python/sysmodule.c#L3895
at least does not seem to do anything more special than what we could do directly if
intern!
is used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we could avoid the cost of the runtime formatting machinery entirely, just passing the byte slice directly to write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented it with the LineWrite wrapper because when I tested it I seemed to be getting output immediately from every call to
write!(stream,...)
from within Rust, rather then getting full lines, i.e. seemingly no buffering was happening. I'm happy to remove it though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that this is coming so piece-meal, but maybe we just want a public wrapper type that will adapt any
Py<PyAny>
as aWrite
impl by calling itswrite
andflush
methods, e.g.(I am also wondering whether we should have a variant storing
Bound<'py, PyAny>
to avoid repeatedly callingwith_gil
in theWrite
impl. It should be easy enough to convert betweenPyWrite
andPyWriteBound
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this depends on under what conditions you tested it, but in any case, the decision for buffering or not just be with the stream stored at
sys.stdout/err
, not with our wrapper of it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're happy for me to do so, I could add a
buffered
method to create a LineWriter wrapper as before, i.e.Or, alternatively, an
unbuffered
that works the other way around. Fine if not though, since it's easy enough to implement such a feature myself as a user.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can certainly add a convenience method to add optional buffering. I would suggest calling it
line_buffered
though as it is a relatively specific buffering strategy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to think about this but I am not really convinced as
sys.stdout
is not the same the Unix stdout on which Rust'sstd::io::stdout
module is operatring, it is defined as a text stream and not a byte stream. I think if you want to expose an impl ofstd::io::Write
, you should usesys.stdout.buffer
as the backing stream and notsys.stdout
. But note that this will not always be available. (Silently corrupting the written data usingfrom_utf8_lossy
is not option IMHO as it is just too surprising if e.g.std::io::copy
produces garbage ZIP archives for that reason.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a solution would be to implement
new
forPyWriterBound
, and to test at the point of object creation whether the innerPyAny
is an instance of eitherio.BufferedIOBase
orio.TextIOBase
. Subsequent write behaviour could then be configured accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what would that be? I don't think we can support
io::Write
usingio.TextIOBase
. (The other way around works, i.e.fmt::Write
can write intoio.BufferedIOBase
by callingas_bytes
, but it would need to do that explicitly to avoid errors on the Python side.)So personally, I think we should
io::Write
andfmt::Write
, or since you can here with a specific use case in mind, at least forfmt::Write
.io::stdout
(accessingsys.stdout.buffer
) andfmt::stdout
(accessingsys.stdout
) which only succeed if the correct Python writer is present.