-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
… attr name from Value
3bdff50
to
f5d63a8
Compare
I'm intending to add a copy-the-ioc test and a pre-release docs before asking for reviews. |
tests go boom :( |
I should have caught that one, I forgot to re-run the tests after making "just one small change" |
There was a problem hiding this 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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
capsys: pytest.CaptureFixture, | ||
monkeypatch: pytest.MonkeyPatch, | ||
tmp_path: Path, | ||
): |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
There was a problem hiding this 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)
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.
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. |
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. |
Description
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
docs/pre-release-notes.sh
and created a pre-release documentation page