-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use ophyd-async 0.9a2 #770
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #770 +/- ##
==========================================
+ Coverage 87.02% 87.04% +0.02%
==========================================
Files 101 103 +2
Lines 6974 6988 +14
==========================================
+ Hits 6069 6083 +14
Misses 905 905
|
resources.files(panda_resource) / "panda-gridscan.yaml" | ||
) as config_yaml_path: | ||
yield from load_device(panda, str(config_yaml_path)) | ||
yield from load_panda_from_yaml( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the motivation from moving from importlib.resources
to passing around paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I could see, importlib.resources was very awkward when needing to specify the directory rather than exact path - and we need the directory now for the load plan
@@ -0,0 +1,3 @@ | |||
import os | |||
|
|||
ROOT_DIR = os.path.dirname(os.path.abspath(__file__)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For things which we know are resources contained within our python project, I think I prefer to use importlib.resources
to locate them rather than to use file paths. As far as I can tell it seems to be the "proper" way.
Python doesn't guarantee that every module has a __file__
attribute since modules aren't always in files. Having said that I suspect that our packages will probably always be available as files.
I think most of my thoughts in this area derive from analogy with Java ClassLoaders which are a similar concept to python module, and it's best practice to bundle your resources in your jar file (usually in a separate hierarchy), and I think that is the overall intent of things like importlib.resources
to encourage this, so that python packaging tools also know how to deploy your resources.
Having said all this
importlib.resources
does seem to be annoyingly immature as an API- there's no functional imperative to use this mechanism at the moment
so I leave this here as an opinion you can choose to follow or ignore...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in person - as of Python 3.12, we can use this API to get a directory traversible, but we can't use it just yet. I will leave the code as-is but add a comment + issue to use resources
again once we drop python < 3.12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #798
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
Fixes #706
DeviceCollector
renamed toinit_devices
Requires DiamondLightSource/dodal#1014