From a2aee2d9270d9900a2cb402f0f4a3e1f942017ab Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Thu, 13 Aug 2026 09:15:02 +0000 Subject: [PATCH] fix(sweep,sync): correct optuna install hint + drop false "0 records" warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small SDK bugs found in a docs audit of #131: - sweep: the bayes-needs-optuna hint said `pip install pluto[sweep]`, but the distribution is `pluto-ml` and `pluto` is an unrelated package on PyPI — so following the hint silently installs someone else's package (with an unknown extra) instead of failing. Corrected to `pip install "pluto-ml[sweep]"`. - op: finish() warned "{pending} records may not have been uploaded" using a count read AFTER stop()'s SIGTERM drain had already emptied the queue, so it printed "0 records may not have been uploaded" — a data-loss-looking message at the moment nothing was lost. Only warn when pending > 0. Tests: bayes-missing-optuna message names pluto-ml (not pluto); finish() doesn't warn when the drain emptied the queue but does when records genuinely remain. Note: does NOT touch the ~30s finish stall (throttle-vs-shutdown mismatch) — a separate, more involved change, deferred. Co-Authored-By: Claude Opus 4.8 --- pluto/op.py | 16 +++++++++----- pluto/sweep.py | 2 +- tests/test_run_status.py | 46 ++++++++++++++++++++++++++++++++++++++++ tests/test_sweep.py | 12 +++++++++++ 4 files changed, 70 insertions(+), 6 deletions(-) diff --git a/pluto/op.py b/pluto/op.py index 536e2f8..b0151c3 100644 --- a/pluto/op.py +++ b/pluto/op.py @@ -1164,12 +1164,18 @@ def _teardown(self, code: Union[int, None], update_status: bool) -> None: wait=True, ) if not sync_completed: + # stop() SIGTERMs the sync process on timeout, and its + # unthrottled drain usually empties the queue before we + # read the count here — so only warn when records are + # genuinely still unsent (avoids a contradictory + # "0 records may not have been uploaded"). pending = self._sync_manager.get_pending_count() - logger.warning( - f'{tag}: Sync did not complete within timeout, ' - f'{pending} records may not have been uploaded. ' - f'Data is preserved in {self._sync_manager.db_path}' - ) + if pending > 0: + logger.warning( + f'{tag}: Sync did not complete within timeout, ' + f'{pending} records may not have been uploaded. ' + f'Data is preserved in {self._sync_manager.db_path}' + ) self._sync_manager.close() self._sync_manager = None diff --git a/pluto/sweep.py b/pluto/sweep.py index cd443e6..eba2ae6 100644 --- a/pluto/sweep.py +++ b/pluto/sweep.py @@ -491,7 +491,7 @@ def _run_bayes( except ImportError: raise ImportError( "sweep method='bayes' needs optuna — install it with " - '`pip install optuna` (or `pip install pluto[sweep]`).' + '`pip install optuna` (or `pip install "pluto-ml[sweep]"`).' ) if count is None: raise ValueError("method='bayes' needs count= in pluto.agent(...)") diff --git a/tests/test_run_status.py b/tests/test_run_status.py index 5e3d435..3b2a672 100644 --- a/tests/test_run_status.py +++ b/tests/test_run_status.py @@ -408,6 +408,52 @@ def test_finish_confirmed_status_clears_error_flag(self): assert settings._op_status == 0 +class TestFinishDrainWarning: + """finish()'s 'records may not have been uploaded' warning must not fire when + the SIGTERM drain already emptied the queue (pending == 0) — otherwise it + prints a contradictory '0 records may not have been uploaded' that reads like + data loss at the moment nothing was lost. + """ + + def _noop_op(self): + from pluto.op import Op + from pluto.sets import Settings + + settings = Settings() + settings.mode = 'noop' + settings.sync_process_enabled = False # don't spawn a real sync subprocess + op = Op(config={}, settings=settings) + op.start() + return op + + def _warns(self, op): + from unittest.mock import patch + + with patch('pluto.op.logger.warning') as warn: + op.finish() + return ' '.join(str(c.args[0]) for c in warn.call_args_list if c.args) + + def test_no_warning_when_drain_emptied_queue(self): + from unittest.mock import MagicMock + + op = self._noop_op() + sm = MagicMock() + sm.stop.return_value = False # 30s wait "timed out" + sm.get_pending_count.return_value = 0 # but the drain emptied it + op._sync_manager = sm + assert 'may not have been uploaded' not in self._warns(op) + + def test_warns_when_records_genuinely_pending(self): + from unittest.mock import MagicMock + + op = self._noop_op() + sm = MagicMock() + sm.stop.return_value = False + sm.get_pending_count.return_value = 5 # genuinely unsent + op._sync_manager = sm + assert '5 records may not have been uploaded' in self._warns(op) + + class TestExcepthookSubprocess: """ End-to-end test: run a script that raises an unhandled exception diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 6650b08..62d99de 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -307,3 +307,15 @@ def test_bayes_requires_count(self, monkeypatch): ) with pytest.raises(ValueError, match='needs count'): pluto.agent(sid, lambda: None) + + +def test_bayes_missing_optuna_names_the_real_package(monkeypatch): + # When optuna isn't installed, the hint must point at the real distribution + # name (pluto-ml) — NOT `pluto`, which is an unrelated package on PyPI, so + # following the wrong hint silently installs someone else's package. + monkeypatch.setitem(sys.modules, 'optuna', None) # makes `import optuna` raise + with pytest.raises(ImportError) as exc: + sw._run_bayes('sid', {}, None, lambda: None, 1, [], 'loss') + msg = str(exc.value) + assert 'pluto-ml[sweep]' in msg + assert 'pluto[sweep]' not in msg