ENH: add getPVAliases and grep_more_ioc scripts #180
Conversation
There was a problem hiding this comment.
Nice work, seems like this should be very helpful! I've left a couple nitpicky notes, I'll take a bit to look at this from a higher level after this.
- I like to avoid regex when possible. It's powerful but often difficult to maintain and parse (by humans). There's a few places here where I don't think you need it.
- Thanks for the comments / docstrings! Love to see it.
aberges-SLAC
changed the title
| # --------------------------------------------------------------------------- # | ||
| # print the dataframe | ||
| if hasattr(args, 'print'): |
There was a problem hiding this comment.
I think this is probably not the recommended way to determine which subparser is being invoked, but I can't immediately come up with a better way 😞
There was a problem hiding this comment.
I think the sparse nature of argparse (:badumtiss:) including only the invoked parser & subparser made it weird for testing, and I got distracted by other things and didn't leave a note to clean up.
I should probably do something with subparser.set_default to give me some namespace clues. Something like subparser.set_default(command='print') etc.
There was a problem hiding this comment.
Turns out my original approach is one of the few stable ways to check for other possible args. When argparse parses the args, if any subparser is unused, none of its attributes get populated in the resultant Namespace object.
I guess the other way of doing this would be generating a full Namespace object with all of the defaults set and to check the bool of any sub-commands because I already structured them as simple toggles at the top level.
Something like:
class Args:
self.patt = None
self.hutch = None
self.ignore_disabled = False
...
But then I'm hard coding all the defaults anyway that argparse was originally handling. 🤷
|
I'm starting to feel like this has been stuck in review cycle for longer than it needs to be relative to how complete the code is, so I'm going to try to focus my reviews on bigger things related to deploying/sharing/using the scripts. I can't wait to see this in engineering tools proper. |
|
I just caught a bug that's worth fixing before merging as well. Happens when you I'm really just commenting this here to remind myself to use |
…r function to grep_more_ioc to circumvent this
|
Okay, I fixed the bug when searching IOCs that had |
What's the story here? Maybe we need to make some of the pre-commit entries ignore the readme? |
It complained loudly at me for markdown syntax that I never edited, and then seemingly wanted to remove lines like Just let it its thing and committed the pre-commit changes a few tries ago. Now that it is happy I'm hesitant to make any further modifications. Besides, its much cleaner on the web interface anyhow. |
|
The README.md probably hadn't been pre-commit-checked, since our checks only run on modified files. |
| <tr> | ||
| <td>grep_more_ioc</td> | ||
| <td> | ||
| usage: grep_more_ioc [-h] [-d] patt hutch {print,search} <br/> |
There was a problem hiding this comment.
The script is actually grep_more_ioc.sh, I think we should document as such here or rename the scripts to not have .sh suffixes.
There was a problem hiding this comment.
I'd vote to not have .sh as most scripts in engineering tools do not have extensions.
There was a problem hiding this comment.
To hop on to the readme suggestions, could you add an example call for those of us who don't know what pattern/hutch to search for? I think examples are helpful for people not so familiar with EPICS and our systems
There was a problem hiding this comment.
I think if this script renaming/examples stuff get in then this PR has my approval (I don't want to hold it up any longer, but I'd like the names to be finalized + I'd like people to be ready to use it when it gets merged)
README.md
Outdated
| patt Regex str to search through iocmanager.cfg<br/> | ||
| hutch hutch to search<br/> |
There was a problem hiding this comment.
These two are documented well here but not in the --help text for either script. I don't think patt is self-describing enough.
There was a problem hiding this comment.
$ grep_more_ioc.sh --help
usage: grep_more_ioc [-h] [-d] patt hutch {print,search} ...
Transforms grep_ioc output to json object and prints in pandas.DataFrame
positional arguments:
patt
hutch
{print,search} Follow-up commands after grep_ioc executes
print Just a simple print of the dataframe to the terminal.
search For using regex-like searches in the child IOC.cfg captured by grep_ioc. Useful for quickly gathering instance information, IP addr, etc.
There was a problem hiding this comment.
I can improve this in the argparse documentation so it is more transparent.
The annoying thing about how argparse handles subparsers is that each subparser has its own help text, so something like:
grep_more_ioc.sh . all print --help triggers the help text for print
grep_more_ioc.sh . all search --help triggers the help text for search
Personally, I hate it, but I haven't found a good enough work around yet
There was a problem hiding this comment.
One simple thing you could do is add some text in the top-level parser that hints the user to try those sub-parser help messages.
scripts/getPVAliases.py
Outdated
| print(build_table(alias_dicts, ['record', 'alias'], align='l')) | ||
| # optional skip for all resulting PV aliases | ||
| save_all = (simple_prompt( | ||
| 'Do you want to save all resulting PV aliases? ' |
There was a problem hiding this comment.
What is this message telling me as a user? What is being saved and to where?
There was a problem hiding this comment.
I should update this docstring to "Do you want to add all resulting PV <--> alias associations to the final output?"
The script now saves a single file with all the results appended together (less annoying to parse when you're playing with the result later). Since you're iterating through a base PV and its resulting PVs (each with aliases), instead of pestering the user to approve every single instance (e.g. each axis in an MCS2 ioc), the user can approve if the first association looks good.
There was a problem hiding this comment.
I think if the script saves a file we should mention this in the help text.
There was a problem hiding this comment.
More informative?
There was a problem hiding this comment.
It's definitely better but I think my overall confusion was a lack of understanding that this script was supposed to save anything at all.
script name "get pv aliases", help text "gathers all record <-> alias associations" = confusion about a save prompt
There was a problem hiding this comment.
With some massaging on argparse.RawTextHelpFormatter:
There was a problem hiding this comment.
Looks good to me! Very helpful output!
There was a problem hiding this comment.
It seems like this script doesn't do anything unless I also select print or search, is this the expected behavior?
There was a problem hiding this comment.
Yes, they are positional arguments. I contemplated making grep_more_ioc.sh <patt> <hutch> default to print but it gets hairy with the subparsers.
There was a problem hiding this comment.
Also improved:
|
I ran the scripts myself and asked the questions that I had as a user, maybe they inform documentation/examples/etc. |
|
Okay, all help doc-text improved, committed, and pushed! |
There was a problem hiding this comment.
The final tiny thing before merge is the script names, the majority of the executables in engineering_tools don't have the .sh extension, so I suggest renaming these to remove it.
It's worth noting that you've already documented this without the .sh extension in the readme, so renaming things makes everything consistent.
Otherwise, if there's a good reason to keep the .sh, it should be documented as such in the readme.
Added getPVAliases to README and fixed formatting
Done and done! see new commits |
|
I'm going to merge this now. Post-merge if there are other concerns we can handle them in smaller, reviewable follow-up PRs. We can either tag/deploy this immediately or we can wait, let me know what you all prefer. |
I'm excited to get this out for folks to use & stress test. I'm sure there's more bugs hiding somewhere that's outside of my normal use cases. So I'm in favor of deployment sooner rather than later — the script is pretty stable for now. Are there any confluences pages I should update as well? Or do we leave the documentation in the README.md? |
|
I'll release another version right away then- tags are cheap. Do you want to announce the release of R3.8.0 (which, compared to last version, only has this PR) or would you prefer I do it? I don't think there's any confluence page in particular that needs to be updated, unless you think a bigger page about usage etc. would be useful. |
Description
Added a script for reconstructing PV aliases from their child IOC's,
st.cmdfiles, and.archivefiles for use in ArchiverAppliance scripts. Does a pythonicgrep_iocfor the regex pattern supplied, then searches the child IOC for the parent's release pointer. After confirmation, reconstructs all the record ←→ aliases using.../db/*.archiveandst.cmdfrom the child IOC.Also added a script called
grep_more_iocwhich is an a personal utility inspired bygrep_ioc. Most of the legwork of this script is reused ingetPVAliasso I decided to bundle it for posterity.Motivation and Context
Motors, namely SmarActs and Elliptecs, tend to move around to different areas often and/or have dynamic configurations for channels based on the area's application. This results in PV aliases becoming stale references to channels that no longer house that stage. To rectify this, you'll need to get the currently built association of EPICS records ←→ aliases for the PVs.
Additionally, I added some QOL in
grep_more_iocthat helps day-to-day controls work in CLI without having to open theiocmanagergui.How Has This Been Tested?
I've run this script & wrapper in an ipython session and using the supplied bash wrapper. Please see the screenshots below!
Where Has This Been Documented?
All documentation is supplied inline on the scripts with accompanying usage/help doctext.
Screenshots (if appropriate):
getPVAliases
Initial confirmation of ioc caught by a
grep_ioclike search:All axes/motors generated by the child IOC's
st.cmdfile:Preview of what will be saved to the
.txtfile dumps:Assumes current directory as the base for new folder generation & dump, but accepts different directories:
Makes the folder if it does not exist, then iterates to the next IOC:
Individual axes can be skipped, with lazy skipping through the rest of the axes:
--> Otherwise if you don't accept all axes dumped, (or lazy accept/skip the rest), you will be asked to confirm each dump:
Example of output:
grep_more_ioc
This script uses
pandasto print DataFrames, which is quite slower thanprettytablebut doesn't suffer the wrapping/width issues that the latter does. (see below)There are 2 major functions:
printandsearch. In my examples below I'll use the aliasgmiforgrep_more_iocprint
Options:
skip_commentsFunctions like
cat, except you skip commented code. Useful for quickly grabbing IOC information when buildinghappientries.releaseSearches the child IOCs for the release pointer, then adds an abbreviated path to the parent's current release.
print_dirsDumps directories to terminal for quicker copy/pasting/
pushdsearch
Options:
quietSuppress warnings for paths that don't exist when doing batch searches (as disabled IOCs are often previously deleted...)
onlyOnly print the search results, don't print the DataFrame.