Skip to content

Fix: SkillCorner loading from non-seekable streams - #599

Open
Litju wants to merge 1 commit into
PySport:masterfrom
Litju:fix/skillcorner-nonseekable-streams
Open

Fix: SkillCorner loading from non-seekable streams#599
Litju wants to merge 1 commit into
PySport:masterfrom
Litju:fix/skillcorner-nonseekable-streams

Conversation

@Litju

@Litju Litju commented Aug 1, 2026

Copy link
Copy Markdown

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:

  • V2: JSON array with frame data under data
  • V3: JSONL with frame data under player_data

Previously, 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:

  • Non-seekable automatic loading failed when the provider attempted seek(0).
  • Non-seekable explicit loading still failed when the deserializer attempted seek(0).
  • Seekable V2 automatic loading failed because provider-level detection consumed the complete JSON array before the deserializer ran.
  • Valid one-line V3 automatic loading failed because provider-level detection consumed the only JSONL record.

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:

  • V2 JSON-array payloads
  • V3 JSONL payloads
  • automatic and explicit data_version
  • seekable direct streams
  • true non-seekable RawIOBase streams
  • non-seekable streams whose seek() and tell() raise UnsupportedOperation
  • a valid one-line V3 JSONL feed
  • existing path-backed tests as compatibility controls

The reproduced behavior was:

  • Seekable V2 automatic: failed after the provider consumed the complete array
  • Seekable V2 explicit: passed
  • Seekable one-line V3 automatic: failed after the provider consumed the only record
  • Seekable one-line V3 explicit: passed
  • Non-seekable V2 automatic: failed during provider rewind
  • Non-seekable V2 explicit: failed during deserializer rewind
  • Non-seekable V3 automatic: failed during provider rewind
  • Non-seekable V3 explicit: failed during deserializer rewind

Design

The raw-data lifecycle now has one owner:

  1. Open the raw input.
  2. Normalize it to a buffered read interface.
  3. Inspect the first logical byte.
  4. Parse the feed exactly once.
  5. Infer V2 or V3 when automatic detection is required.
  6. Deserialize the already parsed records.

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_version directly to SkillCornerDeserializer.

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 _NonClosingRawReader adapter.

The adapter requires only:

read(size) -> bytes

from the underlying input and exposes the minimal RawIOBase interface required by io.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:

  • uses peek(1) at the normalized parser boundary to distinguish JSON arrays from JSONL
  • parses the raw feed exactly once
  • infers V2 when the first parsed record contains data
  • infers V3 when the first parsed record contains player_data
  • preserves an explicitly supplied data_version as authoritative
  • resolves the frame parser and frame-data key once for the rest of deserialization

The implementation introduces no prefix replay and no complete secondary payload copy.

Tests

The permanent SkillCorner tests now cover:

  • V2 and V3
  • automatic and explicit version selection
  • seekable direct streams
  • non-seekable direct streams
  • valid one-line V3 JSONL input
  • existing path-backed loading
  • automatic and explicit semantic equivalence
  • exact single traversal of non-seekable payloads
  • caller-owned raw and metadata streams remaining open
  • temporary wrapper cleanup on successful loads
  • temporary wrapper cleanup on parsing failures
  • malformed JSON arrays
  • malformed JSONL
  • unknown raw formats
  • exact preserved exception messages

The non-seekable test stream explicitly rejects seek() and tell(), 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:

  • Python 3.9: SkillCorner passed; full suite 427 passed, 1 xfailed
  • Python 3.10: SkillCorner passed; full suite 427 passed, 1 xfailed
  • Python 3.11: SkillCorner passed; full suite 427 passed, 1 xfailed
  • Python 3.12: SkillCorner passed; full suite 427 passed, 1 xfailed
  • Python 3.13: SkillCorner passed; full suite 427 passed, 1 xfailed

Across all five versions:

  • failures: 0
  • errors: 0
  • missing extras: 0

Additional validation:

  • repository-wide pre-commit passed
  • pre-commit did not modify the candidate
  • lockfile consistency passed
  • git diff --check passed
  • caller-owned streams remained open on success and failure
  • temporary wrappers closed on success and failure
  • each non-seekable payload was traversed once
  • automatic and explicit V2/V3 outputs were semantically equivalent
  • no material runtime regression was observed
  • no material memory regression was observed

Validation used the repository's CI-equivalent environment:

uv sync --frozen --all-extras --python <VERSION>

uv run --python <VERSION> pytest kloppy/tests/test_skillcorner.py

uv run --python <VERSION> python -m pytest kloppy/tests

This was repeated for Python 3.9 through 3.13.

Scope

This PR addresses only the SkillCorner half of #469.

It intentionally does not modify:

  • generic Kloppy I/O
  • BufferedStream
  • adapters
  • dependencies
  • lockfiles
  • CI configuration
  • compression handling
  • the public loading API

The 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.

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.

1 participant