Fix: SkillCorner loading from non-seekable streams - #599
Open
Litju wants to merge 1 commit into
Open
Conversation
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This is part 1 of the two provider-specific fixes for #469.
This PR fixes SkillCorner loading from direct non-seekable streams by removing duplicate raw-feed inspection and moving version detection into a single parse lifecycle owned by the deserializer.
The public API, generic Kloppy I/O layer, adapters, dependencies, and compression handling remain unchanged.
Problem
SkillCorner supports two raw tracking formats:
dataplayer_dataPreviously, automatic version detection happened in the provider before deserialization. The provider inspected the raw stream to infer V2 or V3, and the deserializer then inspected and parsed the same stream again.
For caller-owned file-like inputs, both stages received the same stream object at its current position. This created multiple failure modes:
seek(0).seek(0).Path-backed inputs did not expose every failure because Kloppy materializes them through its adapter-backed stream path.
Reproduction
The issue was reproduced through the public
skillcorner.load()API using:data_versionRawIOBasestreamsseek()andtell()raiseUnsupportedOperationThe reproduced behavior was:
Design
The raw-data lifecycle now has one owner:
This removes the duplicated inspection and rewind assumptions while keeping the repair local to SkillCorner.
Implementation
Provider
The SkillCorner provider no longer reads the raw feed to detect its version.
It now passes the requested
data_versiondirectly toSkillCornerDeserializer.When no explicit version is supplied, the deserializer resolves the version after parsing the raw records.
The previous provider-level
identify_data_version()helper is removed.Stream normalization
The deserializer introduces a private
_NonClosingRawReaderadapter.The adapter requires only:
read(size) -> bytesfrom the underlying input and exposes the minimal
RawIOBaseinterface required byio.BufferedReader.Every SkillCorner raw input follows the same normalization path:
io.BufferedReader(_NonClosingRawReader(raw_stream))This avoids requiring the underlying stream to implement:
peek()readinto()readable()seek()tell()The temporary adapter and buffered reader are closed after parsing, while the caller-owned raw stream remains open.
Parse-once version resolution
The deserializer now:
peek(1)at the normalized parser boundary to distinguish JSON arrays from JSONLdataplayer_datadata_versionas authoritativeThe implementation introduces no prefix replay and no complete secondary payload copy.
Tests
The permanent SkillCorner tests now cover:
The non-seekable test stream explicitly rejects
seek()andtell(), and the tests verify that the number of bytes read from the underlying stream equals the payload size.Validation
The SkillCorner module and complete repository suite were run with all extras on every supported Python version:
427 passed, 1 xfailed427 passed, 1 xfailed427 passed, 1 xfailed427 passed, 1 xfailed427 passed, 1 xfailedAcross all five versions:
000Additional validation:
git diff --checkpassedValidation used the repository's CI-equivalent environment:
uv sync --frozen --all-extras --python <VERSION>uv run --python <VERSION> pytest kloppy/tests/test_skillcorner.pyuv run --python <VERSION> python -m pytest kloppy/testsThis was repeated for Python 3.9 through 3.13.
Scope
This PR addresses only the SkillCorner half of #469.
It intentionally does not modify:
BufferedStreamThe Wyscout half will be addressed in a separate PR. Its automatic detection currently parses the complete document to choose a deserializer and then attempts to parse the same direct stream again, so it requires a distinct parse-once repair.
Related to #469.