Deploy branch status - August 2023 #41
Conversation
|
Automatic PR creation worked (as in #33) 👏 |
|
I noticed this time that the It looks like it's the .py file in master that's missing- coincidentally, if we merged master back in (and pulled back to the nfs local branch) it would satisfy the CodeQL error, which would give us a free (meaningless) green check:
|
| @@ -511,6 +511,44 @@ | |||
| "type": "pcdsdevices.happi.containers.LCLSItem", | |||
| "z": 742.4427 | |||
| }, | |||
| "comp_motor": { | |||
There was a problem hiding this comment.
If I'm allowed to be pedantic in these reviews, this is a rough name for a motor- high chance for cross-hutch name collisions. I'm not worried though since we can rename things later with low impact.
There was a problem hiding this comment.
We should come up with a way to follow up on these things and work with the maintainer (?) of the device.
For now, maybe we make issues here and ping that person / open Jira tickets?
There was a problem hiding this comment.
Small note: it's hard to track down the maintainer of a device because it isn't tracked- we don't put usernames on the edit fields and the "ioc_engineer" field is optional.
| @@ -5899,7 +5937,7 @@ | |||
| ], | |||
| "beamline": "TMO", | |||
| "creation": "Mon Jul 18 16:05:31 2022", | |||
| "device_class": "pcdsdevices.pim.PPM", | |||
| "device_class": "pcdsdevices.pim.PPMCOOL", | |||
There was a problem hiding this comment.
Side note: I wonder if there's any way to avoid this problem:
- A new class is added to pcdsdevices
- happi is updated to expect the new class
- All old code that referenced this device needs a pcdsdevices update or the load fails
It makes me wonder if we need to do versioning on these or have some other way to maintain backward compatibility in this sort of environment...
There was a problem hiding this comment.
It depends on the type of the backward-compatibility.
If it's that the new device has additional PVs - great, we can use the old device without problems.
If the new device changes previous PVs - well, not so great, typhos screens may be broken and hutch-python data acquisition may fail.
I'm not sure there's a good way of doing this, really.
I think my preferred workaround would be to do like AT2L0 and AT1K4 in pcdsdevices - having device-specific classes of the same name. We can change its parent class in dev/prod and happi doesn't need any updating.
There was a problem hiding this comment.
The "factory class" method is nice for backwards compatibility, but I'm not a fan of the obfuscation it brings. If I edit a class and want to check which devices in our db are affected, it's not immediately clear.
To be clear I do think it might be the best option, nothing better comes to mind immediately. (Other than just telling people to update...)
There was a problem hiding this comment.
I think I'm most a fan of the "device specific class" method since it's straightforward and clear- though it feels a bit strange to figure out which things belong in the specific class and which things belong in the database. Isn't part of the point of the database to include what classes devices are? Does something like a static prefix belong in a device-specific class? Does any of this even matter? I'm not sure.
|
I'm going to merge this but we should follow up on @ZLLentz 's comments above |
** This PR was created automatically ** This PR can be used to easily see when cron-based pushes to GitHub happen, and allow us an opportunity to review changes prior to merging into master.