diff --git a/.dev/tools/check-cookbook-snippets.py b/.dev/tools/check-cookbook-snippets.py index 59924be..ac74d85 100755 --- a/.dev/tools/check-cookbook-snippets.py +++ b/.dev/tools/check-cookbook-snippets.py @@ -87,6 +87,8 @@ ConfigurationActivationQuery, ConfigurationActivationQuery as CA, to_timestamp, + activation_is_open, + activation_end_time, SaveConfigurationRequestParams, SaveConfigurationActivationRequestParams, QueryClient, diff --git a/CLAUDE.md b/CLAUDE.md index cd97d2d..7f3292d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -234,7 +234,7 @@ plan documents one change, `CLAUDE.md` documents the invariant it established. - `src/dp_python_lib/client/ingestion_client.py` - Ingestion service client with methods like `register_provider()` - `src/dp_python_lib/client/annotation_client.py` - Annotation service facade; groups feature-scoped clients sharing the one `DpAnnotationService` channel (`.pv_metadata`, `.machine_config`, `.sample_status`, `.datasets`, `.annotations`, `.export` — every implemented `DpAnnotationService` feature area) - `src/dp_python_lib/client/pv_metadata_client.py` - PV metadata client (`save_pv_metadata()`, `get_pv_metadata()`, `query_pv_metadata()`, `iter_pv_metadata()`, `delete_pv_metadata()`) plus the `PvMetadataQuery` (`Q`) criterion helpers -- `src/dp_python_lib/client/machine_config_client.py` - Machine configuration client covering both configurations (`save_configuration()`, `get_configuration()`, `query_configurations()`, `iter_configurations()`, `delete_configuration()`) and their temporal activations (`save_configuration_activation()`, `get_configuration_activation()`, `query_configuration_activations()`, `iter_configuration_activations()`, `delete_configuration_activation()`, `get_active_configurations()`). Includes the `ConfigurationQuery` (`C`) and `ConfigurationActivationQuery` (`CA`) criterion helpers. The shared time converters it used to own now live in `time_conversions.py`. Get/delete activation take a composite key (`client_activation_id` XOR `configuration_name`+`start_time`). Activation `end_time` is optional — omit it for an open-ended activation ("still in effect"); the field is then genuinely absent on the wire +- `src/dp_python_lib/client/machine_config_client.py` - Machine configuration client covering both configurations (`save_configuration()`, `get_configuration()`, `query_configurations()`, `iter_configurations()`, `delete_configuration()`) and their temporal activations (`save_configuration_activation()`, `get_configuration_activation()`, `query_configuration_activations()`, `iter_configuration_activations()`, `delete_configuration_activation()`, `get_active_configurations()`). Includes the `ConfigurationQuery` (`C`) and `ConfigurationActivationQuery` (`CA`) criterion helpers. The shared time converters it used to own now live in `time_conversions.py`. Get/delete activation take a composite key (`client_activation_id` XOR `configuration_name`+`start_time`). Activation `end_time` is optional — omit it for an open-ended activation ("still in effect"); the field is then genuinely absent on the wire. Read it back with the module-level `activation_is_open()` / `activation_end_time()` (#26), which work on an activation from any read path: reading `.endTime` directly on an open record silently yields a zero `Timestamp` (1970), and passing that back as `end_time=` in a re-save gets the save rejected. `activation_end_time()` returns `None` for an open record, so `end_time=activation_end_time(current)` is the correct carry-forward. Presence is the only test: an `endTime` present with value 0 is closed, not open - `src/dp_python_lib/client/sample_status_client.py` - Sample status client (`save_sample_statuses()`, `query_sample_statuses()`, `iter_sample_statuses()`, `iter_sample_statuses_stream()`, `delete_sample_statuses()`) plus the `SampleStatusColumn` / `SampleStatusFrame` construction classes. The `sampling_clock()` / `timestamp_list()` axis builders now live in `data_frame.py` (issue #6 Phase 2, once calculations frames became a second caller) and are re-exported here, so existing imports are unaffected. A status's identity key is `(pvName, timestamp, domain, layer)`; `delete_sample_statuses()` requires either `pv_names` or an explicit `all_pvs=True` opt-in for the destructive wildcard - `src/dp_python_lib/client/sample_status_conversions.py` - Per-sample expansion of query results (no optional extras required): `expand_data_timestamps()` (SamplingClock positions computed in **integer nanoseconds**, never float seconds — the exact-match contract depends on it), `bucket_to_rows()` / `buckets_to_rows()` / `iter_rows()` yielding `SampleStatusRow` objects with absent confidence/reason surfaced as `None` rather than fabricated `0.0`/`""` - `src/dp_python_lib/client/dataset_client.py` - DataSet client (`save_dataset()`, `get_dataset()`, `query_datasets()`, `iter_datasets()`, `delete_dataset()`, plus the `get_datasets(ids)` batch fetch that avoids the annotation-listing N+1) with the `DataSetQuery` (`DS`) criterion helpers and the `data_block()` builder. `data_block()` is the only place `begin < end` is checked — the server does not @@ -495,6 +495,8 @@ from dp_python_lib.client import ( SaveConfigurationActivationRequestParams, ConfigurationQuery as C, ConfigurationActivationQuery as CA, + activation_is_open, + activation_end_time, ) client = MldpClient() @@ -527,10 +529,12 @@ mc.get_configuration_activation(configuration_name="beamline-optics", start_time # query/iterate activations (raises RuntimeError on a page error) for a in mc.iter_configuration_activations([CA.configuration_name(["beamline-optics"])]): - print(a.clientActivationId) + ends_at = activation_end_time(a) # None while open; never read a.endTime directly + print(a.clientActivationId, ends_at.epochSeconds if ends_at is not None else "open") # what is active right now? (pass a timestamp for a historical instant) active = mc.get_active_configurations().configuration_activations +open_ended = [a for a in active if activation_is_open(a)] mc.delete_configuration_activation(client_activation_id="act-001") mc.delete_configuration("beamline-optics") @@ -539,6 +543,8 @@ mc.delete_configuration("beamline-optics") Notes: - `to_timestamp()` (also exported) is the shared time converter; naive datetimes raise `ValueError`. - Composite-key get/delete require exactly one key form (id XOR name+start_time); violations raise `ValueError`. +- Never read `activation.endTime` without a presence check: on an open-ended activation it is a zero `Timestamp` + (1970-01-01), not an error or `None`. Use `activation_is_open()` / `activation_end_time()`. - `ConfigurationQuery` (`C`) criteria: `name`/`category`/`tags`/`attributes`/`parent`. `ConfigurationActivationQuery` (`CA`) criteria: `timestamp`/`time_range`/`configuration_name`/`client_activation_id`/`category`/`tags`/`attributes`. Each helper raises `ValueError` on empty inputs. diff --git a/doc/cookbook/machine-configuration.md b/doc/cookbook/machine-configuration.md index 23071fd..29d375f 100644 --- a/doc/cookbook/machine-configuration.md +++ b/doc/cookbook/machine-configuration.md @@ -21,6 +21,8 @@ from dp_python_lib.client import ( ConfigurationQuery as C, ConfigurationActivationQuery as CA, to_timestamp, + activation_is_open, + activation_end_time, ) ``` @@ -180,7 +182,8 @@ machine_config.save_configuration_activation(SaveConfigurationActivationRequestP after its `start_time`. To close it, re-save the record with the same `client_activation_id` and a real `end_time` — see the next section. -To test whether a record you have read back is still open, check the field directly: +To test whether a record you have read back is still open, use `activation_is_open()`, and read +its end time with `activation_end_time()`, which returns `None` for an open record: ```python # cookbook:partial @@ -190,9 +193,13 @@ read = machine_config.get_configuration_activation(client_activation_id="act-ope activation = read.configuration_activation assert activation is not None -still_open = not activation.HasField("endTime") +still_open = activation_is_open(activation) # True +ends_at = activation_end_time(activation) # None while still open ``` +Do not read `activation.endTime` directly. On an open record it does not fail: it returns a zero +`Timestamp`, which is 1970-01-01, is truthy, and is not `None`, so no ordinary check catches it. + ## Closing one activation and opening the next When the machine changes configuration at time `t`, close the current interval at `t` and open @@ -242,7 +249,53 @@ Two things to get right: than a second, overlapping activation record. - **Copy every field forward.** `save_*` is full-replace, so omitting `description`, `tags`, or `attributes` erases them. Note that `start_time` accepts the `common.Timestamp` you read back - directly — no conversion needed. + directly — no conversion needed. For a re-save that is *not* closing the activation (a retag, + say), carry the end time forward with `end_time=activation_end_time(current)`, never + `end_time=current.endTime`. The helper passes `None` through, so an open activation stays open; + the raw field would send the 1970 value, and the server rejects the save because the end + precedes the start. + +### When you do not have the activation's id + +A live bridge often knows only the configuration it is switching *to*, not the id of the +activation it must close, or even which configuration that activation belongs to. Look it up +by what the server checks. Step 3 is rejected if the new interval overlaps any activation in +the same category, whatever that activation's configuration name. So ask for the activation in +that category that is in effect at the changeover: + +```python +# cookbook:partial +machine_config = client.annotation.machine_config +changeover = datetime(2026, 2, 2, 23, 0, tzinfo=timezone.utc) + +incoming = machine_config.get_configuration("mfx-production") +if incoming.result_status.is_error: + raise RuntimeError(incoming.result_status.message) +assert incoming.configuration is not None +category = incoming.configuration.category + +in_effect = list(machine_config.iter_configuration_activations([ + CA.category([category]), + CA.timestamp(changeover), # in effect at the changeover, open-ended ones included +])) +if len(in_effect) > 1: + raise RuntimeError(f"{len(in_effect)} activations in effect for category {category!r}; expected at most one") +current = in_effect[0] if in_effect else None # None: nothing to close, go to step 3 +``` + +Do not narrow this to open activations with `activation_is_open()`. An activation with a +scheduled end after the changeover blocks step 3 just as an open one does, and step 2 closes +either kind the same way, by setting `end_time` to the changeover. + +Zero results is normal: nothing in that category is in effect, so skip the close and go straight +to step 3. More than one should not happen, since the server rejects an activation overlapping +another in the same category. It can arise only if two saves race past that check, which is not +atomic. Raise rather than pick one: closing either leaves the other in effect and still +overlapping. + +An activation in the category that *starts* after the changeover is not returned, and step 3 +is still rejected if it overlaps one. That is a scheduled activation, and a real conflict for +a person to resolve, not something for the bridge to close. ### Late reports @@ -272,8 +325,8 @@ for activation in result.configuration_activations: ``` `get_active_configurations()` returns every activation whose interval covers that instant — -`startTime <= t` and `endTime > t`. Several may be active at once when they belong to different -categories. +`startTime <= t`, and either `endTime > t` or no `endTime` at all (an open-ended activation). +Several may be active at once when they belong to different categories. Called with no argument, it answers **"what is active right now"**: @@ -328,7 +381,8 @@ for activation in client.annotation.machine_config.iter_configuration_activation for activation in client.annotation.machine_config.iter_configuration_activations([ CA.configuration_name(["cxi-production"]), ]): - print(activation.startTime.epochSeconds, activation.endTime.epochSeconds) + ends_at = activation_end_time(activation) + print(activation.startTime.epochSeconds, ends_at.epochSeconds if ends_at is not None else "still in effect") ``` ### All the beam time an experiment received diff --git a/doc/release-notes/NEXT.md b/doc/release-notes/NEXT.md index dfaf6df..b0ab122 100644 --- a/doc/release-notes/NEXT.md +++ b/doc/release-notes/NEXT.md @@ -29,6 +29,7 @@ person cutting the release has any reason to re-read. - [Type checking in CI (#30)](#type-checking-in-ci-issue-30) - [Ready for typed gRPC stubs (#61)](#ready-for-typed-grpc-stubs-issue-61) - [Environment variables override the config file (#19)](#environment-variables-override-the-config-file-issue-19) +- [Detecting open-ended activations (#26)](#detecting-open-ended-activations-issue-26) - [Cutting the release](#cutting-the-release) --- @@ -114,6 +115,31 @@ Two smaller consequences of the same fix: See [#19](https://github.com/osprey-dcs/dp-python-lib/issues/19) and the configuration priority section of [`doc/cookbook/connecting.md`](https://github.com/osprey-dcs/dp-python-lib/blob/main/doc/cookbook/connecting.md#configuration-priority). +## Detecting open-ended activations (Issue #26) + +Two new helpers, exported from `dp_python_lib.client`, read an open-ended configuration activation +back (one saved without `end_time`, meaning "still in effect"): + +- **`activation_is_open(activation)`** reports whether an activation has no end time. +- **`activation_end_time(activation)`** returns the end time as a `common.Timestamp`, or `None` + for an open activation. + +Both work on an activation from any read path: get, query, iterate, or +`get_active_configurations()`. They exist because reading `activation.endTime` directly on an open +record does not fail. It returns a zero `Timestamp`, 1970-01-01, which is truthy and not `None`, +so no ordinary check notices. Passing that value back as `end_time=` in a re-save gets the save +rejected for an end time before the start; `end_time=activation_end_time(current)` carries it +forward correctly. + +This is additive; nothing needs to change on upgrade. **If you copied the "Every interval a +configuration was in effect" recipe** from +[`doc/cookbook/machine-configuration.md`](https://github.com/osprey-dcs/dp-python-lib/blob/main/doc/cookbook/machine-configuration.md), +it printed `0` as the end of an open interval; the recipe now uses `activation_end_time()`. The +cookbook also gains a recipe for finding the activation to close at a changeover when you do +not have its id. + +See [#26](https://github.com/osprey-dcs/dp-python-lib/issues/26). + ## Installing ```bash diff --git a/plan/tickets/26/plan.md b/plan/tickets/26/plan.md index 9bb57ef..73b7a31 100644 --- a/plan/tickets/26/plan.md +++ b/plan/tickets/26/plan.md @@ -194,6 +194,17 @@ format. two open records for one name can exist only through the race that check's own comment accepts as a v1 limitation: the check and the write are not atomic. The recipe raises rather than choosing one. Closing either would leave the other open and still overlapping. + + *Revised in review of the implementation PR (#65), 2026-09-26.* The shipped recipe does not filter by + configuration name and `activation_is_open()`. A name query pages through the configuration's whole + activation history on every changeover. It also misses the case that actually gets step 3 rejected: + `overlapExists()` checks by category as well as by name, and a bridge switching to a new configuration may + not know the name of the one in effect. The recipe instead reads the incoming configuration's category and + queries `CA.category([category])` with `CA.timestamp(changeover)`. The server's `activationContainsInstantFilter` + counts an absent `endTime` as in effect. It deliberately does not narrow to open records: an activation with + a scheduled end after the changeover blocks step 3 just as an open one does, and step 2 closes either kind. + The zero / one / more-than-one handling is unchanged. An activation starting after the changeover is not + returned, and the recipe leaves that conflict to the server's rejection. - "Copy every field forward" (the list after the closing recipe): add `end_time` to it. A re-save that is not closing the activation carries the end time forward with `end_time=activation_end_time(current)`, never `current.endTime`. Give the one-sentence reason (T4, last bullet). diff --git a/src/dp_python_lib/client/__init__.py b/src/dp_python_lib/client/__init__.py index 1d20093..5f0b8b9 100644 --- a/src/dp_python_lib/client/__init__.py +++ b/src/dp_python_lib/client/__init__.py @@ -67,6 +67,8 @@ SaveConfigurationActivationRequestParams, SaveConfigurationApiResult, SaveConfigurationRequestParams, + activation_end_time, + activation_is_open, ) from dp_python_lib.client.mldp_client import MldpClient from dp_python_lib.client.pv_metadata_client import ( @@ -163,6 +165,8 @@ "SaveSampleStatusesApiResult", "SaveSampleStatusesRequestParams", "TimestampInput", + "activation_end_time", + "activation_is_open", "bool_column", "calculations", "calculations_source", diff --git a/src/dp_python_lib/client/machine_config_client.py b/src/dp_python_lib/client/machine_config_client.py index fdb8856..c59e83c 100644 --- a/src/dp_python_lib/client/machine_config_client.py +++ b/src/dp_python_lib/client/machine_config_client.py @@ -268,6 +268,45 @@ def attributes( return criterion +# Reading an open-ended activation back (issue #26). The server leaves endTime genuinely absent on every read path +# (get, query, and getActiveConfigurations share one converter), so presence is the whole test. These take a +# ConfigurationActivation rather than a result object so they cover all four read paths with one definition. + + +def activation_is_open(activation: common_pb2.ConfigurationActivation) -> bool: + """ + Reports whether an activation is open-ended, meaning its configuration is still in effect. + + An open-ended activation has no endTime at all. Test it with this (or activation_end_time()) rather than by + reading activation.endTime: on an open record that read does not fail, but returns a zero Timestamp + (1970-01-01), which is truthy, is not None, and converts to 0 epoch nanoseconds. + + An endTime that is present with value zero is a real end time, not an open one; only absence means open. + + :param activation: An activation from any read path: get, query, iterate, or get_active_configurations(). + :return: True if the activation has no endTime. + """ + return not activation.HasField("endTime") + + +def activation_end_time(activation: common_pb2.ConfigurationActivation) -> common_pb2.Timestamp | None: + """ + Returns an activation's endTime, or None if the activation is open-ended. + + Use this instead of reading activation.endTime directly, which on an open record silently yields a zero + Timestamp (1970-01-01) rather than signalling absence. + + The result is the field's own Timestamp, which end_time= accepts as-is, so this is also the correct way to + carry the end time forward in a full-replace re-save: end_time=activation_end_time(current) keeps an open + activation open, whereas end_time=current.endTime would send the 1970 value and be rejected by the server as + ending before it starts. + + :param activation: An activation from any read path: get, query, iterate, or get_active_configurations(). + :return: The activation's endTime (the message's own sub-message, not a copy), or None if it is open-ended. + """ + return None if activation_is_open(activation) else activation.endTime + + class SaveConfigurationRequestParams: """ Encapsulates client parameters for a call to the saveConfiguration() API method. @@ -438,6 +477,7 @@ def __init__( :param start_time: Start of the activation interval (tz-aware datetime, epoch seconds, or common.Timestamp). :param end_time: End of the activation interval (tz-aware datetime, epoch seconds, or common.Timestamp). Omit or pass None for an open-ended activation, meaning the configuration is still in effect. + Read it back with activation_is_open() / activation_end_time(), not by reading endTime directly. :param client_activation_id: Optional client-supplied identifier for the activation. :param description: Human-readable description of the activation. :param tags: List of tags (keywords) describing the activation. diff --git a/tests/integration/test_machine_config_client_integration.py b/tests/integration/test_machine_config_client_integration.py index 588adb8..1d9019c 100644 --- a/tests/integration/test_machine_config_client_integration.py +++ b/tests/integration/test_machine_config_client_integration.py @@ -15,6 +15,8 @@ ConfigurationQuery, SaveConfigurationActivationRequestParams, SaveConfigurationRequestParams, + activation_end_time, + activation_is_open, ) from dp_python_lib.client.mldp_client import MldpClient @@ -359,6 +361,9 @@ def test_open_ended_activation_round_trip(self): activation.HasField("endTime"), "an open-ended activation must round-trip with endTime absent, not defaulted to zero", ) + # The helpers must agree with the raw check above, which is the one that verifies the wire. + self.assertTrue(activation_is_open(activation)) + self.assertIsNone(activation_end_time(activation)) self.logger.info("Open-ended activation round-tripped with endTime absent") # --- an open-ended activation is active at any instant at or after start_time --- @@ -401,6 +406,10 @@ def test_open_ended_activation_round_trip(self): self.assertIsNotNone(closed, "getConfigurationActivation should return the closed record") self.assertTrue(closed.HasField("endTime"), "closing the activation should set endTime") self.assertEqual(closed.endTime.epochSeconds, end_time) + self.assertFalse(activation_is_open(closed)) + closed_end = activation_end_time(closed) + self.assertIsNotNone(closed_end) + self.assertEqual(closed_end.epochSeconds, end_time) self.logger.info("Closed the open-ended activation at %d", end_time) # --- once closed, the far-future probe no longer sees it --- diff --git a/tests/unit/test_machine_config_activation_client.py b/tests/unit/test_machine_config_activation_client.py index 32fb111..33fa0ee 100644 --- a/tests/unit/test_machine_config_activation_client.py +++ b/tests/unit/test_machine_config_activation_client.py @@ -16,6 +16,8 @@ MachineConfigClient, QueryConfigurationActivationsApiResult, SaveConfigurationActivationRequestParams, + activation_end_time, + activation_is_open, ) from dp_python_lib.grpc import annotation_pb2, common_pb2 @@ -194,6 +196,89 @@ def test_end_time_zero_is_not_treated_as_absent(self): self.assertEqual(request.endTime.epochSeconds, 0) +class TestActivationOpenEndedHelpers(unittest.TestCase): + """activation_is_open() / activation_end_time(): reading an open-ended activation back (#26).""" + + @staticmethod + def _activation(activation_id="act-1", end_seconds=None): + activation = common_pb2.ConfigurationActivation( + configurationName="cfg-1", + clientActivationId=activation_id, + startTime=common_pb2.Timestamp(epochSeconds=100), + ) + if end_seconds is not None: + activation.endTime.CopyFrom(common_pb2.Timestamp(epochSeconds=end_seconds)) + return activation + + def test_open_ended(self): + activation = self._activation() + self.assertTrue(activation_is_open(activation)) + self.assertIsNone(activation_end_time(activation)) + + def test_bounded(self): + activation = self._activation(end_seconds=200) + self.assertFalse(activation_is_open(activation)) + self.assertEqual(activation_end_time(activation), common_pb2.Timestamp(epochSeconds=200)) + + def test_epoch_zero_end_time_is_not_open(self): + """Presence is the only test: an endTime present with value 0 is a real end time, not an open one.""" + activation = self._activation(end_seconds=0) + self.assertFalse(activation_is_open(activation)) + end_time = activation_end_time(activation) + self.assertIsNotNone(end_time) + self.assertEqual(end_time.epochSeconds, 0) + + def test_reading_end_time_does_not_close_an_open_record(self): + """Reading .endTime on an open record yields a zero Timestamp but must not set presence.""" + activation = self._activation() + self.assertEqual(activation.endTime.epochSeconds, 0) # the silent 1970 value the helpers exist to avoid + self.assertTrue(activation_is_open(activation)) + self.assertIsNone(activation_end_time(activation)) + + def test_end_time_round_trips_into_a_resave(self): + client = MachineConfigClient(Mock()) + activation = self._activation(end_seconds=200) + params = SaveConfigurationActivationRequestParams( + configuration_name=activation.configurationName, + start_time=activation.startTime, + end_time=activation_end_time(activation), + ) + request = client._build_save_configuration_activation_request(params) + self.assertTrue(request.HasField("endTime")) + self.assertEqual(request.endTime.epochSeconds, 200) + + def test_carrying_an_open_record_forward_keeps_it_open(self): + """end_time=activation_end_time(open) re-saves as open; end_time=open.endTime would send 1970.""" + client = MachineConfigClient(Mock()) + activation = self._activation() + params = SaveConfigurationActivationRequestParams( + configuration_name=activation.configurationName, + start_time=activation.startTime, + end_time=activation_end_time(activation), + ) + request = client._build_save_configuration_activation_request(params) + self.assertFalse(request.HasField("endTime")) + + def test_filters_a_query_page_to_the_open_records(self): + """The query-path case: find the open activation(s) among a query page's mixed results.""" + response = annotation_pb2.QueryConfigurationActivationsResponse() + response.queryConfigurationActivationsResult.configurationActivations.extend( + [ + self._activation("closed-1", end_seconds=200), + self._activation("open-1"), + self._activation("closed-2", end_seconds=0), + ] + ) + client = MachineConfigClient(Mock()) + client._stub = Mock() + client._stub.queryConfigurationActivations.return_value = response + + result = client._send_query_configuration_activations(annotation_pb2.QueryConfigurationActivationsRequest()) + self.assertFalse(result.result_status.is_error) + open_ids = [a.clientActivationId for a in result.configuration_activations if activation_is_open(a)] + self.assertEqual(open_ids, ["open-1"]) + + class TestActivationKeyValidation(unittest.TestCase): def setUp(self): self.client = MachineConfigClient(Mock())