Skip to content

FIX: reject invalid inputs up-front to avoid PUT timeouts - #40

Merged
JJL772 merged 10 commits into
mainfrom
pr-fix-put-timeouts
Aug 11, 2026
Merged

FIX: reject invalid inputs up-front to avoid PUT timeouts#40
JJL772 merged 10 commits into
mainfrom
pr-fix-put-timeouts

Conversation

@JJL772

@JJL772 JJL772 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Models that take a while to simulate can delay the put done callbacks until they timeout. caput just shows a stale value in its readback, and pvput displays "Put timeout".

Even if we move the done callbacks to before the Model.set call, updates can get stalled in the queue and timeout. For example, if you have a slow model and caput/pvput while the model is simulating.

I'm not sure that there's a good way to implement done callbacks that encompass exceptions thrown during model simulation. The best thing we can do is reject invalid values up front in the CA/PVA callbacks. Luckily PVA allows us to specify an error message, though CA doesn't (hence the LOG.warn).

Curious if you have any better ideas or feedback on this change @tangkong. Do you guys need the caput-waits-for-model-simulation behavior?

Closes #39
Closes #43

@JJL772
JJL772 requested a review from tangkong July 31, 2026 00:51
@JJL772
JJL772 force-pushed the pr-fix-put-timeouts branch from 8bee898 to b41050c Compare July 31, 2026 05:15
@tangkong

Copy link
Copy Markdown
Collaborator

The put-completion was important to us to allow scans to not have hard-coded sleep statements between points. Without this signal we're left guessing at how long a step will take. For simulations that take varying amounts of time, this helps us be more efficient (and not have scans with single long steps force our step-waits to be long across the board)

I might argue that the current behavior is correct. EPICS IOCs are terribly inconsistent in their behavior, but it's not uncommon for records to reject puts while processing is ongoing. We could have an additional PV similar to the PACT (processing active) field that publicizes when the IOC is locked out. (some EPICS docs) This is an alternate way of timing your scans, but still involves some amount of manual polling logic, rather than using built-in put-completion logic in e.g. pyepics.

Even if we move the done callbacks to before the Model.set call, updates can get stalled in the queue and timeout.

Doesn't a model need to finish simulating before any updates to PVs can be meaningful? Or do you mean that other clients attempting to read from the IOC cannot caget properly during a simulation? (am I misunderstanding the issue here?)

@JJL772

JJL772 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

The put-completion was important to us to allow scans to not have hard-coded sleep statements between points. Without this signal we're left guessing at how long a step will take. For simulations that take varying amounts of time, this helps us be more efficient (and not have scans with single long steps force our step-waits to be long across the board)

Makes sense. My original idea for gating involved monitoring output variables, which would only be posted with a new value after the simulation completes. That obviously has some caveats.

I'm going to add a generic status PV to indicate model state.

I might argue that the current behavior is correct.

In terms of rejecting puts while the model is simulating, I tend to agree. The issue I'm seeing though is all pvputs timing out with a slow model, because it takes so long to ACK it.

Doesn't a model need to finish simulating before any updates to PVs can be meaningful? Or do you mean that other clients attempting to read from the IOC cannot caget properly during a simulation? (am I misunderstanding the issue here?)

Since updates are queued along with their done callback, caput/pvput during slow model updates end up timing out too. This isn't really relevant though; I just initially tried to fix it by adding the done callback handling to before the Model.set.

I guess there are a couple of behavioral things we need to think about:

  1. PV updates during model execution
    1. Should pvput/caput fail/timeout if they happen while the model is simulating?
    2. Should updates to these input variables be queued up for the next simulation pass? (Current behavior)
      1. This is problematic for variables that act as both inputs and outputs, since the post-simulation update will overwrite the new value received during the previous model run.
  2. CA/PVA operation completion
    1. Should these be acknowledged up-front after a quick value validation? (what this PR implements)
    2. Should these stall until the model run completes?
      1. Again, with this, slow models will cause pvput timeouts and caput readback timeouts even if the model simulates fine.

Should we setup a meeting this afternoon to hash this out?

JJL772 added 3 commits July 31, 2026 16:08
We do not need monotonic time here, and we are generally dealing with UNIX time everywhere else (including in p4p and pcaspy)
Two modes for PV ack'ing:
* immediate: Ack the pvput/caput request as soon as it's been inserted
  into the update queue.
* complete: Ack the pvput/caput request after the model is done
  simulating. Requires timeout changes on the client side for
  long-simulating models.

Also adds a model state PV that gets updated with the model's current
state.
@JJL772
JJL772 force-pushed the pr-fix-put-timeouts branch from b41050c to 540a818 Compare August 3, 2026 20:20
@JJL772
JJL772 marked this pull request as draft August 4, 2026 16:29
@JJL772
JJL772 marked this pull request as ready for review August 5, 2026 22:19
@JJL772

JJL772 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

There are intermittent test failures due to the STATUS PV not connecting. I have zero idea why this is the case; the tests work reliably locally.

@tangkong tangkong left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems reasonable. It's backwards compatible (so I'm happy). I mostly have questions and suggestions that we could look at later.

In IMMEDIATE mode, it seems like puts are queued up so that as soon as the first sim is complete the next will fire? This seems like it could lead to some confusing behavior where puts get automated and the model just runs sim after sim.

I think my preferred behavior would be to ignore puts while the sim is running, but perhaps that's not what the people wanted

Comment thread lume_pva/runner.py
Comment thread lume_pva/runner.py
Comment thread lume_pva/runner.py Outdated
Comment thread lume_pva/runner.py Outdated
Comment thread lume_pva/runner.py
Comment thread lume_pva/runner.py Outdated
@JJL772
JJL772 requested a review from tangkong August 7, 2026 22:41
@JJL772
JJL772 force-pushed the pr-fix-put-timeouts branch from deac3f7 to da12775 Compare August 7, 2026 22:42
@JJL772
JJL772 force-pushed the pr-fix-put-timeouts branch from da12775 to f01711f Compare August 7, 2026 23:05

@tangkong tangkong left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me! Thanks for accommodating my insistence on put-completion 😆

Comment thread lume_pva/runner.py Outdated
Complete = "complete"


class VariableMode(str, Enum):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

oh I forgot to mention, I think StrEnum is available in our versions of python (3.11). But it's not an important adjustment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. I pretty much just copied LUME base here

@JJL772

JJL772 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

GitHub wasting CI time yet again.
image

@JJL772
JJL772 merged commit 3acccef into main Aug 11, 2026
13 of 14 checks passed
@JJL772
JJL772 deleted the pr-fix-put-timeouts branch August 11, 2026 15:58
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.

Add --prefix command-line argument to all example scripts Timeouts when interacting with a slow model

2 participants