search

bluesky / ophyd

hardware abstraction in Python with an emphasis on EPICS
https://blueskyproject.io/ophyd
BSD 3-Clause "New" or "Revised" License
51 stars 79 forks source link

semantics of Signal.value have changed #814

Open tacaswell opened 4 years ago

tacaswell commented 4 years ago

In 8acc424b325e3447db2ac4601e7bc0c6d1403532 which came in via https://github.com/bluesky/ophyd/pull/653 the meaning of sig.value changed from "go consult the control system" to "here have my cached value". For Signal this isn't a problem (as setting it will update the cache) and for EpicsSignal with auto-monitoring this will also be OK, but for EpicsSignal it means that it returns stale values (which caused confusion at a beamline which is testing the new software [1]). A quick search across the profiles at NSLS-II shows something like ~1.2k usages of .value across 20 beamlines.

Short term, I think the correct thing to do is remove the cache logic from signal.value because this is a silent gotcha for our users.

Long term do we:

I can see a case for all of them.

[1] https://github.com/NSLS-II-SIX/profile_collection/issues/29

prjemian commented 4 years ago

Big problem for both XPCS & USAXS, this is XPCS: https://github.com/BCDA-APS/apstools/issues/270

On Fri, Mar 6, 2020, 5:39 PM Thomas A Caswell notifications@github.com wrote:

In 8acc424 https://github.com/bluesky/ophyd/commit/8acc424b325e3447db2ac4601e7bc0c6d1403532 which came in via #653 https://github.com/bluesky/ophyd/pull/653 the meaning of sig.value changed from "go consult the control system" to "here have my cached value". For Signal this isn't a problem (as setting it will update the cache) and for EpicsSignal with auto-monitoring this will also be OK, but for EpicsSignal it means that it returns stale values (which caused confusion at a beamline which is testing the new software [1]). A quick search across the profiles at NSLS-II shows something like ~1.2k usages of .value across 20 beamlines.

Short term, I think the correct thing to do is remove the cache logic from signal.value because this is a silent gotcha for our users.

Long term do we:

  • want leave it as a maybe-slow attribute?
  • make it raise on EpicsSignal is not monitoring?
  • always auto-monitor with EpicsSignal?
  • deprecate

I can see a case for all of them.

[1] NSLS-II-SIX/profile_collection#29 https://github.com/NSLS-II-SIX/profile_collection/issues/29

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bluesky/ophyd/issues/814?email_source=notifications&email_token=AARMUMCZ2NJ3QRNS3Q3YM2TRGGCSXA5CNFSM4LDJ2T32YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ITH223Q, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARMUMGT5ZLSXJNTAMM3EETRGGCSXANCNFSM4LDJ2T3Q .

prjemian commented 4 years ago

Monitor EpicsSignal is the assumption at APS, stunned to find that had not been the case

The monitor should be the most efficient use. It only updates on new information. There are a few fields which do not post monitors (consult EPICS record reference manual) but those fields can be handled.

On Fri, Mar 6, 2020, 6:41 PM Pete Jemian prjemian@gmail.com wrote:

Big problem for both XPCS & USAXS, this is XPCS: https://github.com/BCDA-APS/apstools/issues/270

On Fri, Mar 6, 2020, 5:39 PM Thomas A Caswell notifications@github.com wrote:

In 8acc424 https://github.com/bluesky/ophyd/commit/8acc424b325e3447db2ac4601e7bc0c6d1403532 which came in via #653 https://github.com/bluesky/ophyd/pull/653 the meaning of sig.value changed from "go consult the control system" to "here have my cached value". For Signal this isn't a problem (as setting it will update the cache) and for EpicsSignal with auto-monitoring this will also be OK, but for EpicsSignal it means that it returns stale values (which caused confusion at a beamline which is testing the new software [1]). A quick search across the profiles at NSLS-II shows something like ~1.2k usages of .value across 20 beamlines.

Short term, I think the correct thing to do is remove the cache logic from signal.value because this is a silent gotcha for our users.

Long term do we:

  • want leave it as a maybe-slow attribute?
  • make it raise on EpicsSignal is not monitoring?
  • always auto-monitor with EpicsSignal?
  • deprecate

I can see a case for all of them.

[1] NSLS-II-SIX/profile_collection#29 https://github.com/NSLS-II-SIX/profile_collection/issues/29

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bluesky/ophyd/issues/814?email_source=notifications&email_token=AARMUMCZ2NJ3QRNS3Q3YM2TRGGCSXA5CNFSM4LDJ2T32YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ITH223Q, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARMUMGT5ZLSXJNTAMM3EETRGGCSXANCNFSM4LDJ2T3Q .

tacaswell commented 4 years ago

Early on we had two concerns with "monitor and cache". One was overwhelming the client with too many updates (particularly if you subscribe to a couple of noisey and/or big PVs that you don't actually need (like the Array pvs on AD or some of the stats values). The other, a bit ironically, is that the protocol specifies that monitors can be dropped on the floor so we were worried about getting stale information.

@prjemian Is there already an ophyd issue that I missed? This is the sort of thing that should definitely be reported upstream!

prjemian commented 4 years ago

Thanks. I will next time.

On Fri, Mar 6, 2020, 7:01 PM Thomas A Caswell notifications@github.com wrote:

Early on we had two concerns with "monitor and cache". One was overwhelming the client with too many updates (particularly if you subscribe to a couple of noisey and/or big PVs that you don't actually need (like the Array pvs on AD or some of the stats values). The other, a bit ironically, is that the protocol specifies that monitors can be dropped on the floor so we were worried about getting stale information.

@prjemian https://github.com/prjemian Is there already an ophyd issue that I missed? This is the sort of thing that should definitely be reported upstream!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bluesky/ophyd/issues/814?email_source=notifications&email_token=AARMUMACFDHYDTZSIRHCVK3RGGMHBA5CNFSM4LDJ2T32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEODJ34I#issuecomment-596024817, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARMUMEWELU3HV72DQQIYNLRGGMHBANCNFSM4LDJ2T3Q .

tacaswell commented 4 years ago

As a rule of thumb, if something changes in a way that requires client code to change and it is not documented as an intentional API change, then we want to know (as that is a bug). If it is documented as an API change and it is from an rc or master we also want to know (as that is a chance to talk us off of that ledge ;) )