Rodrigo Tobar (50492841) at 16 Mar 22:47

Rodrigo Tobar (a2bcb17a) at 16 Mar 22:47

... and 14 more commits

This MR is part 2 of !525. It builds on the fact that Helm values for the SDP release under test are now fully available and it proceeds to remove redundant make variables, and their associated exported environment variables, further reducing the complexity of running tests against any given SDP deployment.

I would suggest to go through commits one by one to see individual changes, making digestion a bit easier. They:

• Clean minor things up: a docstring is being adjusted, a duplicate function is being removed
• Change the context feature to fill its pvc_name and namespace_sdp fields from Helm values, not env vars
• Skip resource management tests based on SDP release under test rather than an environment variable
• Issue ReleaseAllResources command instead of ReleaseResources at Subarray tear down, the latter didn't consistently reach EMPTY since some tests change the resources in the AssignResources command, but ReleaseResources used a fixed payload.
• Rely on SDP LMC's timeout for long obsState transitions instead of managing those in the test code
• Remove CLUSTER_DOMAIN override on CI
• Remove TEST_TIMEOUT_{ASSIGN_RES,CONFIGURE} variables
  • This and the other "Remove ..." commits also make sure that the corresponding Helm values file specify the correct value for each target deployment as needed
  • These variables in particular were meant to be used by the tests, but they were never effectively communicated to them, so they were a red herring
• Remove unnecessary DATA_PRODUCT_PVC_NAME variables
• Remove unnecessary FF_RESOURCE_MANAGEMENT variables
• Remove unnecessary KUBE_NAMESPACE_SDP env variable
  • Note that this only removes the environment variable being exported from make, not the make variable itself
• Remove unnecessary TIMEOUT_{ASSIGN_RESOURCES,CONFIGURE} make variables
• Remove unnecessary NSUBARRAY variable
• Reorganise environment variable listing exported by make

Separately (for a later MR) I have changes that remove the need to specify KUBE_NAMESPACE_SDP altogether, letting the ska-sdp chart (or the ska-sdp-integration chart maybe?) create the processing namespace instead of doing this through the Makefile. These seem to work and I was planning to originally deliver them in this MR, but now I'm unsure what the sentiment towards such a change would be, so I held them back and could submit them separately.

Checklist: Required Before Merge

• Code reviewed
• Integration tests added or updated
• CI pipeline passes
• Added unit tests
• Added docstrings and appropriate comments to code
• Updated documentation (if appropriate)
• Updated CHANGELOG.rst (if appropriate)
• Updated CONTRIBUTORS and LICENCE (if appropriate)

Notes:

Please ensure that all items above are checked before merging this MR.

done now!

Would you consider adding something similar to

@property
def _validated_eb_state(self) -> dict:
    return TypeAdapter(EbState).dump_python(self._eb_state, mode="json")

to avoid sprinkling TypeAdapters in too many places? Similar for self._subarray.

I quite liked how this simplified the rest of the code a bit; the fact that you now have to duplicate the component name across all calling sites doesn't seem beneficial?

In the new world of ADR-111 this should be

            scans=[EbStateScan(scan_id=0, scan_type=scan_type_id, start_time=datetime.now())],

further hinting that EbStateScan.scan_id should always be int rather than int | None.

Given how only the command field from subarray_values is only ever considered now (and is the only field that is ever passed through that dictionary actually) it might be a good idea to accept subarray_command directly as a parameter instead of accepting a dictionary from which we only ever extract command.

Rodrigo Tobar (fc81776e) at 16 Mar 15:11

... and 1 more commit

Haven't finished, just need to go a bit more carefully through emulated_sdp.py yet, but hopefully there's enough feedback here to unblock you.

This is a very unusual spelling for Subarray, at least in radioastronomy. Not saying that is necessarily incorrect, but I suspect it would raise more than one eyebrow.

Once we start reading the scans table we should expect the scan id of each entry to always be set.

Beware that execution_block.state(....).get() returns EB | None. The next statement checks for None and logs an error (and on the caller side there are also checks for None, which are compatible with the return type of tis method). With this new implementation an exception would be raised instead.

I'd suggest you changed this to TypeAdapter(EbState | None) to keep the changes to a minimum, otherwise the rest of the code using this method would need to be adapted to except an exception instead of checking for None.

I also wanted to clarify what seems like an incorrect understanding of Thread.join: in the MR description you say

... where joining of the monitoring thread (propagates exceptions to raise on the main thread) ...

That is not how Thread.join works: it simply blocks, and any unhandled exceptions in the thread are not propagated (they are instead handled by the the threading.excepthook, which usually prints them). So while adding a logging statement in these cases is definitely better, it doesn't change the behaviour of the receiver, or the fact that exceptions in this monitoring thread are not propagated at join time.

This was like this originally, then I changed it to split the config DB client (a single one per session, hence making tests faster to run) from any per-test cleanup (see 8306a6a1), which I feel is more correct (it clearly isolates each step's setup/teardown), but also more error prone (an incorrect cleanup will affect subsequent tests).

While I'm not strongly opposed to going back to doing a full cleanup here, it'd be good to also understand which cleanup didn't happen correctly to fix that too.

The message says it's stopping monitoring, but with the exception now being caught the monitoring will now actually continue, potentially getting stuck in a logging loop. Did you mean to add a break/return after the logging statement?

Rodrigo Tobar (64db5fe6) at 16 Mar 04:32

(Pending the schema updates in DPPT-1175)

This MR adds scan_intent and integration_time attributes to the ScanType class, and populates them from the new AssignResources schema version 2.0. integration_time is given a default value of 1.0; this is the default value currently used elsewhere for writing measurement sets when a integration time (interval) is not provided.

parse_scantypes_from_assignres has also been updated to accept assignres interface versions (0.4,0.5,1.0,1.1,1.2,2.0), rather than just 0.4 and 1.0.

very, very minor: maybe intent_{i} or scan_intent_{i}?

This MR adds integration_time and scan_intent to the SCAN_TYPE table, and time_centroids argument to process_visibility to pass such a table through the process_visibility RPC, allowing these values to be written to the plasma store.

A new version of ska-sdp-dal-schemas is also being released as part of this MR. .make is updated.