improve error messages when running plan without parameters

[stan-dot]

closes #206

[codecov]

Codecov Report

Attention: Patch coverage is 57.14286% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 89.04%. Comparing base (4d52871) to head (e754e6b).
Report is 3 commits behind head on main.

❗ Current head e754e6b differs from pull request most recent head e8fd356. Consider uploading reports for the commit e8fd356 to get more accurate results

Files	Patch %	Lines
src/blueapi/cli/rest.py	18.18%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #385      +/-   ##
==========================================
- Coverage   89.38%   89.04%   -0.35%     
==========================================
  Files          43       43              
  Lines        1800     1817      +17     
==========================================
+ Hits         1609     1618       +9     
- Misses        191      199       +8     

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

[stan-dot]

one error in the test_cli.py. the test must be updated to first expect the helper call to the API

https://github.com/DiamondLightSource/blueapi/actions/runs/8192137690/job/22402991512?pr=385

[joeshannon]

I haven't tested actually using it yet but will aim to tomorrow.

Two of the tests are currently failing though, is this expected?

[stan-dot]

I haven't tested actually using it yet but will aim to tomorrow.

Two of the tests are currently failing though, is this expected?

re: failing tests - that's due to the now changed order and complexity of requests. one way is to change the mockrequests. _request_and_deserialize method is kind of a hammer, not a scalpel. I mention more in the issue thread #206 (comment)

[stan-dot]

fwiw the REST part of CLI has create_task while the server has submit_task. should this naming be consistent? @callumforrester

[joeshannon]

It's been quite a while since the issue was opened and things have changed quite a bit now.

The approach used looks reasonable, and does prevent the very nasty server/pool errors, however running blueapi controller run scan with this PR gives:

Traceback (most recent call last):
  File "/scratch/30day_tmp/delete24apr/blueapi/venv/bin/blueapi", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/scratch/30day_tmp/delete24apr/blueapi/venv/lib/python3.11/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/30day_tmp/delete24apr/blueapi/venv/lib/python3.11/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/scratch/30day_tmp/delete24apr/blueapi/venv/lib/python3.11/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/30day_tmp/delete24apr/blueapi/venv/lib/python3.11/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/30day_tmp/delete24apr/blueapi/venv/lib/python3.11/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/30day_tmp/delete24apr/blueapi/venv/lib/python3.11/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/30day_tmp/delete24apr/blueapi/src/blueapi/cli/cli.py", line 108, in wrapper
    func(*args, **kwargs)
  File "/scratch/30day_tmp/delete24apr/blueapi/venv/lib/python3.11/site-packages/click/decorators.py", line 38, in new_func
    return f(get_current_context().obj, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/30day_tmp/delete24apr/blueapi/src/blueapi/cli/cli.py", line 188, in run_plan
    task = Task(name=name, params=parsed_params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for Task
params
  value is not a valid dict (type=type_error.dict)

which doesn't seem to be an improvement over the original error reported in the issue.

[stan-dot]

in the CLI file we don't have access to the raw http response as that's in the request_and_deserialize file.

[stan-dot]

run fails

what should we return with Handler? I'm not sure

[stan-dot]

request_and_deserialize

that is a too optmistic a function, does not provide natural thin error handling from the http to cli layer

[stan-dot]

suggested plan of action - hard code the request into this specific cli command - without using the request_and_deserialize

[callumforrester]

@stan-dot You can do what you like within the RestClient class, but I don't think the HTTP response should be exposed outside

[stan-dot]

the thing is that right now _request_and_deserialize is defined to necessarily return a TargetType. but this function by name handles 2 things that do not necessarily follow one another. so this function if wants to join those 2 things will necessarily have handling of a rejected request. But we don't want to terminate the operation there. we want to provide output in the cli.

we can throw the error, but then the function does not return normally.

one alternative is to change the return tpe from 'T' to tuple[T |None, E| None].
the second alternative is to just throw an error in the RestClient and hope that the cli will catch it.

NOTE: if auto-client gen succeeds then this is a moot question

[stan-dot]

that's a better serverside message but we don't want a 5** message but a 4** one. on it

blueapi controller run scan '{"tim":5}'
('server error with this message: Response failed with text: Internal Server '
'Error,\n'
' with error code: 500\n'
' which corresponds to Internal Server Error ')

is the behavior

[stan-dot]

how is this supposed to handle errors ???? there is a fundamental architectural void in the design of how it is now.

suggestion: do client autogen issue first #364

[stan-dot]

doing this fresh in #206 issue