-
Notifications
You must be signed in to change notification settings - Fork 77
[bugfix] ODPS reader: pin pyodps==0.12.5.1 (retry connection resets) + log session/table context on errors #537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5af5369
6d676ea
30237b0
1e59b81
54a9b8e
06c67de
cb8864b
60798f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new tests raise Consider adding a stub whose |
||
| 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): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Coverage gap: Consider a stub whose |
||
| 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() | ||
There was a problem hiding this comment.
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_nameis the one attribute accessed without a guard, whilequota/endpointbelow usegetattr(..., None). Ifclient.tableever raises (e.g. a partially-constructed client, or an unexpectedAttributeError), the diagnostic log this PR exists to produce won't be emitted, and the originalConnectionError/ODPSErrorgets buried behind a chained secondary exception.(I checked pyodps:
full_table_nameresolves via_get_schema_name()+ already-setproject.name/name, so it does not trigger a networkreload()— 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: