Skip to content
Merged
2 changes: 1 addition & 1 deletion requirements/runtime.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ psutil
pyfg @ https://tzrec.oss-accelerate.aliyuncs.com/third_party/pyfg-1.0.5-cp312-cp312-linux_x86_64.whl ; python_version=="3.12"
pyfg @ https://tzrec.oss-accelerate.aliyuncs.com/third_party/pyfg-1.0.5-cp311-cp311-linux_x86_64.whl ; python_version=="3.11"
pyfg @ https://tzrec.oss-accelerate.aliyuncs.com/third_party/pyfg-1.0.5-cp310-cp310-linux_x86_64.whl ; python_version=="3.10"
pyodps>=0.12.4
pyodps==0.12.5.1
safetensors
scikit-learn
tensorboard
Expand Down
38 changes: 37 additions & 1 deletion tzrec/datasets/odps_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,21 +206,52 @@ def _parse_table_path(
return str_list[2], table_name, table_partitions, schema


def _storage_debug_info(client: StorageApiArrowClient, **req_fields: Any) -> str:
"""Format MaxCompute storage-api context for error logging."""
parts = {
"table": client.table.full_table_name,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"quota": getattr(client, "_quota_name", None),
"endpoint": getattr(client, "_rest_endpoint", None),
**req_fields,
}
return ", ".join(f"{k}={v}" for k, v in parts.items())


def _read_rows_arrow_with_retry(
client: StorageApiArrowClient,
read_req: ReadRowsRequest,
) -> ArrowReader:
def debug_info() -> str:
return _storage_debug_info(
client,
session_id=read_req.session_id,
row_index=read_req.row_index,
row_count=read_req.row_count,
max_batch_rows=read_req.max_batch_rows,
)

max_retry_count = 3
retry_cnt = 0
while True:
try:
reader = client.read_rows_arrow(read_req)
except ODPSError as e:
if retry_cnt >= max_retry_count:
logger.error(
f"read_rows_arrow failed after {retry_cnt} retries "
f"({debug_info()}): {e!r}"
)
raise e
retry_cnt += 1
logger.warning(
f"read_rows_arrow retry {retry_cnt}/{max_retry_count} "
f"({debug_info()}): {e!r}"
)
time.sleep(random.choice([5, 9, 12]))
continue
except Exception as e:
logger.error(f"read_rows_arrow failed ({debug_info()}): {e!r}")
raise
break
return reader

Expand All @@ -231,7 +262,12 @@ def _get_session_record_count(
) -> int:
"""Get record count from a session, waiting until ready."""
while True:
scan_resp = client.get_read_session(sess_req)
try:
scan_resp = client.get_read_session(sess_req)
except Exception as e:
debug_info = _storage_debug_info(client, session_id=sess_req.session_id)
logger.error(f"get_read_session failed ({debug_info}): {e!r}")
raise
if scan_resp.session_status == SessionStatus.INIT:
time.sleep(1)
continue
Expand Down
107 changes: 107 additions & 0 deletions tzrec/datasets/odps_dataset_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,19 @@
import os
import time
import unittest
from types import SimpleNamespace
from unittest import mock

import numpy as np
import pyarrow as pa
import requests
from odps import ODPS
from odps.errors import ODPSError
from parameterized import parameterized
from torch import distributed as dist
from torch.utils.data import DataLoader

from tzrec.datasets import odps_dataset
from tzrec.datasets.odps_dataset import OdpsDataset, OdpsWriter, _create_odps_account
from tzrec.features.feature import FgMode, create_features
from tzrec.protos import data_pb2, feature_pb2, sampler_pb2
Expand Down Expand Up @@ -689,5 +694,107 @@ def _writer_worker(rank, port):
self.assertEqual(reader.count, 1280)


class _FailingStorageClient:
"""Stub storage-api client whose calls raise a connection-reset error."""

table = SimpleNamespace(full_table_name="test_project.test_table")
_quota_name = "test_quota"
_rest_endpoint = "http://service.odps.test"

def _raise(self):
raise requests.exceptions.ConnectionError(
"Connection aborted.",
ConnectionResetError(104, "Connection reset by peer"),
)

def read_rows_arrow(self, read_req):
self._raise()

def get_read_session(self, sess_req):
self._raise()


class _RetryingStorageClient:
"""Stub whose read_rows_arrow raises ODPSError `fail_times` times, then succeeds."""

table = SimpleNamespace(full_table_name="test_project.test_table")
_quota_name = "test_quota"
_rest_endpoint = "http://service.odps.test"

def __init__(self, fail_times):
self._fail_times = fail_times
self.calls = 0
self.sentinel = object()

def read_rows_arrow(self, read_req):
self.calls += 1
if self.calls <= self._fail_times:
raise ODPSError("synthetic ODPS error", request_id="REQ-XYZ")
return self.sentinel


class OdpsStorageErrorLogTest(unittest.TestCase):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def test_read_rows_arrow_logs_session_and_reraises(self):
client = _FailingStorageClient()
read_req = SimpleNamespace(
session_id="sess-read-123",
row_index=0,
row_count=10,
max_batch_rows=100,
)
with mock.patch.object(odps_dataset, "logger") as m_logger:
with self.assertRaises(requests.exceptions.ConnectionError):
odps_dataset._read_rows_arrow_with_retry(client, read_req)
m_logger.error.assert_called_once()
logged = m_logger.error.call_args[0][0]
self.assertIn("sess-read-123", logged)
self.assertIn("test_project.test_table", logged)

def test_get_read_session_logs_session_and_reraises(self):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

client = _FailingStorageClient()
sess_req = SimpleNamespace(session_id="sess-scan-456")
with mock.patch.object(odps_dataset, "logger") as m_logger:
with self.assertRaises(requests.exceptions.ConnectionError):
odps_dataset._get_session_record_count(client, sess_req)
m_logger.error.assert_called_once()
logged = m_logger.error.call_args[0][0]
self.assertIn("sess-scan-456", logged)

def test_read_rows_arrow_retries_odps_error_then_succeeds(self):
client = _RetryingStorageClient(fail_times=2)
read_req = SimpleNamespace(
session_id="sess-retry-1",
row_index=0,
row_count=10,
max_batch_rows=100,
)
with mock.patch.object(odps_dataset.time, "sleep"):
with mock.patch.object(odps_dataset, "logger") as m_logger:
reader = odps_dataset._read_rows_arrow_with_retry(client, read_req)
self.assertIs(reader, client.sentinel)
self.assertEqual(client.calls, 3) # 2 failures + 1 success
self.assertEqual(m_logger.warning.call_count, 2)
m_logger.error.assert_not_called()

def test_read_rows_arrow_odps_error_exhausts_retries_and_raises(self):
client = _RetryingStorageClient(fail_times=99)
read_req = SimpleNamespace(
session_id="sess-retry-2",
row_index=5,
row_count=10,
max_batch_rows=100,
)
with mock.patch.object(odps_dataset.time, "sleep"):
with mock.patch.object(odps_dataset, "logger") as m_logger:
with self.assertRaises(ODPSError):
odps_dataset._read_rows_arrow_with_retry(client, read_req)
self.assertEqual(client.calls, 4) # initial + 3 retries
self.assertEqual(m_logger.warning.call_count, 3)
m_logger.error.assert_called_once()
logged = m_logger.error.call_args[0][0]
self.assertIn("after 3 retries", logged)
self.assertIn("sess-retry-2", logged)


if __name__ == "__main__":
unittest.main()
2 changes: 1 addition & 1 deletion tzrec/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@
# See the License for the specific language governing permissions and
# limitations under the License.

__version__ = "1.2.19"
__version__ = "1.2.20"
Loading