619 move to pyright #632

stan-dot · 2024-09-11T13:55:51Z

points that need addressing

sim_motor types issues - might ignore but alternatively just delete
task worker
- state is also problematic
- 'run', the part calling the RunEngine with the hooks is unclear

stan-dot · 2024-10-04T14:38:05Z

@callumforrester please let me know your thoughts

callumforrester · 2024-10-11T07:54:30Z

@stan-dot I like where this is going, thanks

stan-dot · 2024-10-11T08:00:07Z

ok will try to finish this today

stan-dot · 2024-10-11T09:46:52Z

simmotor has float and has that in the tests as well - but the Ophyd type is this: value : object, optional The initial value. Default is 0. which pyright reads as int.

adding an ignore line pending ophyd choosing a side here

stan-dot · 2024-10-11T09:49:47Z

task_worker complains - because run_engine is not typed 😢

stan-dot · 2024-10-11T09:55:26Z

DeviceStatus in devices.py complains that the DeviceStatus (imported from ophyd ❗ ) does not support the bluesky Status protocol

stan-dot · 2024-10-11T09:59:16Z

still one issue with the stomp instantiation from the CLI - what's your opinion @callumforrester ?

stan-dot · 2024-10-14T08:56:52Z

link to the error:

https://github.com/DiamondLightSource/blueapi/actions/runs/11290324342/job/31401954874?pr=632

src/blueapi/service/interface.py

src/blueapi/service/runner.py

src/blueapi/startup/simmotor.py

src/blueapi/utils/__init__.py

callumforrester · 2024-10-14T10:28:32Z

src/blueapi/worker/task_worker.py

@@ -95,7 +95,7 @@ def __init__(

        self._tasks = {}

-        self._state = WorkerState.from_bluesky_state(ctx.run_engine.state)
+        self._state = WorkerState.from_bluesky_state(ctx.run_engine.state)  # type: ignore


This seems fixable by setting the return type of from_bluesky_state, possibly including a cast?

will look into it

callumforrester · 2024-10-14T10:31:09Z

src/blueapi/worker/task_worker.py

@@ -181,8 +182,8 @@ def mark_task_as_started(event: WorkerEvent, _: str | None) -> None:
                task_started.set()

        LOGGER.info(f"Submitting: {trackable_task}")
+        sub = self.worker_events.subscribe(mark_task_as_started)


Should: Moving this out of the try block means a network error could potentially bring down the worker subprocess

from the first look on it, to add a callback on a potential event is not a network event.

it is only the following line:

self._task_channel.put_nowait(trackable_task)

that does something that looks like network logic

That's not network logic, that's adding to an internal queue. However, it is not just network logic you need to worry about: Anything that can throw an exception should go inside the try block, since we don't want the subprocess to crash and leave the main process spinning.

from the first look on it, to add a callback on a potential event is not a network event.

as I expected. if the first look was wrong, it means that the variable and file naming is not descriptive enough...

why adding to an internal queue would fail?

I added a comment explaining that part.

Still I think the discussion got sidetracked a bit as

sub = self.worker_events.subscribe(mark_task_as_started)

is not the put_nowait line

You're right, I don't think that line could cause a network error, but it could still raise an exception, so why was it moved out of the try block?

because it's just adding a callback

So if anything in _cycle raises an uncaught error it will kill the worker thread and blueapi will need a manual restart, which is why I prefer to play it safe and just catch and log any exception in the cycle loop. That code won't currently raise an exception, but it could be modified in the future such that it does, and the person modifying it won't necessarily know about this line. Please put it back.

ok, I'll include information from your comment for clarity

tests/unit_tests/worker/devices.py

callumforrester · 2024-10-14T10:35:03Z

@stan-dot: Maybe we should make the password a secret in a separate PR, in order to deal with the errors. It is not essential to adopting pyright.

stan-dot · 2024-10-14T12:56:11Z

I'd agree with you, @callumforrester had this difference was between a 200 lines PR and 1k lines PR. it's 100 +- lines at the moment so easy to review. :( The errors are from pyright, so it makes sense to make this as a change

Maybe we should make the password a secret in a separate PR

this soft maybe does not feel honest as we both know you'd abosolutely not allow for this to go in the same PR, so please skip the pleasantries and just tell me to raise an issue and make a PR...

callumforrester · 2024-10-14T14:23:31Z

For the record it was a genuine maybe, I agree that those changes are tangentially related. I read them during review and decided not to contest them unless they caused problems. I am unable to commit the time to help you debug the error right now so I'm leaving it up to you whether you want to put time into debugging on your own or make an issue and defer until later. I am happy with either resolution.

stan-dot · 2024-10-15T11:22:26Z

  /workspaces/blueapi/src/blueapi/startup/simmotor.py:41:9 - error: Method "set" overrides class "SynAxis" in an incompatible manner
    Return type mismatch: base method returns type "MoveStatus", override returns type "Status"
      "Status" is not assignable to "MoveStatus" (reportIncompatibleMethodOverride)
  /workspaces/blueapi/src/blueapi/startup/simmotor.py:70:14 - error: "MoveStatus" is not defined (reportUndefinedVariable)
  /workspaces/blueapi/src/blueapi/startup/simmotor.py:93:36 - error: "MoveStatus" is not defined (reportUndefinedVariable)
  /workspaces/blueapi/src/blueapi/startup/simmotor.py:94:16 - error: "MoveStatus" is not defined (reportUndefinedVariable)
/workspaces/blueapi/src/blueapi/worker/task_worker.py
  /workspaces/blueapi/src/blueapi/worker/task_worker.py:99:54 - error: Argument of type "LoggingPropertyMachine | ProxyString | str" cannot be assigned to parameter "bluesky_state" of type "RawRunEngineState" in function "from_bluesky_state"
    Type "LoggingPropertyMachine | ProxyString | str" is not assignable to type "RawRunEngineState"
      Type "str" is not assignable to type "RawRunEngineState"
        Type "str" is not assignable to type "type[ProxyString]"
        Type "str" is not assignable to type "type[str]"
        Type "str" is not assignable to type "type[PropertyMachine]" (reportArgumentType)
/workspaces/blueapi/tests/unit_tests/worker/devices.py
  /workspaces/blueapi/tests/unit_tests/worker/devices.py:43:9 - error: Method "set" overrides class "Movable" in an incompatible manner
    Return type mismatch: base method returns type "Status", override returns type "Status"
      "Status" is incompatible with protocol "Status"
        "exception" is an incompatible type
          Type "(timeout: Unknown | None = None) -> (StatusTimeoutError | Exception | type[Exception] | None)" is not assignable to type "(timeout: float | None = 0) -> (BaseException | None)"
            Function return type "StatusTimeoutError | Exception | type[Exception] | None" is incompatible with type "BaseException | None"
        "bluesky.protocols.Status" is not assignable to "ophyd.status.Status"
        "bluesky.protocols.Status" is not assignable to "ophyd.status.Status" (reportIncompatibleMethodOverride)
  /workspaces/blueapi/tests/unit_tests/worker/devices.py:45:16 - error: Type "AdditionalUpdateStatus" is not assignable to return type "Status"
    "AdditionalUpdateStatus" is not assignable to "Status" (reportReturnType)
7 errors, 0 warnings, 0 informations

those are the errors, they are all grounded in the chaos around ophyd-vs-ophyd-async bluesky protocol import (SynAxis and worker devices cases), as well as with State tracking - EngineState and the application specific state, and the unexpected super_state_machine import - not maintained, afaik vendored in into Bluesky.

either those are solved in a different PR or this PR is merged with 'ignore' flags.

src/blueapi/service/runner.py

callumforrester · 2024-10-16T10:39:52Z

@stan-dot I am okay with ignore flags on ophyd (not ophyd-async) devices, this problem is probably related to bluesky/bluesky#1809

stan-dot · 2024-10-16T15:11:24Z

any idea why are the tests taking forever @callumforrester ?

callumforrester · 2024-10-17T12:17:30Z

No although all bets are off while main is broken...
They may have been queued, we can only run up to 20 in the org at once

stan-dot · 2024-10-24T09:37:27Z

will just make new issues for the type: ignores. also a propagation of changes to upstream repos to increase type safety there

…mment

!

stan-dot · 2024-10-30T17:37:56Z

locally tests also take a long time, looking for which one makes it so long

stan-dot · 2024-10-30T17:39:01Z

also issues on main

callumforrester · 2024-10-31T10:16:51Z

@stan-dot is #601 related to the test issues?

stan-dot · 2024-10-31T14:59:44Z

not sure

stan-dot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file python Pull requests that update Python code dev-environment Issues for improving the developer environment/experience labels Sep 11, 2024

stan-dot requested review from ZohebShaikh and callumforrester September 11, 2024 13:55

stan-dot self-assigned this Sep 11, 2024

stan-dot linked an issue Sep 11, 2024 that may be closed by this pull request

Move to pyright - awaits dodal change #619

Open

stan-dot force-pushed the 619-move-to-pyright branch from 1bd5afb to 48371ac Compare September 11, 2024 14:28

stan-dot force-pushed the 619-move-to-pyright branch from 48371ac to d28365e Compare October 11, 2024 09:34

stan-dot marked this pull request as ready for review October 11, 2024 09:59

stan-dot requested a review from dan-fernandes October 11, 2024 09:59

callumforrester requested changes Oct 14, 2024

View reviewed changes

DiamondJoseph reviewed Oct 15, 2024

View reviewed changes

src/blueapi/service/runner.py Outdated Show resolved Hide resolved

stan-dot force-pushed the 619-move-to-pyright branch from 53dc1ad to 5850a02 Compare October 16, 2024 13:22

callumforrester mentioned this pull request Oct 18, 2024

Add Tracing and rework cli so that the controller command does not start a FastAPI app #669

Merged

stan-dot force-pushed the 619-move-to-pyright branch from 5850a02 to c9c2d44 Compare October 28, 2024 10:59

stan-dot requested a review from callumforrester October 28, 2024 16:20

stan-dot force-pushed the 619-move-to-pyright branch from c9c2d44 to 56483a7 Compare October 29, 2024 12:34

stan-dot added 11 commits October 30, 2024 17:24

switch to pyright, 34 errors left

2e11392

errors list

ffbb6d7

fixing some errors

f25f8a7

rebasing

63b22fa

fix pyright complaints, mostly due to ophyd, mostly through ignore co…

118c2ce

…mment

partial response to feedback

b5116f7

partial fix

6bc1bb5

add ignore lint to the devices@

beb4d29

!

type change

e5a8b9b

add a comment explaining the queue behavior

36c5097

fix line length

406e39c

stan-dot force-pushed the 619-move-to-pyright branch from 5f15f7c to 406e39c Compare October 30, 2024 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

619 move to pyright #632

619 move to pyright #632

stan-dot commented Sep 11, 2024 •

edited

Loading

stan-dot commented Oct 4, 2024

callumforrester commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 14, 2024

callumforrester Oct 14, 2024

stan-dot Oct 14, 2024

callumforrester Oct 14, 2024

stan-dot Oct 15, 2024

callumforrester Oct 16, 2024

stan-dot Oct 24, 2024

stan-dot Oct 24, 2024

stan-dot Oct 30, 2024

callumforrester Oct 30, 2024

stan-dot Oct 30, 2024

callumforrester Oct 31, 2024

stan-dot Oct 31, 2024

callumforrester commented Oct 14, 2024

stan-dot commented Oct 14, 2024 •

edited

Loading

callumforrester commented Oct 14, 2024

stan-dot commented Oct 15, 2024

callumforrester commented Oct 16, 2024

stan-dot commented Oct 16, 2024

callumforrester commented Oct 17, 2024 •

edited

Loading

stan-dot commented Oct 24, 2024

stan-dot commented Oct 30, 2024

stan-dot commented Oct 30, 2024

callumforrester commented Oct 31, 2024

stan-dot commented Oct 31, 2024

619 move to pyright #632

Are you sure you want to change the base?

619 move to pyright #632

Conversation

stan-dot commented Sep 11, 2024 • edited Loading

stan-dot commented Oct 4, 2024

callumforrester commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 11, 2024

stan-dot commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callumforrester commented Oct 14, 2024

stan-dot commented Oct 14, 2024 • edited Loading

callumforrester commented Oct 14, 2024

stan-dot commented Oct 15, 2024

callumforrester commented Oct 16, 2024

stan-dot commented Oct 16, 2024

callumforrester commented Oct 17, 2024 • edited Loading

stan-dot commented Oct 24, 2024

stan-dot commented Oct 30, 2024

stan-dot commented Oct 30, 2024

callumforrester commented Oct 31, 2024

stan-dot commented Oct 31, 2024

stan-dot commented Sep 11, 2024 •

edited

Loading

stan-dot commented Oct 14, 2024 •

edited

Loading

callumforrester commented Oct 17, 2024 •

edited

Loading