pcdshub / pmps-ui

User interfaces and diagnostics for PMPS
Other
0 stars 9 forks source link

REF: apply a proper python package skeleton to pmpsui #100

Closed tangkong closed 9 months ago

tangkong commented 9 months ago

This one's a bit of a doozy. I think this is going to be helpful moving forward, especially as I try to start contributing.

So what happened:

I am not confident that this is the cleanest way to invoke PyDMApplication, but I think it's probably worth addressing any deficiencies in a followup, since this diff is already irritating enough.

After this though we'll have test checkmarks and proper PR templates yay!

ZLLentz commented 9 months ago

I am not confident that this is the cleanest way to invoke PyDMApplication, but I think it's probably worth addressing any deficiencies in a followup, since this diff is already irritating enough.

I had at some point created https://github.com/pcdshub/pydm-application-template which needs to be re-done and maybe whatever we decide on can be used here. Or maybe this can be pulled into that, I have no idea.

I'll review this PR now.

tangkong commented 9 months ago

That seems similar to what I landed on, which is simultaneously relieving and annoying (that I didn't know about it earlier)

ZLLentz commented 9 months ago

Also, I suspect that the rsync script is totally non-functional after this- maybe we can resolve it in a follow-up https://github.com/pcdshub/pmps-ui/blob/master/nfs_afs_sync.sh

tangkong commented 9 months ago
  • lucid screens have a raw command pydm --hide-nav-bar --hide-menu-bar --hide-status-bar -m "CFG=KFE" pmps-ui/pmps.py --no-web that fails due to a bad path. Putting in the correct path still fails:
self.user_args = argparse.Namespace(no_web=args[0], log_level=args[1])
IndexError: list index out of range
  • the e.g. lpmps shortcut does a similar command pydm --hide-nav-bar --hide-status-bar -m "CFG=$1" pmps.py --no-web & which would also fail.

So, unless there's some magic way to backcompat, we'll need to change those commands appropriately.

I think I want to make this work in this PR, as maintaining backcompat was one of my original goals. I'll focus on this and try to address the other comments in followups

tangkong commented 9 months ago

Thanks for checking the existing entrypoints @ZLLentz. Can you try the pydm entrypoints on your side one more time? The following works for me but I'm never sure what craziness my python environment is hiding

pydm  --hide-nav-bar --hide-menu-bar --hide-status-bar -m "CFG=KFE" pmpsui/pmps.py --no-web

If this works, I think the softlink should be sufficient right?

ZLLentz commented 9 months ago

That command works, but that's not exactly what the existing entrypoints use. The existing entrypoints are gross and inconsistent btw, fair warning. Once this makes it into a pcds_conda tag I'm absolutely going to switch them all to using the command directly.

What the existing entrypoints use can be boiled down to:

Option 1: lucid screens

# Add the dev displays folder to the displays path (from within pcds_conda or dev_conda source script)
export PYDM_DISPLAYS_PATH=/cds/group/pcds/epics-dev/screens/pydm
# Run a pydm command
pydm  --hide-nav-bar --hide-menu-bar --hide-status-bar -m "CFG=LFE" pmps-ui/pmps.py --no-web

Option 2: lpmps/kpmps shortcuts

# Go to the pmps-ui clone in the dev displays folder
pushd /cds/group/pcds/epics-dev/screens/pydm/pmps-ui
# Run a pydm command
pydm --hide-nav-bar --hide-status-bar -m "CFG=LFE" pmps.py --no-web &

Neither of these work because the symbolic link is pmps-ui.py instead of pmps.py. We can test these by replacing the path with the equivalent path in our local clones.

It looks like renaming the symbolic link was enough to get these (and the launch.sh script) to work for me using this repo (note: run the tests using LFE instead of KFE because it loads faster)

ZLLentz commented 9 months ago

Side note: it looks like TST_config.yml has been abandoned in the root directory here. I think it probably belongs with the real configs.

tangkong commented 9 months ago

Ok, if we're committed to changing the existing entrypoints I'll not spend more time trying to leave this as is. I will move the tst config though 👍

tangkong commented 9 months ago

Let's goooooo