Skip to content

feat: Support plan *args and **kwargs - #1662

Open
oliwenmandiamond wants to merge 36 commits into
mainfrom
support_star_args
Open

oliwenmandiamond wants to merge 36 commits into
mainfrom
support_star_args

Conversation

@oliwenmandiamond

@oliwenmandiamond oliwenmandiamond commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1450.

API changes
Plan parameters: Added support for passing both positional (*args) and keyword (**kwargs) arguments to plans via the API. This is using TaskParametersV2 and TaskRequestV2

Backwards compatibility: Existing requests that pass plan parameters as a simple JSON object of keyword arguments remain supported, TaskParametersV1 and TaskRequestV1. They are automatically converted to the new V2 before plans are ran.

@oliwenmandiamond oliwenmandiamond changed the title Support *args Support plan *args Sep 9, 2026
@oliwenmandiamond oliwenmandiamond changed the title Support plan *args feat: Support plan *args Sep 9, 2026
@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.38%. Comparing base (f9af725) to head (5ff57de).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1662      +/-   ##
==========================================
+ Coverage   96.36%   96.38%   +0.02%     
==========================================
  Files          49       49              
  Lines        3686     3739      +53     
==========================================
+ Hits         3552     3604      +52     
- Misses        134      135       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@oliwenmandiamond
oliwenmandiamond marked this pull request as ready for review October 14, 2026 16:04
@oliwenmandiamond
oliwenmandiamond requested a review from a team as a code owner October 14, 2026 16:05
@oliwenmandiamond

Copy link
Copy Markdown
Contributor Author

@DiamondLightSource/developers-daq-core this is ready for review. It should be fully backwards compatible so shouldn't break anything downstream. However, we should depreciate the V1 Task and Parameters and then remove after X amount of time.

@oliwenmandiamond oliwenmandiamond changed the title feat: Support plan *args feat: Support plan *args and **kwargs Oct 15, 2026

@fajinyuan fajinyuan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments or questions - I only partly went through the changes now. but would like to submit them for today, and continue the review next time - hopefully tomorrow!

Comment thread docs/reference/openapi.yaml
Comment thread src/blueapi/cli/cli.py Outdated
Comment thread src/blueapi/cli/cli.py
Comment thread src/blueapi/cli/cli.py

@abbiemery abbiemery left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the large size of the change I don't have time to look at this today, but I shall next week. I would prefer it isn't merged until I have had that chance.

@oliwenmandiamond

oliwenmandiamond commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Given the large size of the change I don't have time to look at this today, but I shall next week. I would prefer it isn't merged until I have had that chance.

Thanks, fine by me. The change itself is not that large about 300 lines, but I've gone out of my way to make the unit tests and system tests to be as comprehensive as possible due to the API change as we now have different combinations to test as supporting old parameters and new and unfortunately this has made the PR grow larger than I wanted.

Comment thread src/blueapi/service/main.py Outdated
request: Request,
response: Response,
task_request: Annotated[TaskRequest, Body(..., examples=[example_task_request])],
task_request: Annotated[TaskRequestV2, Body(..., examples=[example_task_request])],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right in thinking that this will only accept request bodies that desearlize to a TaskRequestV2? Eg. the request below yields an "Extra inputs are not permitted" error:

curl -X 'POST' \
  'http://localhost:4180/api/v1/tasks' \
  -H 'accept: */*' \
  -H 'Content-Type: application/json' \
  -d '{
  "name": "sleep",
  "params": {
    "time": 1.0
  },
  "instrument_session": "cm12345-1"
}'

If this is the case (and not my user error) I think it breaks backwards compatibility, so will need to either be reserved for Blueapi 2.0, or re-worked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes good spot. This is easily fixed by using TaskRequest = TaskRequestV2 | TaskRequestV1 for the annotation. Fixed now.

Again, I don't think there are existing tests covering this otherwise the CI would of been unhappy?

Comment thread src/blueapi/worker/task.py Outdated
func = ctx.plan_functions[self.name]

sig = signature(func)
bound = sig.bind(*self.params.args, **self.params.kwargs)

@dan-fernandes dan-fernandes Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running a plan without the correct parameters (blueapi -c tests/system_tests/config.yaml controller run sleep -i cm12345-1) yields a stack trace (for both server and client) rather than a neat error like below:

$ blueapi -c tests/system_tests/config.yaml controller run sleep -i cm12345-1
Authorization required, but no authorization protocol specified

Error: Incorrect parameters supplied
    Missing value for 'time'

I think this is because sig.bind raises a TypeError rather than the ValidationError which submit_task is expecting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, I tried adding this

        try:
            bound = sig.bind(*self.params.args, **self.params.kwargs)
        except TypeError as e:
            raise ValidationError() from e

But that didn't fix it. Will have to investigate this more next week We should probably add a new test for this too because the existing would of caught this otherwise

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay managed to fix it. I switched to use sig.bind_partial so that it doesn't raise error and error is instead raised by pydantic instead, thus restoring previous behaviour. Added test_task-worker.py:test_do_task_with_invalid_parameters_raises_validation_error that tests for this.



SUBMIT_REQUEST = {
SUBMIT_REQUEST_V1 = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're continuing to support requests that only contains Keyword arguments, I think the API needs to have TaskParameterV1 tests to promote backwards compatability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is outdated now, added TaskParametersV1 to test here as "params": {"one": "two"},

@@ -253,6 +255,8 @@ def get_active_task(self) -> WorkerTask:
return self._request_and_deserialize("/worker/task", WorkerTask)

def create_task(self, task: TaskRequest) -> TaskResponse:

@dan-fernandes dan-fernandes Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is used by externally, but Within Blueapi .create_task is only called with a TaskRequestV2. Could this signature be changed to expect TaskRequest2 and not bother with conversion?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to support V2 only breaks tests

test_rest.py:test_create_task_serialization

fails

AssertionError: expected call not found.
Expected: _request_and_deserialize('/tasks', <class 'blueapi.service.model.TaskResponse'>, method='POST', get_exception=<function _create_task_exceptions at 0x7fe3cd8726c0>, data={'name': 'demo', 'instrument_session': 'cm12345-1', 'params': {'args': [], 'kwargs': {'devices': ['foo']}}})
  Actual: _request_and_deserialize('/tasks', <class 'blueapi.service.model.TaskResponse'>, method='POST', get_exception=<function _create_task_exceptions at 0x7fe3cd8726c0>, data={'name': 'demo', 'params': {'devices': ['foo']}, 'instrument_session': 'cm12345-1'})

However, this might be okay because it's because I make it test for V1 and V2. So if you think this should be V2 only, we can make that change

)


class TaskRequestV2(BlueapiBaseModel):

@dan-fernandes dan-fernandes Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With your changes, the REST API only accepts TaskRequestV2 (I think?) and the python client automatically converts to TaskRequestV2. As such is there a need for two different internal representations of a task?

This may need to be re-visited if the REST API changes are re-worked. But even if the API accepts both, if possible it would be handy to convert at the top level, rather than have an ambiguous TaskRequest with multiple possible fields throughout the service.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python client only ever needs to submit things using V2 as you trying to use python so need access *args and **kwargs for plans. I've updated REST API so it should accept V1 and V2 now.

So there's two ways I could of done this. There was TaskRequest which accepts nested structure of kwargs and args, or I could of added an additional parameter at top level. I think the later was much more messy option so updating the model and being nested was cleaner and lesser of two evils in my opinion. I think that has answered your question? (let me if it hasn't...)

Comment thread src/blueapi/config.py

#: API version to publish in OpenAPI schema
REST_API_VERSION: ClassVar[str] = "1.5.0"
REST_API_VERSION: ClassVar[str] = "1.6.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to double check that the API isn't broken before confirming this API version

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I updated assuming everything was okay and to get all tests to pass. If we agree it should 2.0.0 that's fine, just let me know

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plans do not function with *args

5 participants