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

Merged
merged 38 commits into from
Nov 18, 2024

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
  • Ease of reading as you say

beams/tests/mock_iocs/various_types_ioc.py Outdated Show resolved Hide resolved
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.

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.

Approving, mostly because I think it's good. Totally not because I have a vested interest in getting my annoying logging PR in soon

Copy link
Collaborator

@joshc-slac joshc-slac left a comment

Choose a reason for hiding this comment

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

In the most ideal world this may be two seperate PRs, but I don't practice what I preach myself.

sim ioc generation looks great, love the visualizer for the current tree.

My only concerns need not be addressed in this PR:

  • shell files kept within the repository, intuitively wrong to me. tethers us too deeply to our file system and prod. how can we better share / utilize these helper scripts needed for prod environment? Maybe just org change within the repo if we want ot keep them version controlled.
  • mfx_tree gen file is great, but long, maybe we can ask ourselves how we want to better facillitate the scripting of these things or if we want to have a codestyle for how trees are confederated (very future facing)

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 18, 2024

shell files kept within the repository, intuitively wrong to me. tethers us too deeply to our file system and prod.

You're absolutely correct here... definitely the parts that could/should be broken out into beams entrypoints and the parts that assume too much lcls-specific stuff (the environment setup). I'll ruminate a bit.

mfx_tree gen file is great, but long, maybe we can ask ourselves how we want to better facillitate the scripting of these things or if we want to have a codestyle for how trees are confederated (very future facing)

I think this is immediately super relevant because most nontrivial trees are going to be at least this long. Figuring this out during out first few built-outs might be appropriate but having guidelines early will save everyone some grief.

@ZLLentz ZLLentz merged commit 32de05f into pcdshub:master Nov 18, 2024
7 checks passed
@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 18, 2024

Possibly the best way to do examples is more like caproto where the examples are also entrypoints (and less like PyDM, which is what I did here). Then, you would be able to install beams and be ready to roll with any example.

@ZLLentz ZLLentz deleted the enh_mfx_example branch November 18, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants