From 4865588393e5292483575fb417b549d046ff496f Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 13 May 2026 10:07:18 +0200 Subject: [PATCH 01/14] instrument configs in ui get sensor choices --- src/virtualship/cli/_plan.py | 50 ++++++++++++++++++++++++++++++ src/virtualship/models/__init__.py | 2 ++ 2 files changed, 52 insertions(+) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index e397aad7..68619501 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -36,6 +36,7 @@ DrifterConfig, Expedition, Location, + SensorConfig, ShipConfig, ShipUnderwaterSTConfig, Waypoint, @@ -76,6 +77,14 @@ def log_exception_to_file( f.write("\n") +def _default_sensors(config_class) -> list: + """List of SensorConfig instances from the config class's sensors default_factory.""" + sensors_field = config_class.model_fields.get("sensors") + if sensors_field is None or sensors_field.default_factory is None: + return [] + return sensors_field.default_factory() + + DEFAULT_TS_CONFIG = {"period_minutes": 5.0} DEFAULT_ADCP_CONFIG = { @@ -83,6 +92,7 @@ def log_exception_to_file( "period_minutes": 5.0, } + INSTRUMENT_FIELDS = { "adcp_config": { "class": ADCPConfig, @@ -339,6 +349,34 @@ def compose(self) -> ComposeResult: id=f"validation-failure-label-{instrument_name}_{attr}", classes="-hidden validation-failure", ) + # sensor toggles, derived from the config class's sensors default_factory + default_sensor_configs = _default_sensors(config_class) + if default_sensor_configs: + yield Label("[b]Sensors:[/b]", markup=True) + # which sensors are currently active + if config_instance and hasattr( + config_instance, "sensors" + ): + active_sensor_types = { + sc.sensor_type + for sc in config_instance.sensors + if sc.enabled + } + else: + # if no config loaded yet, default all sensors on + active_sensor_types = { + sc.sensor_type for sc in default_sensor_configs + } + for sc in default_sensor_configs: + sensor_id = f"{instrument_name}_sensor_{sc.sensor_type.value}" + with Horizontal(classes="sensor-toggle-row"): + yield Label( + f" {sc.sensor_type.value.replace('_', ' ').title()}:" + ) + yield Switch( + value=sc.sensor_type in active_sensor_types, + id=sensor_id, + ) ## 2) SCHEDULE EDITOR @@ -449,6 +487,18 @@ def _update_instrument_configs(self): kwargs["max_depth_meter"] = -1000.0 else: kwargs["max_depth_meter"] = -150.0 + # collect sensor toggles + default_sensor_configs = _default_sensors(config_class) + if default_sensor_configs: + sensors = [ + SensorConfig(sensor_type=sc.sensor_type) + for sc in default_sensor_configs + if self.query_one( + f"#{instrument_name}_sensor_{sc.sensor_type.value}", Switch + ).value + ] + if sensors: + kwargs["sensors"] = sensors setattr( self.expedition.instruments_config, instrument_name, diff --git a/src/virtualship/models/__init__.py b/src/virtualship/models/__init__.py index 71f24211..dd4b2bf1 100644 --- a/src/virtualship/models/__init__.py +++ b/src/virtualship/models/__init__.py @@ -9,6 +9,7 @@ Expedition, InstrumentsConfig, Schedule, + SensorConfig, ShipConfig, ShipUnderwaterSTConfig, Waypoint, @@ -23,6 +24,7 @@ __all__ = [ # noqa: RUF022 "Location", "Schedule", + "SensorConfig", "ShipConfig", "Waypoint", "ArgoFloatConfig", From efc9ce2b805caeae59bdec38c78791e6bbed7315 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 13 May 2026 10:19:24 +0200 Subject: [PATCH 02/14] better handle non valid combinations of active instruments with no active sensors --- src/virtualship/cli/_plan.py | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 68619501..701a320e 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -101,6 +101,7 @@ def _default_sensors(config_class) -> list: {"name": "num_bins"}, {"name": "period", "minutes": True}, ], + # underway instrument, so active/inactive is controlled by the on/off toggle, not the schedule (therefore no instrument_type key) }, "ship_underwater_st_config": { "class": ShipUnderwaterSTConfig, @@ -108,10 +109,12 @@ def _default_sensors(config_class) -> list: "attributes": [ {"name": "period", "minutes": True}, ], + # underway instrument, so active/inactive is controlled by the on/off toggle, not the schedule (therefore no instrument_type key) }, "ctd_config": { "class": CTDConfig, "title": "CTD", + "instrument_type": InstrumentType.CTD, "attributes": [ {"name": "max_depth_meter"}, {"name": "min_depth_meter"}, @@ -121,6 +124,7 @@ def _default_sensors(config_class) -> list: "xbt_config": { "class": XBTConfig, "title": "XBT", + "instrument_type": InstrumentType.XBT, "attributes": [ {"name": "min_depth_meter"}, {"name": "max_depth_meter"}, @@ -131,6 +135,7 @@ def _default_sensors(config_class) -> list: "argo_float_config": { "class": ArgoFloatConfig, "title": "Argo Float", + "instrument_type": InstrumentType.ARGO_FLOAT, "attributes": [ {"name": "min_depth_meter"}, {"name": "max_depth_meter"}, @@ -145,6 +150,7 @@ def _default_sensors(config_class) -> list: "drifter_config": { "class": DrifterConfig, "title": "Drifter", + "instrument_type": InstrumentType.DRIFTER, "attributes": [ {"name": "depth_meter"}, {"name": "lifetime", "days": True}, @@ -431,6 +437,8 @@ def save_changes(self) -> bool: self._update_schedule() self.expedition.to_yaml(self.path.joinpath(EXPEDITION)) return True + except UserError: + raise except Exception as e: log_exception_to_file( e, @@ -497,8 +505,31 @@ def _update_instrument_configs(self): f"#{instrument_name}_sensor_{sc.sensor_type.value}", Switch ).value ] - if sensors: - kwargs["sensors"] = sensors + if not sensors: + # for schedule-based instruments, only raise if actually used in a waypoint + # for underway, this is handled by the on/off toggle + instrument_type = info.get("instrument_type") + is_active = instrument_type is None or any( + instrument_type + in ( + wp.instrument + if isinstance(wp.instrument, list) + else [wp.instrument] + ) + for wp in self.expedition.schedule.waypoints + if wp.instrument + ) + if is_active: + title = info.get( + "title", instrument_name.replace("_", " ").title() + ) + raise UserError( + f"'{title}' has no sensors selected. " + f"At least one sensor must be enabled for each active instrument." + ) + kwargs["sensors"] = ( + sensors if sensors else _default_sensors(config_class) + ) setattr( self.expedition.instruments_config, instrument_name, From 90ef8c17d186e2637b4ca2be353168e293e4a26c Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 13 May 2026 10:20:42 +0200 Subject: [PATCH 03/14] better horizontal align sensor toggles --- src/virtualship/cli/_plan.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 701a320e..ac992445 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -1337,6 +1337,18 @@ class PlanApp(App): margin: 0 1; } + .sensor-toggle-row { + height: auto; + margin: 0; + padding: 0; + } + + .sensor-toggle-row Label { + width: 24; + margin-top: 1; + color: $text-muted; + } + .year-select { width: 20; } From 33a84760c960005acebb306fdec2bb38787e274e Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 13 May 2026 12:03:26 +0200 Subject: [PATCH 04/14] add new and update existing tests --- tests/cli/test_plan.py | 262 +++++++++++++++++++++++++++++++++-------- 1 file changed, 210 insertions(+), 52 deletions(-) diff --git a/tests/cli/test_plan.py b/tests/cli/test_plan.py index f0522cde..078590b1 100644 --- a/tests/cli/test_plan.py +++ b/tests/cli/test_plan.py @@ -1,20 +1,20 @@ -import os -import shutil -import tempfile from datetime import datetime from pathlib import Path from unittest.mock import MagicMock import pytest import yaml -from textual.widgets import Button, Collapsible, Input +from textual.widgets import Button, Collapsible, Input, Switch -from virtualship.cli._plan import ExpeditionEditor, PlanApp +from virtualship.cli._plan import ExpeditionEditor, PlanApp, _default_sensors +from virtualship.instruments.sensors import SensorType from virtualship.models import ( + CTDConfig, Expedition, InstrumentsConfig, Location, Schedule, + SensorConfig, Waypoint, ) from virtualship.utils import EXPEDITION, get_example_expedition @@ -24,6 +24,24 @@ NEW_LON = "0.015" +def _make_expedition( + tmpdir: Path, + waypoints: list, + instruments_config=None, +) -> None: + """Write a minimal expedition YAML.""" + if instruments_config is None: + instruments_config = InstrumentsConfig.model_validate( + yaml.safe_load(get_example_expedition()).get("instruments_config") + ) + ship_config = yaml.safe_load(get_example_expedition()).get("ship_config") + Expedition( + schedule=Schedule(waypoints=waypoints), + instruments_config=instruments_config, + ship_config=ship_config, + ).to_yaml(tmpdir / EXPEDITION) + + async def simulate_input(pilot, box, new_value): """Simulate inputs to the UI.""" box.focus() @@ -36,14 +54,8 @@ async def simulate_input(pilot, box, new_value): @pytest.mark.asyncio -async def test_UI_changes(): +async def test_UI_changes(tmp_path): """Test making changes to UI inputs and saving to YAML (simulated botton presses and typing inputs).""" - tmpdir = Path(tempfile.mkdtemp()) - - instruments_config = InstrumentsConfig.model_validate( - yaml.safe_load(get_example_expedition()).get("instruments_config") - ) - ship_config = yaml.safe_load(get_example_expedition()).get("ship_config") waypoints = [ Waypoint( location=Location(0, 0), @@ -61,15 +73,9 @@ async def test_UI_changes(): instrument=["CTD"], ), ] - expedition = Expedition( - schedule=Schedule(waypoints=waypoints), - instruments_config=instruments_config, - ship_config=ship_config, - ) - - expedition.to_yaml(tmpdir / EXPEDITION) + _make_expedition(tmp_path, waypoints) - app = PlanApp(path=tmpdir) + app = PlanApp(path=tmp_path) async with app.run_test(size=(120, 100)) as pilot: await pilot.pause(0.5) @@ -133,8 +139,7 @@ async def test_UI_changes(): ) # verify changes to speed, lat, lon in saved YAML - expedition_path = os.path.join(tmpdir, EXPEDITION) - with open(expedition_path) as f: + with open(tmp_path / EXPEDITION) as f: saved_expedition = yaml.safe_load(f) assert saved_expedition["ship_config"]["ship_speed_knots"] == float(NEW_SPEED) @@ -148,45 +153,22 @@ async def test_UI_changes(): args, _ = plan_screen.notify.call_args assert "*** Error saving changes ***" in args[0] - # cleanup - shutil.rmtree(tmpdir) - @pytest.mark.asyncio -async def test_UI_opens_with_null_time_and_instrument(): +async def test_UI_opens_with_null_time_and_instrument(tmp_path): """Test that the UI opens correctly when waypoints have time: null and instrument: null.""" - tmpdir = Path(tempfile.mkdtemp()) - - instruments_config = InstrumentsConfig.model_validate( - yaml.safe_load(get_example_expedition()).get("instruments_config") - ) - ship_config = yaml.safe_load(get_example_expedition()).get("ship_config") waypoints = [ Waypoint( location=Location(0, 0), time=datetime(2022, 1, 1, 0, 0, 0), instrument=["CTD"], ), - Waypoint( - location=Location(0.01, 0.01), - time=None, - instrument=None, - ), - Waypoint( - location=Location(0.02, 0.02), - time=None, - instrument=None, - ), + Waypoint(location=Location(0.01, 0.01), time=None, instrument=None), + Waypoint(location=Location(0.02, 0.02), time=None, instrument=None), ] - expedition = Expedition( - schedule=Schedule(waypoints=waypoints), - instruments_config=instruments_config, - ship_config=ship_config, - ) + _make_expedition(tmp_path, waypoints) - expedition.to_yaml(tmpdir / EXPEDITION) - - app = PlanApp(path=tmpdir) + app = PlanApp(path=tmp_path) async with app.run_test(size=(120, 100)) as pilot: await pilot.pause(0.5) @@ -197,5 +179,181 @@ async def test_UI_opens_with_null_time_and_instrument(): # verify the app opened without errors by checking the editor loaded assert expedition_editor is not None - # cleanup - shutil.rmtree(tmpdir) + +def test_default_sensors_returns_declared_defaults(): + """_default_sensors() should mirror the config class's default sensor list.""" + ctd_sensors = _default_sensors(CTDConfig) + types = {sc.sensor_type for sc in ctd_sensors} + assert SensorType.TEMPERATURE in types + assert SensorType.SALINITY in types + + +@pytest.mark.asyncio +async def test_sensor_toggle_saved_to_yaml(tmp_path): + """Toggling a sensor switch off and saving should persist only the enabled sensors.""" + _make_expedition( + tmp_path, + [ + Waypoint( + location=Location(0, 0), + time=datetime(2022, 1, 1, 0, 0, 0), + instrument=["CTD"], + ), + Waypoint( + location=Location(0.01, 0.01), + time=datetime(2022, 1, 1, 1, 0, 0), + instrument=None, + ), + ], + ) + + app = PlanApp(path=tmp_path) + async with app.run_test(size=(160, 120)) as pilot: + await pilot.pause(0.5) + plan_screen = pilot.app.screen + plan_screen.notify = MagicMock() + expedition_editor = plan_screen.query_one(ExpeditionEditor) + + # turn off SALINITY on CTD + sal_switch = expedition_editor.query_one("#ctd_config_sensor_SALINITY", Switch) + await pilot.click(sal_switch) + await pilot.pause(0.2) + + await pilot.click(plan_screen.query_one("#save_button", Button)) + await pilot.pause(0.5) + + plan_screen.notify.assert_called_once_with( + "Changes saved successfully", severity="information", timeout=20 + ) + + with open(tmp_path / EXPEDITION) as f: + saved = yaml.safe_load(f) + saved_sensors = saved["instruments_config"]["ctd_config"]["sensors"] + assert "SALINITY" not in saved_sensors + assert "TEMPERATURE" in saved_sensors + + +@pytest.mark.asyncio +async def test_deselecting_all_sensors_on_active_instrument_blocks_save(tmp_path): + """Removing all sensors from an instrument used in the schedule should show an error.""" + _make_expedition( + tmp_path, + [ + Waypoint( + location=Location(0, 0), + time=datetime(2022, 1, 1, 0, 0, 0), + instrument=["XBT"], + ), + Waypoint( + location=Location(0.01, 0.01), + time=datetime(2022, 1, 1, 1, 0, 0), + instrument=None, + ), + ], + ) + + app = PlanApp(path=tmp_path) + async with app.run_test(size=(160, 120)) as pilot: + await pilot.pause(0.5) + plan_screen = pilot.app.screen + plan_screen.notify = MagicMock() + expedition_editor = plan_screen.query_one(ExpeditionEditor) + + # XBT only has TEMPERATURE, deselect it + temp_switch = expedition_editor.query_one( + "#xbt_config_sensor_TEMPERATURE", Switch + ) + await pilot.click(temp_switch) + await pilot.pause(0.2) + + await pilot.click(plan_screen.query_one("#save_button", Button)) + await pilot.pause(0.5) + + args, _ = plan_screen.notify.call_args + assert "*** Error saving changes ***" in args[0] + assert "XBT" in args[0] + + +@pytest.mark.asyncio +async def test_deselecting_all_sensors_on_inactive_instrument(tmp_path): + """Removing all sensors from an instrument NOT in the schedule should not block saving.""" + # only CTD waypoints, XBT is inactive + _make_expedition( + tmp_path, + [ + Waypoint( + location=Location(0, 0), + time=datetime(2022, 1, 1, 0, 0, 0), + instrument=["CTD"], + ), + Waypoint( + location=Location(0.01, 0.01), + time=datetime(2022, 1, 1, 1, 0, 0), + instrument=None, + ), + ], + ) + + app = PlanApp(path=tmp_path) + async with app.run_test(size=(160, 120)) as pilot: + await pilot.pause(0.5) + plan_screen = pilot.app.screen + plan_screen.notify = MagicMock() + expedition_editor = plan_screen.query_one(ExpeditionEditor) + + # deselect XBT's only sensor even though XBT is not in the schedule + temp_switch = expedition_editor.query_one( + "#xbt_config_sensor_TEMPERATURE", Switch + ) + await pilot.click(temp_switch) + await pilot.pause(0.2) + + await pilot.click(plan_screen.query_one("#save_button", Button)) + await pilot.pause(0.5) + + plan_screen.notify.assert_called_once_with( + "Changes saved successfully", severity="information", timeout=20 + ) + + +@pytest.mark.asyncio +async def test_sensor_initial_state_reflects_config(tmp_path): + """Sensor switches should reflect the enabled sensors already stored in the config.""" + ctd_config = CTDConfig( + stationkeeping_time_minutes=50, + min_depth_meter=-11.0, + max_depth_meter=-2000.0, + sensors=[SensorConfig(sensor_type=SensorType.TEMPERATURE)], + ) + instruments_config = InstrumentsConfig.model_validate( + yaml.safe_load(get_example_expedition()).get("instruments_config") + ) + instruments_config.ctd_config = ctd_config + _make_expedition( + tmp_path, + [ + Waypoint( + location=Location(0, 0), + time=datetime(2022, 1, 1, 0, 0, 0), + instrument=["CTD"], + ), + Waypoint( + location=Location(0.01, 0.01), + time=datetime(2022, 1, 1, 1, 0, 0), + instrument=None, + ), + ], + instruments_config, + ) + + app = PlanApp(path=tmp_path) + async with app.run_test(size=(160, 120)) as pilot: + await pilot.pause(0.5) + expedition_editor = pilot.app.screen.query_one(ExpeditionEditor) + + temp_switch = expedition_editor.query_one( + "#ctd_config_sensor_TEMPERATURE", Switch + ) + sal_switch = expedition_editor.query_one("#ctd_config_sensor_SALINITY", Switch) + assert temp_switch.value is True, "TEMPERATURE should be ON" + assert sal_switch.value is False, "SALINITY should be OFF (not in saved config)" From 16f56fbd5a9b6c1e2284b5b8a08ce7853d694602 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 11:04:39 +0200 Subject: [PATCH 05/14] handle nested collapsibles within the UI when testing --- tests/cli/test_plan.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/cli/test_plan.py b/tests/cli/test_plan.py index 078590b1..29459223 100644 --- a/tests/cli/test_plan.py +++ b/tests/cli/test_plan.py @@ -53,6 +53,23 @@ async def simulate_input(pilot, box, new_value): await pilot.pause(0.05) +async def _expand_instrument_configs( + expedition_editor, pilot, instrument_title: str | None = None +): + """Expand the outer 'Instrument Configurations' collapsible and the instrument-specific inner one.""" + for coll in expedition_editor.query(Collapsible): + if "Instrument Configurations" in (coll.title or ""): + coll.collapsed = False + await pilot.pause() + break + if instrument_title is not None: + for coll in expedition_editor.query(Collapsible): + if instrument_title in (coll.title or ""): + coll.collapsed = False + await pilot.pause() + break + + @pytest.mark.asyncio async def test_UI_changes(tmp_path): """Test making changes to UI inputs and saving to YAML (simulated botton presses and typing inputs).""" @@ -213,6 +230,7 @@ async def test_sensor_toggle_saved_to_yaml(tmp_path): plan_screen = pilot.app.screen plan_screen.notify = MagicMock() expedition_editor = plan_screen.query_one(ExpeditionEditor) + await _expand_instrument_configs(expedition_editor, pilot, "CTD") # turn off SALINITY on CTD sal_switch = expedition_editor.query_one("#ctd_config_sensor_SALINITY", Switch) @@ -258,6 +276,7 @@ async def test_deselecting_all_sensors_on_active_instrument_blocks_save(tmp_path plan_screen = pilot.app.screen plan_screen.notify = MagicMock() expedition_editor = plan_screen.query_one(ExpeditionEditor) + await _expand_instrument_configs(expedition_editor, pilot, "XBT") # XBT only has TEMPERATURE, deselect it temp_switch = expedition_editor.query_one( @@ -300,6 +319,7 @@ async def test_deselecting_all_sensors_on_inactive_instrument(tmp_path): plan_screen = pilot.app.screen plan_screen.notify = MagicMock() expedition_editor = plan_screen.query_one(ExpeditionEditor) + await _expand_instrument_configs(expedition_editor, pilot, "XBT") # deselect XBT's only sensor even though XBT is not in the schedule temp_switch = expedition_editor.query_one( From 3cdde84191f6ad9c0f80ea8f8ab9df2f78cbc348 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 11:10:03 +0200 Subject: [PATCH 06/14] remove reference to "advanced users only" for instrument configs; not true anymore --- src/virtualship/cli/_plan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index ac992445..f2e786c7 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -281,7 +281,7 @@ def compose(self) -> ComposeResult: ## SECTION: "Instrument Configurations"" with Collapsible( - title="[b]Instrument Configurations[/b] (advanced users only)", + title="[b]Instrument Configurations[/b]", collapsed=True, ): for instrument_name, info in INSTRUMENT_FIELDS.items(): From b198577bccda74a5b4cdc097786ce359db4ac8b0 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 11:55:19 +0200 Subject: [PATCH 07/14] handle cache presence errors passively when present in 'easy' mode, also refactor _unique_id() --- src/virtualship/cli/_run.py | 51 ++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/virtualship/cli/_run.py b/src/virtualship/cli/_run.py index 2feb43a3..875d977f 100644 --- a/src/virtualship/cli/_run.py +++ b/src/virtualship/cli/_run.py @@ -85,7 +85,7 @@ def _run( # unique id to determine if an expedition has 'changed' since last run (to avoid re-selecting problems when user makes tweaks to schedule to deal with problems encountered) cache_dir = expedition_dir.joinpath(CACHE) - expedition_id = _unique_id(expedition, cache_dir) + expedition_id = _unique_id(expedition, cache_dir, difficulty_level) # dedicated problems directory for this expedition problems_dir = expedition_dir.joinpath( @@ -251,40 +251,43 @@ def _run( print(f"[TIMER] Expedition completed in {elapsed / 60.0:.2f} minutes.") -def _unique_id(expedition: Expedition, cache_dir: Path) -> str: +def _unique_id(expedition: Expedition, cache_dir: Path, difficulty_level: str) -> str: """ Return a unique id for the expedition (marked by datetime), which can be used to determine whether the expedition has 'changed' since the last run. - Simultaneously, log to .txt file if first run or if there have been additions of instruments since last run. + Returns the previous id if no instruments have been added since the last run, allowing re-use of previously encountered problems. + Otherwise generates and persists a new id. """ - current_instruments = expedition.get_instruments() + cache_dir.mkdir(exist_ok=True) + + id_path = cache_dir / EXPEDITION_IDENTIFIER + last_expedition_path = cache_dir / EXPEDITION_LATEST new_id = datetime.now().strftime("%Y%m%d%H%M%S") - previous_id = None - if not cache_dir.exists(): - cache_dir.mkdir() - id_path = cache_dir.joinpath(EXPEDITION_IDENTIFIER) - last_expedition_path = cache_dir.joinpath(EXPEDITION_LATEST) + if not id_path.exists(): + id_path.write_text(new_id) + return new_id - if id_path.exists(): - previous_id = id_path.read_text().strip() - try: - last_expedition = Expedition.from_yaml(last_expedition_path) - except FileNotFoundError as e: - raise RuntimeError( - f"Previous expedition data is present but incomplete in {cache_dir}. This may be because a previous expedition run was interrupted/failed unexpectedly. Deleting the '{cache_dir}' directory and re-running the expedition should resolve this issue." - ) from e - last_instruments = last_expedition.get_instruments() + previous_id = id_path.read_text().strip() - added_instruments = set(current_instruments) - set(last_instruments) - if not added_instruments: - return previous_id # if no additions, keep previous id to allow re-use of previously encountered problems - else: + try: + last_expedition = Expedition.from_yaml(last_expedition_path) + except FileNotFoundError as e: + if difficulty_level == "easy": + # cache is irrelevant in easy mode; update passively id_path.write_text(new_id) + return new_id + raise RuntimeError( + f"Previous expedition data is present but incomplete in {cache_dir}. This may be because a previous expedition run was interrupted/failed unexpectedly. Deleting the '{cache_dir}' directory and re-running the expedition should resolve this issue." + ) from e - else: - id_path.write_text(new_id) + added_instruments = set(expedition.get_instruments()) - set( + last_expedition.get_instruments() + ) + if not added_instruments: + return previous_id # if no additions, keep previous id to allow re-use of previously encountered problems + id_path.write_text(new_id) return new_id From 4bf6a18afbb7669dc4c25cd582a79934d972d270 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 14:47:12 +0200 Subject: [PATCH 08/14] generalise to not throwing error when cache is incomplete; not useful information --- src/virtualship/cli/_run.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/virtualship/cli/_run.py b/src/virtualship/cli/_run.py index 875d977f..14b35cc0 100644 --- a/src/virtualship/cli/_run.py +++ b/src/virtualship/cli/_run.py @@ -272,14 +272,11 @@ def _unique_id(expedition: Expedition, cache_dir: Path, difficulty_level: str) - try: last_expedition = Expedition.from_yaml(last_expedition_path) - except FileNotFoundError as e: - if difficulty_level == "easy": - # cache is irrelevant in easy mode; update passively - id_path.write_text(new_id) - return new_id - raise RuntimeError( - f"Previous expedition data is present but incomplete in {cache_dir}. This may be because a previous expedition run was interrupted/failed unexpectedly. Deleting the '{cache_dir}' directory and re-running the expedition should resolve this issue." - ) from e + except FileNotFoundError: + # cache is not useful in this case as it implies the previous run was interrupted and is incomplete; update passively + # TODO: check this doesn't intefere with re-use of previously encountered problems in the case of interrupted runs (i.e. if no new instruments added, should still re-use previous id and problems) + id_path.write_text(new_id) + return new_id added_instruments = set(expedition.get_instruments()) - set( last_expedition.get_instruments() From 0738bd2817ae07e5cb46a2ccd2f90805940146e8 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 15:09:59 +0200 Subject: [PATCH 09/14] tidy up TODO --- src/virtualship/cli/_run.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/virtualship/cli/_run.py b/src/virtualship/cli/_run.py index 14b35cc0..0578f23d 100644 --- a/src/virtualship/cli/_run.py +++ b/src/virtualship/cli/_run.py @@ -274,7 +274,6 @@ def _unique_id(expedition: Expedition, cache_dir: Path, difficulty_level: str) - last_expedition = Expedition.from_yaml(last_expedition_path) except FileNotFoundError: # cache is not useful in this case as it implies the previous run was interrupted and is incomplete; update passively - # TODO: check this doesn't intefere with re-use of previously encountered problems in the case of interrupted runs (i.e. if no new instruments added, should still re-use previous id and problems) id_path.write_text(new_id) return new_id From 2c62b83c67ad46014c0a260043c79559e20931b3 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 15:10:27 +0200 Subject: [PATCH 10/14] add new test for _unique_id(); does not throw error and returns new id when cache is incomplete --- tests/cli/test_run.py | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/tests/cli/test_run.py b/tests/cli/test_run.py index bbdc2fa9..17b33b17 100644 --- a/tests/cli/test_run.py +++ b/tests/cli/test_run.py @@ -1,12 +1,16 @@ from datetime import datetime from pathlib import Path +from unittest.mock import MagicMock -from virtualship.cli._run import _run +import pytest + +from virtualship.cli._run import _run, _unique_id from virtualship.expedition.simulate_schedule import ( MeasurementsToSimulate, ScheduleOk, ) -from virtualship.utils import EXPEDITION, get_example_expedition +from virtualship.instruments.types import InstrumentType +from virtualship.utils import EXPEDITION, EXPEDITION_IDENTIFIER, get_example_expedition def _simulate_schedule(projection, expedition): @@ -71,3 +75,30 @@ def test_run(tmp_path, monkeypatch): if difficulty_level == "easy": cache_dir = expedition_dir / "cache" assert not cache_dir.exists() + + +# --------------------------------------------------------------------------- +# Unit tests for _unique_id +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("difficulty_level", ["easy", "hard", "medium"]) +def test_unique_id_incomplete_cache_does_not_raise(tmp_path, difficulty_level): + """When expedition_latest.yaml is missing (incomplete cache from an interrupted run), _unique_id must not raise error for any 'difficulty_level', it should return a new id.""" + ORIGINAL_TIMESTAMP = "20240101120000" + + cache_dir = tmp_path / "cache" + cache_dir.mkdir() + (cache_dir / EXPEDITION_IDENTIFIER).write_text(ORIGINAL_TIMESTAMP) + # EXPEDITION_LATEST intentionally absent to simulate an interrupted run + + expedition = MagicMock() + expedition.get_instruments.return_value = {InstrumentType.CTD} + + result = _unique_id(expedition, cache_dir, difficulty_level=difficulty_level) + + assert result != ORIGINAL_TIMESTAMP + assert ( + abs(datetime.strptime(result, "%Y%m%d%H%M%S") - datetime.now()).seconds < 30 + ), "new ID should be timestamp close to the current time" + assert (cache_dir / EXPEDITION_IDENTIFIER).read_text().strip() == result From 5685551a5035d7802b356585308aa6ddefc53d05 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 15:20:47 +0200 Subject: [PATCH 11/14] remove rundandant change --- src/virtualship/cli/_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/virtualship/cli/_run.py b/src/virtualship/cli/_run.py index 0578f23d..1e9a23ba 100644 --- a/src/virtualship/cli/_run.py +++ b/src/virtualship/cli/_run.py @@ -251,7 +251,7 @@ def _run( print(f"[TIMER] Expedition completed in {elapsed / 60.0:.2f} minutes.") -def _unique_id(expedition: Expedition, cache_dir: Path, difficulty_level: str) -> str: +def _unique_id(expedition: Expedition, cache_dir: Path) -> str: """ Return a unique id for the expedition (marked by datetime), which can be used to determine whether the expedition has 'changed' since the last run. From e2d6f5ec63da2f0c05f30df61a9a2d23ecaa81a8 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 15:21:46 +0200 Subject: [PATCH 12/14] remove more redudancy --- src/virtualship/cli/_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/virtualship/cli/_run.py b/src/virtualship/cli/_run.py index 1e9a23ba..8e258903 100644 --- a/src/virtualship/cli/_run.py +++ b/src/virtualship/cli/_run.py @@ -85,7 +85,7 @@ def _run( # unique id to determine if an expedition has 'changed' since last run (to avoid re-selecting problems when user makes tweaks to schedule to deal with problems encountered) cache_dir = expedition_dir.joinpath(CACHE) - expedition_id = _unique_id(expedition, cache_dir, difficulty_level) + expedition_id = _unique_id(expedition, cache_dir) # dedicated problems directory for this expedition problems_dir = expedition_dir.joinpath( From 2ab55397073807ebcf3e24c0133a6b3a6c886857 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 15:24:06 +0200 Subject: [PATCH 13/14] difficulty level not needed in tests --- tests/cli/test_run.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/tests/cli/test_run.py b/tests/cli/test_run.py index 17b33b17..b537e405 100644 --- a/tests/cli/test_run.py +++ b/tests/cli/test_run.py @@ -2,8 +2,6 @@ from pathlib import Path from unittest.mock import MagicMock -import pytest - from virtualship.cli._run import _run, _unique_id from virtualship.expedition.simulate_schedule import ( MeasurementsToSimulate, @@ -77,13 +75,7 @@ def test_run(tmp_path, monkeypatch): assert not cache_dir.exists() -# --------------------------------------------------------------------------- -# Unit tests for _unique_id -# --------------------------------------------------------------------------- - - -@pytest.mark.parametrize("difficulty_level", ["easy", "hard", "medium"]) -def test_unique_id_incomplete_cache_does_not_raise(tmp_path, difficulty_level): +def test_unique_id_incomplete_cache_does_not_raise(tmp_path): """When expedition_latest.yaml is missing (incomplete cache from an interrupted run), _unique_id must not raise error for any 'difficulty_level', it should return a new id.""" ORIGINAL_TIMESTAMP = "20240101120000" @@ -95,7 +87,7 @@ def test_unique_id_incomplete_cache_does_not_raise(tmp_path, difficulty_level): expedition = MagicMock() expedition.get_instruments.return_value = {InstrumentType.CTD} - result = _unique_id(expedition, cache_dir, difficulty_level=difficulty_level) + result = _unique_id(expedition, cache_dir) assert result != ORIGINAL_TIMESTAMP assert ( From d81b44b2d4db845c35357c3b6d150abf2d0b9ff5 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 20 May 2026 09:26:17 +0200 Subject: [PATCH 14/14] from / to .joinpath() --- src/virtualship/cli/_run.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/virtualship/cli/_run.py b/src/virtualship/cli/_run.py index 8e258903..f2622be3 100644 --- a/src/virtualship/cli/_run.py +++ b/src/virtualship/cli/_run.py @@ -260,8 +260,8 @@ def _unique_id(expedition: Expedition, cache_dir: Path) -> str: """ cache_dir.mkdir(exist_ok=True) - id_path = cache_dir / EXPEDITION_IDENTIFIER - last_expedition_path = cache_dir / EXPEDITION_LATEST + id_path = cache_dir.joinpath(EXPEDITION_IDENTIFIER) + last_expedition_path = cache_dir.joinpath(EXPEDITION_LATEST) new_id = datetime.now().strftime("%Y%m%d%H%M%S") if not id_path.exists():