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

ENH: expanded sim ioc generation, mfx example tree #68

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Nov 8, 2024

Description

  • Expand the sim IOCs to build themselves from real PVs to create a mock IOC as similar as possible to the real deal.
  • Add tests for this expanded functionality
  • Add an examples folder
  • Include an example tree that could be used to prepare some portion of the MFX beamline for beam.
  • Include helper scripts for testing and iterating on this example tree offline

Motivation and Context

How Has This Been Tested?

Interactively we can make it all the way through the tree!
I added and updated some unit tests.

Where Has This Been Documented?

Helpful docstrings and commments
Pre-release notes

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • 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 mentioned this pull request Nov 8, 2024
8 tasks
@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 9, 2024

The special launcher script here helps me run and test my tree and sim like this which is quite a bit better than some of the previous test variants:
image

examples/mfx_tree.py Outdated Show resolved Hide resolved
beams/bin/main.py Outdated Show resolved Hide resolved
@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 14, 2024

Updated version:
image

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 14, 2024

I'm intending to add a copy-the-ioc test and a pre-release docs before asking for reviews.

@ZLLentz ZLLentz changed the title WIP: mfx example tree ENH: expanded sim ioc generation, mfx example tree Nov 14, 2024
@ZLLentz ZLLentz marked this pull request as ready for review November 14, 2024 23:58
@tangkong
Copy link
Contributor

tests go boom :(

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 15, 2024

I should have caught that one, I forgot to re-run the tests after making "just one small change"

tangkong
tangkong previously approved these changes Nov 15, 2024
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.

There's a lot less to review here than I originally thought, though I'm mostly just assuming the autogenerated files are fine.

I don't really have much to criticize here. The new sim utilities look very useful and I look forward to using them even outside of BEAMS

I really appreciate all the comments / explanations, I think we'll be very grateful for them in the future.


The results from this should be fed into the test_ioc.py.j2 jinja2 template file.
"""
ctx = Context()
Copy link
Contributor

Choose a reason for hiding this comment

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

there are too many contexts. I see this is not quite the same as pyepics' context, we just use it to gather PVs

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually a close analog of the epics context in a lot of ways, just like pyepics's context (I think?) but the python apis are quite different between the two

print(exc)

# This is probably overkill but it does save some time for large trees
with Batch() as batch:
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL this exists. This is a neat little alternative if a library is using caproto.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I decided to use caproto to measure since I was generating a caproto script anyway, so I assumed it would grant some consistency advantages. I'm not sure this assumption ended up being true but I learned a few tricks from the docs this time.

pv_info = default_pv_info(all_pvnames)
else:
pv_info = collect_pv_info(all_pvnames)
sorted_pv_info = sorted(pv_info)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter that we sort the PVs? Or is it just for ease of reading through the rendered template

Copy link
Member Author

@ZLLentz ZLLentz Nov 16, 2024

Choose a reason for hiding this comment

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

Two reasons:

  • In this implementation, the PVs are in essentially a random order (from the batch call) and I wanted a deterministic order to make regenerations of the same script consistent
  • Easy of reading as you say

ENUM = pvproperty(
value=2,
dtype=ChannelType.ENUM,
enum_strings=["e", "i", "pi"]
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as defaults go I might pick something that doesn't collide with the STRING default

Copy link
Member Author

Choose a reason for hiding this comment

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

that's fair, I wanted all the values to be pi and took it too far

capsys: pytest.CaptureFixture,
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the most python-y test I've seen in a while haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants