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

FIX: more error handling for ElogPoster #389

Merged
merged 5 commits into from
Oct 14, 2024

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Oct 11, 2024

Description

This comes from a hotfix in rix3 that I added onto here.

There is a niche issue where "something" gets loaded as the elog but it isn't a valid elog object. Then, this script picks up the non-elog item and passes it to the ElogPoster, which starts causing an issue (text spam? I don't quite remember) when you try to run a scan later.

This PR rearranges the error reporting a bit and includes a check that the expected objects haven't been replaced.

Motivation and Context

How Has This Been Tested?

Interactively only, checking the error messages

Where Has This Been Documented?

Here only

Pre-merge checklist

  • Code works interactively (a real hutch config file can be loaded)
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@ZLLentz ZLLentz requested a review from tangkong October 11, 2024 23:55
@tangkong
Copy link
Contributor

tangkong commented Oct 12, 2024

Seems like elog was never an explicit dependency on the pip-side, weird (not weird, it's not on pypi)

I forget, why haven't we added it to pypi?

@ZLLentz
Copy link
Member Author

ZLLentz commented Oct 12, 2024

It must be some issue with a dependency of a dependency that we couldn't get onto pypi

@ZLLentz
Copy link
Member Author

ZLLentz commented Oct 12, 2024

For this PR, I'll just protect the import in the safe load or in a try/except

@ZLLentz
Copy link
Member Author

ZLLentz commented Oct 12, 2024

The actual answer is probably that elog is already taken on pypi

@tangkong
Copy link
Contributor

tangkong commented Oct 14, 2024

ah yes. PyPi's elog is an unmaintained elasticsearch log system https://pypi.org/project/elog/

Proposals for names? lcls-elog? explog? the-elog?

Of course we already have an issue for this. pcdshub/elog#30

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Seems like a good fix.

raise RuntimeError("RE not loaded, skip ElogPoster")
# Even if they exist, things can go wrong if the user accidentally clobbers the name
if not isinstance(elog, HutchELog):
raise RuntimeError("elog replaced, skip ELogPoster")
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is something the hint banner could also fall prey to, if someone overwrites an existing namespace global.

I think it's maybe a small miracle that we haven't hit upon a case where a hutch overwrites one of our helpful namespaces (a, m, etc). Or maybe they just don't use those and access the motors by name explicitly. 🤷

@ZLLentz ZLLentz merged commit 4c37c88 into pcdshub:master Oct 14, 2024
11 checks passed
@ZLLentz ZLLentz deleted the fix_elog_replaced_issue branch October 14, 2024 18:44
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