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.

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

795 Care about where a plan is sourced #807

Merged
merged 8 commits into from
Jan 31, 2025
4 changes: 3 additions & 1 deletion docs/how-to/write-plans.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ def my_plan(
...
```

The type annotations (e.g. `: str`, `: int`, `-> MsgGenerator`) are required as blueapi uses them to detect that this function is intended to be a plan and generate its runtime API.
## Detection

The type annotations in the example above (e.g. `: str`, `: int`, `-> MsgGenerator`) are required as blueapi uses them to detect that this function is intended to be a plan and generate its runtime API. If there is an [`__all__` dunder](https://docs.python.org/3/tutorial/modules.html#importing-from-a-package) present in the module, blueapi will read that and import anything within that qualifies as a plan, per its type annotations. If not it will simpyl read everything in the module that hasn't been imported, for example it will ignore a plan imported from another module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The type annotations in the example above (e.g. `: str`, `: int`, `-> MsgGenerator`) are required as blueapi uses them to detect that this function is intended to be a plan and generate its runtime API. If there is an [`__all__` dunder](https://docs.python.org/3/tutorial/modules.html#importing-from-a-package) present in the module, blueapi will read that and import anything within that qualifies as a plan, per its type annotations. If not it will simpyl read everything in the module that hasn't been imported, for example it will ignore a plan imported from another module.
The type annotations in the example above (e.g. `: str`, `: int`, `-> MsgGenerator`) are required as blueapi uses them to detect that this function is intended to be a plan and generate its runtime API. If there is an [`__all__` dunder](https://docs.python.org/3/tutorial/modules.html#importing-from-a-package) present in the module, blueapi will use that: importing anything within that qualifies as a plan, per its type annotations. Else it will read everything in the module that hasn't been imported: e.g. ignoring a plan imported from another module.

Must simpyl -> simply or just being removed: I've decided to be opposed to adverbs in documentation- it doesn't matter if it is simply done or complexly done, only that it is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, master yoda


**Input annotations should be as broad as possible**, the least specific implementation that is sufficient to accomplish the requirements of the plan. For example, if a plan is written to drive a specific motor (`MyMotor`), but only uses the general methods on the [`Movable` protocol](https://blueskyproject.io/bluesky/main/hardware.html#bluesky.protocols.Movable), it should take `Movable` as a parameter annotation rather than `MyMotor`.

Expand Down
16 changes: 14 additions & 2 deletions src/blueapi/core/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@

from blueapi import utils
from blueapi.config import EnvironmentConfig, SourceKind
from blueapi.utils import BlueapiPlanModelConfig, load_module_all
from blueapi.utils import (
BlueapiPlanModelConfig,
is_function_sourced_from_module,
load_module_all,
)

from .bluesky_types import (
BLUESKY_PROTOCOLS,
Expand Down Expand Up @@ -99,7 +103,15 @@ def plan_2(...) -> MsgGenerator:
"""

for obj in load_module_all(module):
if is_bluesky_plan_generator(obj):
# The rule here is that we only inspect objects defined in the module
# (as opposed to objects imported from other modules) to determine if
# they are valid plans, unless there is an __all__ defined in the module,
# in which case we only inspect objects listed there, regardless of their
# original source module.
if (
hasattr(module, "__all__")
or is_function_sourced_from_module(obj, module)
) and is_bluesky_plan_generator(obj):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (
hasattr(module, "__all__")
or is_function_sourced_from_module(obj, module)
) and is_bluesky_plan_generator(obj):
if is_bluesky_plan_generator(obj) and (
hasattr(module, "__all__")
or is_function_sourced_from_module(obj, module)
):

Currently we import the module for every object that is found in the module- reversing this check will a. cut down on the number of times we try and import the module, and b. allow is_function_sourced_from_module to have tighter bounds.

self.register_plan(obj)

def with_device_module(self, module: ModuleType) -> None:
Expand Down
3 changes: 2 additions & 1 deletion src/blueapi/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from .connect_devices import connect_devices
from .file_permissions import get_owner_gid, is_sgid_set
from .invalid_config_error import InvalidConfigError
from .modules import load_module_all
from .modules import is_function_sourced_from_module, load_module_all
from .serialization import serialize
from .thread_exception import handle_all_exceptions

Expand All @@ -17,4 +17,5 @@
"connect_devices",
"is_sgid_set",
"get_owner_gid",
"is_function_sourced_from_module",
]
15 changes: 15 additions & 0 deletions src/blueapi/utils/modules.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import importlib
from collections.abc import Iterable
from types import ModuleType
from typing import Any
Expand Down Expand Up @@ -34,3 +35,17 @@ def get_named_subset(names: list[str]):
for name, value in mod.__dict__.items():
if not name.startswith("_"):
yield value


def is_function_sourced_from_module(obj: Any, module: ModuleType) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

If not doing the above change of order of checking if obj is instance of Callable[[...], MsgGenerator], then this is (as suggested by the docstring) is_object_sourced_from_module...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going with your first suggestion, I'm not aware of any constraint that guarantees that something being Callable means it has a __module__ dunder

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just niting the name and the params telling different stories

"""
Check if an object is originally from a particular module, useful to detect
whether it actually comes from a nested import.

Args:
obj: Object to check
module: Module to check against object
"""
return (
hasattr(obj, "__module__") and importlib.import_module(obj.__module__) is module
)
Copy link
Contributor

Choose a reason for hiding this comment

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

When this is being used over all of the objects in a module, is it going to import the module and run any side effects for every object? This could (in plan modules out of our control) be dangerous. Or does it cache and only import the module once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It caches

# tt.py
print("Hello world")
>>> import importlib
>>> importlib.import_module("tt")
Hello world
<module 'tt' from '/workspaces/blueapi/tt.py'>
>>> importlib.import_module("tt")
<module 'tt' from '/workspaces/blueapi/tt.py'>

9 changes: 9 additions & 0 deletions tests/unit_tests/core/fake_plan_module_with_all.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
from bluesky.utils import MsgGenerator
from tests.unit_tests.core.fake_plan_module import plan_a, plan_b # noqa: F401


def plan_c(c: bool) -> MsgGenerator[None]: ...
def plan_d(d: int) -> MsgGenerator[int]: ...


__all__ = ["plan_a", "plan_d"]
6 changes: 6 additions & 0 deletions tests/unit_tests/core/fake_plan_module_with_imports.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from bluesky.utils import MsgGenerator
from tests.unit_tests.core.fake_plan_module import plan_a, plan_b # noqa: F401


def plan_c(c: bool) -> MsgGenerator[None]: ...
def plan_d(d: int) -> MsgGenerator[int]: ...
14 changes: 14 additions & 0 deletions tests/unit_tests/core/test_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,20 @@ def test_add_plan_from_module(empty_context: BlueskyContext) -> None:
assert EXPECTED_PLANS == empty_context.plans.keys()


def test_only_plans_from_source_module_detectede(empty_context: BlueskyContext) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_only_plans_from_source_module_detectede(empty_context: BlueskyContext) -> None:
def test_only_plans_from_source_module_detected(empty_context: BlueskyContext) -> None:

nit

import tests.unit_tests.core.fake_plan_module_with_imports as plan_module

empty_context.with_plan_module(plan_module)
assert {"plan_c", "plan_d"} == empty_context.plans.keys()


def test_only_plans_from_all_in_module_detectede(empty_context: BlueskyContext) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_only_plans_from_all_in_module_detectede(empty_context: BlueskyContext) -> None:
def test_only_plans_from_all_in_module_detected(empty_context: BlueskyContext) -> None:

nit

import tests.unit_tests.core.fake_plan_module_with_all as plan_module

empty_context.with_plan_module(plan_module)
assert {"plan_a", "plan_d"} == empty_context.plans.keys()


def test_add_named_device(empty_context: BlueskyContext, sim_motor: SynAxis) -> None:
empty_context.register_device(sim_motor)
assert empty_context.devices[SIM_MOTOR_NAME] is sim_motor
Expand Down
4 changes: 4 additions & 0 deletions tests/unit_tests/utils/functions_a.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
def a(): ...


def b(): ...
10 changes: 10 additions & 0 deletions tests/unit_tests/utils/functions_b.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from .functions_a import a, b # noqa: F401


def c(): ...


def d(): ...


e = 1
17 changes: 16 additions & 1 deletion tests/unit_tests/utils/test_modules.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from importlib import import_module

from blueapi.utils import load_module_all
from blueapi.utils import is_function_sourced_from_module, load_module_all


def test_imports_all():
Expand All @@ -11,3 +11,18 @@ def test_imports_all():
def test_imports_everything_without_all():
module = import_module(".lacksall", package="tests.unit_tests.utils")
assert list(load_module_all(module)) == [3, "hello", 9]


def test_source_is_in_module():
module = import_module(".functions_b", package="tests.unit_tests.utils")
assert is_function_sourced_from_module(module.__dict__["c"], module)


def test_source_is_not_in_module():
module = import_module(".functions_b", package="tests.unit_tests.utils")
assert not is_function_sourced_from_module(module.__dict__["a"], module)


def test_source_check_on_non_function():
module = import_module(".functions_b", package="tests.unit_tests.utils")
assert not is_function_sourced_from_module(module.__dict__["e"], module)