Allow buffered reading for non-seekable streams

[here-abarany]

The current implementation will only allow buffered reading when streams are seekable. This means use-cases such as streaming large JSON responses from an HTTP request will be extremely slow as it reads one byte at a time due to the Python overhead. In this case, even the native Python tokenizer is faster than the native Rust implementation.

The correct_cursor parameter is set to true by default and not settable through the Python API. As a result, it will always use unbuffered input, even when explicitly setting the hidden buffering argument, when the stream isn't seekable. In this case the only option is to create a custom RawIOBase implementation that implements some minimal amount of seeking to allow it to create a buffered reader. With one test case, adding seeking functions resulted in ~8x speedup. When using a debugger, neither seek() nor tell() were called.

[smheidrich]

I agree and have been planning to change the default to correct_cursor=False for that reason, cf.

• Consider making non-correct cursor the default for non-seekable streams again #116

The only thing I'm worried about is users who have come to rely on the default behavior now and expect cursor positions to be correct; changing it now could lead to some very difficult to diagnose situations.

The correct_cursor parameter is set to true by default and not settable through the Python API.

It is settable, cf. the 2nd section of daggaz/json-stream#59 (comment). But it's not documented, typechecked, or anything, because the original plan was for json-stream proper to make use of it, not for users to set it manually on the tokenizer itself.

Regarding how to proceed:

Perhaps a good strategy would be to release a version which emits a warning when correct_cursor is not set, saying that the default behavior will change in a new version planned to be released 6 months from now. That should give people time to adjust.

But how would people silence the warning? I definitely don't want them to be go ahead and set correct_cursor to silence it, because it's not even a part of the public API and I don't know if it will ever become one.

So maybe there would just be advice to suppress the warning using Python's built-in warnings filter?

[here-abarany]

It is settable, cf. the 2nd section of daggaz/json-stream#59 (comment).

I see, this is the section of code that made me think it wasn't possible:

    supported_kwargs = {"buffering"}
    unsupported = kwargs.keys() - supported_kwargs
    if unsupported:
        raise RequestedFeatureUnavailable(
            "some requested features are unknown in this version of "
            f"json-stream-rs-tokenizer: {unsupported}"
        )

As well as this comment in Rust:

///   correct_cursor: *(not part of API yet, may be removed at any point)*

I didn't realize it needed to go through a partial function to actually be set.

Regarding how to proceed:

I'm fine with your overall plan, but I feel that there needs to be a better way to set these options. Currently there's two main issues:

1. It's rather inconvenient on top of being undocumented to have to wrap the tokenizer in a partial function.
2. This removes the automatic fallback mechanism to use the pure Python tokenizer if the Rust backend isn't available.

I can think of a couple options to improve this:

• Allow extra options to be passed to json_stream.load() that get forwarded to the tokenizer.
• Have a wrapper to provide a tokenizer with options provided. For example, wrapping the partial but with documented and checked arguments, or making the tokenizer parameter be an object rather than a function so it can hold its own options directly.

In both cases, I think the fallback tokenizer should take the same options, ignoring them if it always does unbuffered reading, and it should be possible to use the default tokenizer type but with custom options. This would make it much easier to say "this is the ideal way to have the optimized parsing", but have it still work, albeit slowly, if the Rust backend isn't available.

[here-abarany]

Perhaps one way to mitigate the amount of warning spam is to only warn if it adjusts the cursor at the end? Best I can tell it doesn't even call seek() if it reached the end of the stream, so this change of behavior is effectively moot for the 99.9% of cases where the full contents of the stream is a single JSON document. It would probably take a little extra plumbing to check if it was set (e.g. maybe a wrapper that passes to the Rust backend whether correct_cursor was explicitly set), but I think the payoff is worth it to avoid potentially a large amount of spam for the vast majority of cases.

[smheidrich]

but I feel that there needs to be a better way to set these options

The best way would be if json-stream had explicit support for correct_cursor, which it would need anyway to actually make use of it for seekable streams (because for those it's not just a matter of setting the argument to RustTokenizer's constructor, but of also calling a special method whenever a JSON document ends - cf. daggaz/json-stream#31), and if it let users turn this functionality on/off via a correct_cursor parameter of its own. To me, that would be much preferable than making it easier for people to configure the tokenizer directly, which should ideally never be necessary (or only in really exotic situations, which this one shouldn't be).

But I think I've only started seeing it that way after thinking about this now - if you read my old comments / MR descriptions in both daggaz/json-stream#31 and #47, it seems like I used to be in favor of the correct-cursor functionality being always activated, and requiring weird hacks (like "intercepting" the tokenizer instantiation, as is currently necessary) to turn off. I don't know why I thought that, it's clearly the wrong choice. Maybe I thought unseekable streams were rare or something, I don't know.

But anyway, even if this were implemented in json-stream, I still think it makes sense to change the default back to non-correct cursors for unseekable streams (and thereby much better performance).

So having thought about it again, I'm currently leaning towards not doing the warning thing at all and just going with a "rip the bandaid off" approach: Release a new pseudo-major version (0.4.X → 0.5.0) to indicate a breaking change and just change the default. Unfortunately, json-stream's version specifier for its json-stream-rs-tokenizer dependency doesn't come with an upper bound, so people whose packages only depend on json-stream-rs-tokenizer transitively via json-stream probably won't see that this is a pseudo-major version increase in one of their dependencies and hence potentially breaking... But still, it seems like the most sensible, not-overly-complicated thing to do.

[daggaz]

I'm up for this.

[daggaz]

So, I've looked into this a bit more, and have some thoughts. They might be stupid thoughts.

The original reason we introduced correct_cursor, if I recall, was situations where it was possible to "over-consume" the stream beyond the end of the current JSON document.

The particular usecase was people with concatenated JSON docs, or NDJSON. In both those cases, the "next" consumer of the stream is json-stream itself, so maybe we can do better at supporting that, mitigating the downsides of defaulting correct_cursor to False.

In other words, move the problem up to a higher level instead of relying on stream-level operations.

I think we should 100% keep correct_cursor (and expose advanced tokeniser options at the json-stream API level) for those users where my assumption about the "next" consumer doesn't hold true.

My initial thought is to extend the tokenize() protocol to add either support for:

• returning the "remainder" that has been read from the stream (and therefore something to wrap up that remainder with the live stream on the next iteration
• returning some sort of wrapper stream that contains both the remainder and the real stream (kinda like we'd itertools.chain()ed the stream)

For the python tokenizer, this would be simply:

class tokenize:
    def __init__(self, stream):
        self._gen = _tokenize_generator(stream) # original tokenize()
        self.remainder = "" # python tokenizer can never read beyond the end of the current JSON document

    def __iter__(self):
        return self

    def __next__(self):
        return next(self._gen)

We could even add a top-level option json_stream.multiple(...) that hides this complexity and yeilds mutliple docs, but that's a different conversations probably.

[daggaz]

I've now realised why this is stupid (the tokeniser itself doesn't maintain state of the overall document...)

[daggaz]

Ah! But, we could just expose the internal buffer state from the tokenizer as a remainder property that the higher-level can use when its done with this tokeniser instance?

[smheidrich]

The particular usecase was people with concatenated JSON docs, or NDJSON. In both those cases, the "next" consumer of the stream is json-stream itself
We could even add a top-level option json_stream.multiple(...) that hides this complexity and yeilds mutliple docs, but that's a different conversations probably.

I thought about exactly that today as well and was going to write: That's exactly what I had forgotten / gotten wrong when I wrote #47 (comment) in response to your idea in #47 (comment) of having something like load_many (or a many param to load, whatever) in json-stream. load_many would maintain the same tokenizer instance between individual documents in the stream, and so it would be fine for the tokenizer to leave the cursor in the wrong place in between documents. So I really take back everything in #47 (comment), I think load_many or something like that for processing "ndjson"-formatted streams specifically might actually be great, because it would cover the most common use case and not require having correct cursor positions (which, as we've seen, are just not possible to do efficiently for unseekable streams) at all.

I don't even think the remainder thing would be needed for this, would it? You just continue using the same tokenizer instance. Any whitespace in between ndjson documents consists of valid JSON whitespace tokens, which the tokenizer already just discards.

In case you do want to experiment with the remainder thing though, that has already been implemented in json-stream-rs-tokenizer for a while (accessible as the RustTokenizer.remainder property):

py-json-stream-rs-tokenizer/src/lib.rs

Lines 278 to 292 in 2e794f4

	/// Bytes/string data that have been buffered but not yet processed.
	///
	/// The type (bytes or str) depends on the type of the data returned by
	/// the underlying Python file-like object / stream.
	///
	/// This is provided as an alternative to park_cursor for unseekable yet
	/// buffered (for performance) streams. In such cases, the cursor will be
	/// in a "wrong" position (namely at the end of the block read ahead into
	/// the buffer) even after park_cursor() has been called, so this feature
	/// allows users to write their own workarounds by obtaining the
	/// read-ahead data.
	#[getter]
	fn remainder(slf: PyRefMut<'_, Self>) -> StreamData {
	slf.stream.remainder()
	}