Skip to content

ENH: add ophyd helpers from typhos/atef#46

Merged
ZLLentz merged 3 commits into
pcdshub:masterfrom
klauer:enh_ophyd_subs
Mar 11, 2022
Merged

ENH: add ophyd helpers from typhos/atef#46
ZLLentz merged 3 commits into
pcdshub:masterfrom
klauer:enh_ophyd_subs

Conversation

@klauer
Copy link
Copy Markdown
Contributor

@klauer klauer commented Feb 18, 2022

Description

  • Adds no_device_lazy_load: Context manager which disables the ophyd.device.Device lazy_wait_for_connection behavior and later restore its value.
  • Adds subscription_context: Context manager which subscribes to a specific event from all provided objects, and unsubscribes after.
  • Adds subscription_context_device: Context manager which subscribes to a specific event from all matching signals on a device, and unsubscribes after.
  • Adds get_all_signals_from_device: Gets all signals that match from a given device, with special lazy handling
  • Adds acquire_blocking which subscribes to a signal for a period of time and returns all values acquired
  • Adds acquire_async which does the same but for asyncio
  • Adds pcdsutils.type_hints for common/reusable hints (considering to add reusable ones from elsewhere down the line)

Motivation and Context

  • This code is reusable and useful, so it belongs here.

How Has This Been Tested?

  • Included test suite.

Where Has This Been Documented?

  • This PR text and docstrings.

@klauer klauer changed the title ENH: add ophyd helpers from typhos ENH: add ophyd helpers from typhos/atef Mar 10, 2022
@klauer klauer marked this pull request as ready for review March 10, 2022 23:35
data : List[PrimitiveType]
The data acquired. Guaranteed to have at least one item.
"""
with _acquire(signal) as data:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know you could accumulate generator values into lists like this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe we're saying the same thing, but to clarify: the context manager returns a mutable value data that's a list - which is appended to and returned by acquire.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I misunderstood- thank you for the clarification.
So, the way this works is that we yield a list which continues to be mutated until we drop out of this with block.

import time
from typing import Callable, List, Optional

import ophyd
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

New dependency? Do we add this to the meta.yaml/requirements.txt or do we take it as optional? If optional, is it ok to leave it exposed and assume nobody imports here if they don't have ophyd?

Previously we had an implied ophyd dependency in the log submodule, but it was not tied to an import.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My feeling is to keep it optional, I think. To recap/add some thoughts:

  • It's currently in dev-requirements.txt for the existing test suite.
  • I could make it fail more gracefully, perhaps, if we think it importing ophyd_helpers without ophyd could ever be a potential issue.
  • Maybe the dependency tree would illuminate how big a deal additional deps would be for pcdsutils. Would you by chance have a tree handy of all our packages that depend on pcdsutils currently?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm happy to leave it like this, but it felt like an important thing to at least exchange words about.

$ mamba repoquery whoneeds pcdsutils

 Name         Version Build        Depends           Channel
─────────────────────────────────────────────────────────────
 pcdsdevices  5.1.0   pyhd8ed1ab_0 pcdsutils >=0.4.0
 typhos       2.2.1   pyhd8ed1ab_0 pcdsutils
 lucid        0.9.0   pyhd8ed1ab_0 pcdsutils
 pmgr         2.0.2   pyhd8ed1ab_1 pcdsutils
 hutch-python 1.13.2  py_1         pcdsutils >=0.6.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Of the above set, only pmgr doesn't have ophyd as a dependency.

In general, as long as this doesn't become a top-level import I see no problem in leaving this as an optional dependency with a loud import failure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to leave it like this, but it felt like an important thing to at least exchange words about.

This is 100% something that should be brought up in the review - thank you.

In general, as long as this doesn't become a top-level import I see no problem in leaving this as an optional dependency with a loud import failure.

I think I agree. For a utility library like this, I think I like the pattern of "use what you like, just make sure you have the deps in your project for what you use".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, great. If ever there comes a day where we feel very strongly about a non-ophyd utility existing in ophyd_helpers we can revisit (let's not do that).

Copy link
Copy Markdown
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

These are all useful utilities and I'm glad to see them here

I'd like to quickly decide on a resolution for handling the dependency before merging

@ZLLentz ZLLentz merged commit e8b308c into pcdshub:master Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants