[bugfix] ODPS reader: pin pyodps==0.12.5.1 (retry connection resets) + log session/table context on errors

[tiankongdeguiji]

Problem

MaxCompute (ODPS) training jobs intermittently crash with a transient network drop during a storage-api read:

requests.exceptions.ConnectionError:
  ('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer'))

The exception escapes the DataLoader worker and tears down the whole distributed job, leaving only a raw stack trace — no session id, table, or row range to tell which read failed.

Root cause

pyodps stopped retrying connection resets in 0.12.6. Up to 0.12.5.1, the storage-api HTTP adapter mounted a urllib3 Retry(total=retry_times, allowed_methods={GET,HEAD,DELETE}, status_forcelist=[502,503,504]). A connection reset surfaces inside urllib3 as a ProtocolError, which Retry classifies as a retryable read error for idempotent methods — so it was retried up to retry_times times. 0.12.6 (and 0.13.0) dropped that adapter-level retry and moved retry into a Python filter that only retries ODPSError with status 502/503/504; a requests.ConnectionError is neither, so it is re-raised on the first attempt. Upgrading past 0.12.5.1 silently removed connection-reset resilience.

On the tzrec side, tzrec/datasets/odps_dataset.py also had two gaps: _read_rows_arrow_with_retry only caught ODPSError (and retried silently), and _get_session_record_count had no error handling at all — so an escaped reset died without any context.

Change

• Pin pyodps==0.12.5.1 (requirements/runtime.txt) to restore the transport-level urllib3 retry of connection resets.
• Log storage-api context before re-raising (diagnostics only — no new tzrec-side retry): _storage_debug_info formats session id / row range / table / quota / endpoint, and both call sites log it plus the exception repr (which carries the ODPS RequestId when present). The existing ODPSError retry path is made non-silent — warning per attempt, error on exhaustion.
• Log client.table.full_table_name, not the Table object — str(Table) dumps the entire column schema (hundreds of lines for wide feature tables) and touches lazily-loaded fields, which could fire a metadata reload inside the except block exactly when the connection is already broken.

Test

Ungated OdpsStorageErrorLogTest (no credentials/network) drives a stub client that raises ConnectionError and asserts both functions re-raise and log the session id / table:

python -m tzrec.datasets.odps_dataset_test OdpsStorageErrorLogTest

🤖 Generated with Claude Code

[github-actions]

debug_info is built eagerly on every call, before the try (lines 227 & 263) — so this runs on the success path too, not just on errors. It dereferences private pyodps attrs (_quota_name, _rest_endpoint). If a future pyodps bump renames one — and this PR exists precisely because of pyodps version sensitivity — _storage_debug_info itself raises AttributeError and breaks every read with an error unrelated to the actual failure, instead of producing the diagnostics it's meant to provide.

Consider making it defensive (fall back to a placeholder per field) and/or building the string lazily only on the error branch. OdpsReader already holds the quota/endpoint as public state, which avoids the private-attr coupling for the read path.

[github-actions]

The new tests raise requests.exceptions.ConnectionError, which is not an ODPSError, so both tests only exercise the generic except Exception branch (odps_dataset.py:251-253). The ODPSError retry loop — per-attempt logger.warning, the time.sleep backoff, and the "after N retries" logger.error on exhaustion (lines 237-250) — has no coverage. A refactor that broke the retry loop entirely would still pass both tests.

Consider adding a stub whose read_rows_arrow raises ODPSError N times then returns a sentinel reader (patch time.sleep to keep it fast): assert success-after-retry (warning count, no error) and exhaustion-after-3-retries (error text contains the retry count + session id).

[github-actions]

Code review summary

Focused, well-scoped bugfix with an excellent PR description — the root-cause analysis (pyodps 0.12.6 dropping the urllib3-level connection-reset retry) and the deliberate full_table_name-over-str(Table) choice are clearly reasoned and correct. The new test is appropriately ungated (no creds/network), and the diagnostics genuinely improve on-call debuggability.

Two items worth addressing (inline):

• _storage_debug_info is fragile + eager — built on every call before the try (success path included) and reads private pyodps attrs (_quota_name/_rest_endpoint). A future pyodps rename would make the diagnostic helper itself crash every read. Suggest defensive per-field fallback and/or lazy construction on the error branch.
• Retry path untested — ConnectionError isn't an ODPSError, so the tests only hit the except Exception branch; the ODPSError retry loop (warning-per-attempt, backoff, exhaustion error) has no coverage.

Minor / optional (not blocking):

• The except Exception branch re-raises connection errors with no tzrec-side retry — intentional per the PR (transport-layer retry is restored via the pin), but a one-line comment stating that would help future readers.
• {e!r} is logged at several sites; safe today (endpoint is a host, not a signed URL; creds are never passed in), but logging exception repr is a recurring leak vector — str(e) or a small redaction pass would future-proof it.

Performance and documentation review: clean — .full_table_name correctly avoids the lazy metadata reload, debug_info is per-interval/session (not per-batch), and the docstring/comment are accurate and consistent with the module's conventions.

[github-actions]

Minor / defensive: this helper runs only on the error path, but client.table.full_table_name is the one attribute accessed without a guard, while quota/endpoint below use getattr(..., None). If client.table ever raises (e.g. a partially-constructed client, or an unexpected AttributeError), the diagnostic log this PR exists to produce won't be emitted, and the original ConnectionError/ODPSError gets buried behind a chained secondary exception.

(I checked pyodps: full_table_name resolves via _get_schema_name() + already-set project.name/name, so it does not trigger a network reload() — so this is purely robustness, not the I/O risk the PR description hints at.)

Consider making the whole helper best-effort so logging can never raise:

try:
    table = client.table.full_table_name
except Exception:
    table = None

[github-actions]

Coverage gap: _get_session_record_count's happy path is untested. This test only exercises the exception branch; the SessionStatus.INIT wait loop (time.sleep(1) → retry) and the final return scan_resp.record_count have no coverage, so a regression in the status comparison or the return value wouldn't be caught.

Consider a stub whose get_read_session returns INIT once then a ready status with a record_count, with time.sleep mocked, asserting the returned count and that it polled once. (Minor, also: none of the tests assert quota=/endpoint= actually appear in the logged string — worth one assertIn so a future pyodps rename of those private attrs doesn't silently log None while tests stay green.)

[github-actions]

Code review summary

Solid, well-scoped bugfix. The root-cause analysis (pyodps dropping urllib3-level connection-reset retry in 0.12.6) is convincing, the ==0.12.5.1 pin is the right call given the regression, and the diagnostic logging adds genuine value for debugging transient MaxCompute failures. The choice of full_table_name over str(Table) to avoid a lazy schema reload is correct, and the new tests cover the three meaningful paths (re-raise non-ODPSError, retry-then-succeed, retry-exhaustion). Nice diagnostics design (session_id/row_index per line aids disambiguation across distributed workers).

Reviewed across code quality, performance, tests, docs, and security. Nothing blocking. Two minor items posted inline:

• _storage_debug_info robustness — client.table.full_table_name is the one unguarded access in an error-only helper; making it best-effort (like the getattr guards on quota/endpoint) ensures the diagnostic log never raises a secondary exception. Defensive only — verified it doesn't trigger network I/O.
• Test coverage — the happy path of _get_session_record_count (the INIT wait loop + record_count return) is untested; and no test asserts quota=/endpoint= actually appear in the logged string.

Cleared with no concerns: performance (all new logic is on per-session/error paths, the per-batch reader.read() hot path is untouched), security (logged fields — table/quota/endpoint/RequestId — carry no credentials; helpers are stateless and worker-safe), and docs (no conflicting pyodps version references; version bump and docstrings follow convention).