feat(operators): add script operation support for DiscoverySpace#956
Open
michael-johnston wants to merge 2 commits into
Open
feat(operators): add script operation support for DiscoverySpace#956michael-johnston wants to merge 2 commits into
michael-johnston wants to merge 2 commits into
Conversation
This allows dynamic operations to be recorded on discoveryspaces i.e. without having to install a separate operator plugin
Comment on lines
+909
to
+911
| name: Human-readable script name stored in the operation configuration. | ||
| description: Optional description for the operation metadata. | ||
| metadata: Optional extra metadata fields merged into ConfigurationMetadata. |
Member
There was a problem hiding this comment.
Wouldn't it be clearer to have a ConfigurationMetadata parameter instead of these three? When I read name I assumed it would've been basically the operation identifier, which felt weird to me but that's what I assumed
Member
Author
There was a problem hiding this comment.
🤷 no real opinion :-)
Comment on lines
+341
to
+350
| mock_store = MagicMock() | ||
| pfas_space._metadataStore = mock_store | ||
|
|
||
| with pfas_space.operation_context( | ||
| name="test-script", | ||
| description="Script operation for testing", | ||
| metadata={"labels": {"source": "test"}}, | ||
| ) as operation_id: | ||
| assert operation_id.startswith("operation-script-test-script-") | ||
| assert operation_id in pfas_space._verified_operation_ids |
Member
There was a problem hiding this comment.
Why are these things mocked?
Comment on lines
+382
to
+393
| mock_store = MagicMock() | ||
| pfas_space._metadataStore = mock_store | ||
|
|
||
| with ( | ||
| pytest.raises(RuntimeError, match="boom"), | ||
| pfas_space.operation_context(name="failing-script"), | ||
| ): | ||
| raise RuntimeError("boom") | ||
|
|
||
| finished_operation = mock_store.updateResource.call_args_list[-1].args[0] | ||
| assert finished_operation.status[-1].event == OperationResourceEventEnum.FINISHED | ||
| assert finished_operation.status[-1].exit_state == OperationExitStateEnum.FAIL |
Member
There was a problem hiding this comment.
I assume this serves to test a case where there's an exception after the operation id is returned. First of all, it looks a bit weird with the "boom", so I would maybe change that to be something like "exception during execution"
We also don't need to mock the store, right?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds first-class support for inline script operations on DiscoverySpace instances, so custom sampling code can store measurements under a proper OperationResource without registering an operator plugin or going through orchestrate().
Use
Combined with #169 you can now do the following to iterate over a space, make measurements synchronously, store them without ray objects.
Motivation
Developers want to use DiscoverySpace for sampling and result storage from custom scripts or lightweight inline operator logic, perhaps inside an existing operator. Previously this required a registered explore operator in order to create the required OperationResource to associate measurements to (measurements stored without a linked OperationResource were invisible to most ADO query paths.)