Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

619 move to pyright #632

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

619 move to pyright #632

wants to merge 9 commits into from

Conversation

stan-dot
Copy link
Collaborator

@stan-dot stan-dot commented Sep 11, 2024 •
edited
Loading

points that need addressing

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

@stan-dot 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 stan-dot self-assigned this Sep 11, 2024
@stan-dot stan-dot linked an issue Sep 11, 2024 that may be closed by this pull request
Move to pyright #619
Open
@stan-dot
Copy link
Collaborator Author

stan-dot commented Oct 4, 2024

@callumforrester please let me know your thoughts

@callumforrester
Copy link
Collaborator

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

@stan-dot
Copy link
Collaborator Author

ok will try to finish this today

@stan-dot
Copy link
Collaborator Author

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
Copy link
Collaborator Author

task_worker complains - because run_engine is not typed 😢

@stan-dot
Copy link
Collaborator Author

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

@stan-dot
Copy link
Collaborator Author

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

@stan-dot stan-dot marked this pull request as ready for review October 11, 2024 09:59
@stan-dot
Copy link
Collaborator Author

link to the error:

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

callumforrester
callumforrester requested changes Oct 14, 2024
View reviewed changes
src/blueapi/service/interface.py Outdated Show resolved Hide resolved
src/blueapi/service/runner.py Outdated Show resolved Hide resolved
src/blueapi/startup/simmotor.py Show resolved Hide resolved
src/blueapi/utils/__init__.py Show resolved Hide resolved
src/blueapi/worker/task_worker.py Outdated
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will look into it

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

tests/unit_tests/worker/devices.py Show resolved Hide resolved
@callumforrester
Copy link
Collaborator

@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
Copy link
Collaborator Author

stan-dot commented Oct 14, 2024 •
edited
Loading

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
Copy link
Collaborator

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
Copy link
Collaborator Author 

  /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.

@callumforrester
Copy link
Collaborator

@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
Copy link
Collaborator Author

any idea why are the tests taking forever @callumforrester ?

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

@callumforrester callumforrester callumforrester requested changes

@DiamondJoseph DiamondJoseph DiamondJoseph left review comments

@ZohebShaikh ZohebShaikh Awaiting requested review from ZohebShaikh

@dan-fernandes dan-fernandes Awaiting requested review from dan-fernandes

Requested changes must be addressed to merge this pull request.

Assignees

@stan-dot stan-dot

Labels
dependencies Pull requests that update a dependency file dev-environment Issues for improving the developer environment/experience documentation Improvements or additions to documentation python Pull requests that update Python code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move to pyright
3 participants
@stan-dot @callumforrester @DiamondJoseph