Skip to content
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

Adds except for caget failure #576

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Adds except for caget failure #576

merged 3 commits into from
Nov 27, 2023

Conversation

jlashner
Copy link
Collaborator

Adds exception for epics channel access get failure in pysmurf-controller check-state. I believe this should make the check-state function more robust by preventing the behavior seen in #575 .

Description

Excepts caget failures in check_state

Motivation and Context

#575

How Has This Been Tested?

Not tested yet

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

This looks fine to me for catching in the check_state process, but seeing bias steps occasionally see similar behavior (https://github.com/simonsobs/daq-discussions/discussions/59) makes me wonder if this handling is needed throughout. Maybe that goes in a separate PR, but I'd be interested to hear your thoughts.

Also, this adds pyepics as a direct dependency of socs when dealing with the pysmurf controller agent. We should add it to setup.py. I'm happy to do that bit if you'd like. Either way it should be done before merging.

@jlashner
Copy link
Collaborator Author

Ya... I think we want other operations to fail though in the event of an epics failure, where we don't really ever want the check_state process to stop running, so I don't think we should change the other tasks until we understand the source of the issue.

pyepics was already a dependency through sodetlib and pysmurf, so I don't think this actually adds any new dependencies... Dependencies for this agent were all handled because it is run in its own docker that is built on the so-smurf stack, rather than through setup.py

@BrianJKoopman
Copy link
Member

pyepics was already a dependency through sodetlib and pysmurf, so I don't think this actually adds any new dependencies... Dependencies for this agent were all handled because it is run in its own docker that is built on the so-smurf stack, rather than through setup.py

Right, but now that we're directly importing it in socs code it makes sense to track here, rather than rely on it being pulled in by another dep. I added it with the needed docs in c7792e6.

@BrianJKoopman BrianJKoopman self-requested a review November 27, 2023 15:05
@jlashner
Copy link
Collaborator Author

If that's the case, should we also uncomment the pysmurf, sodetlib, and sotodlib reqs?

@BrianJKoopman
Copy link
Member

If that's the case, should we also uncomment the pysmurf, sodetlib, and sotodlib reqs?

In the setup.py file? Direct references can't be packaged like that. If pysmurf, sodetlib, and sotodlib publish to PyPI we could.

@jlashner
Copy link
Collaborator Author

Ok, I'm fine w/ merging this but am still pretty confused by it. Maybe it makes sense for tracking / docs, but

pip install -U socs[pysmurf]

will never really be sufficient for running the agent outside of the docker

@BrianJKoopman
Copy link
Member

Ok, I'm fine w/ merging this but am still pretty confused by it. Maybe it makes sense for tracking / docs, but

pip install -U socs[pysmurf]

will never really be sufficient for running the agent outside of the docker

True, that's why there's the dependencies section on the agent page: https://socs--576.org.readthedocs.build/en/576/agents/pysmurf-controller.html#dependencies

This is pointed to from the installation page. I agree it's not ideal. But the best solution is to get those packages to publish to PyPI.

@BrianJKoopman BrianJKoopman merged commit 83c75f7 into main Nov 27, 2023
7 checks passed
@BrianJKoopman BrianJKoopman deleted the except-caget-failure branch November 27, 2023 15:43
hnakata-JP pushed a commit that referenced this pull request Apr 12, 2024
* Adds except for caget failure

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add pyepics as dependency for pysmurf-controller agent

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Brian Koopman <[email protected]>
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.

Pysmurf controller check_state failing out with ChannelAccessGetFailure
2 participants