795 Care about where a plan is sourced

[callumforrester]

Fixes #795

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.46%. Comparing base (e18b9be) to head (62f60b2).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #807   +/-   ##
=======================================
  Coverage   93.45%   93.46%           
=======================================
  Files          38       38           
  Lines        2108     2111    +3     
=======================================
+ Hits         1970     1973    +3     
  Misses        138      138           

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

[DiamondJoseph]

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.

[callumforrester]

Yes, master yoda

[DiamondJoseph]

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.

[DiamondJoseph]

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

[callumforrester]

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

[DiamondJoseph]

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

[DiamondJoseph]

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

[DiamondJoseph]

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

[DiamondJoseph]

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?

[callumforrester]

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'>