[LLM-authored] EPICS record alias support to softioc - #210
Conversation
Adds three ways to create EPICS record aliases: an `alias` keyword argument on record creation functions (aIn, aOut, etc.), a standalone Alias(name, alias_name) function for aliasing an existing record by name, and AddDeviceAlias(prefix) for automatically aliasing every record subsequently created under the current device with an alternate prefix. Device-alias-prefixed names are generated for a record's own aliases too, always pointing directly at the real record rather than chaining through another alias. Built entirely on epicsdbbuilder's existing alias primitives, with no changes needed to epicsdbbuilder or epicscorelibs. PV-naming mistakes (unknown alias targets, name collisions between records and aliases in either direction, oversized generated names, and cases where a device alias prefix can't sensibly apply to a name) are validated eagerly and raise immediately, rather than being deferred to LoadDatabase(). Also adds the `dbla` command to the interactive_ioc shell for listing record aliases, matching the existing dbl/dbgf/etc. commands. Co-authored-by: Philipp Klenze <p.klenze@gsi.de> Co-authored-by: Claude <noreply@anthropic.com>
AlexanderWells-diamond
left a comment
AlexanderWells-diamond
left a comment
There was a problem hiding this comment.
First of all, thank you for taking the time to write a PR. We are not militantly anti-AI and so are happy to discuss this proposal.
Could I please request an Example file is written, in the docs/examples directory, to show how to use the various different Aliasing methods.
At some point the CHANGELOG file needs to have conflicts fixed - I recently did a new release.
I have gone through the code and provided review comments, but there are some more major questions to discuss.
-
We are uncertain of the benefit of many of the features added here. Adding the kwarg to record creation seems acceptable, but is ultimately just a wrapper for
record.add_alias(alias). We can't see a use of the arbitrary addition of Aliases at arbitrary times. The automatic DeviceName aliases are again not something we can see the use of.
Can you please provide some use cases for these? -
The complexity of the changes. Adding two separate collections (
_device_aliasesand_alias_names) increases complexity and the liklihood to the state of one or both not being cleaned properly, or leaking out at some future time.
In principal the_alias_namesdata already exists inepicsdbbuilder- albeit you have to iterate over all records and examine all of their__aliasesfields. To do this properly would probably require a change toepicsdbbuilder. However, this also ties into my next question: -
Is all of the up front error handling necessary? I can understand wanting to identify the issue as soon as possible, but what does it look like if we do not do those pre-emptive checks and let the database be loaded. Are the errors obvious enough that we can ignore the up front checks?
| current device name, or as an absolute name if it contains a colon. | ||
|
|
||
| .. seealso:: | ||
| `Alias` for creating an alias for an arbitrary existing record. |
There was a problem hiding this comment.
This links back to the "alias" title just a few lines above. It also produces a warning when built, which explains why it's done that.
Run pipenv run docs to see the warning.
| that only exists in some other already-running IOC, is not supported. | ||
|
|
||
| .. seealso:: | ||
| `alias` for adding an alias at the point a record is created. |
There was a problem hiding this comment.
Another ambiguous target, that also produces a warning when built.
| The target record must already have been created earlier in this | ||
| session; aliasing a record that will only be created later, or one |
There was a problem hiding this comment.
We don't really have a concept of a "session" defined anywhere. Best not to mention it and simply say "must have been created earlier".
| def _validate_length(full_name): | ||
| # GetRecordNames() already length-checks relative record names as they | ||
| # are resolved, but that's not the only way a name reaches us: absolute | ||
| # names bypass it, and so does device-alias-prefix substitution (which | ||
| # builds a name by string surgery, not by resolving it). Applied | ||
| # uniformly here so every alias name is checked no matter how it was | ||
| # constructed, catching e.g. a pathological AddDeviceAlias prefix that | ||
| # would otherwise generate a name long enough to crash the EPICS | ||
| # database parser instead of failing cleanly in Python. | ||
| max_length = GetRecordNames().maxLength | ||
| shown = full_name if len(full_name) <= 80 else full_name[:80] + '...' | ||
| assert 0 < len(full_name) <= max_length, \ | ||
| 'Record name %r too long' % shown |
There was a problem hiding this comment.
This should really be a common function, probably inside of epicsdbbuilder, rather than a very brittle implementation of it being placed here.
Also I'm not sure I understand the value of cutting the record name to 80 characters. That's already too long for EPICS, too long for a standard 80 character terminal.
| caught by epicsdbbuilder itself.)''' | ||
| full_name = resolve_name(name) | ||
| assert full_name not in _alias_names, \ | ||
| 'Alias %r already defined' % full_name |
There was a problem hiding this comment.
Is there a need to use %r rather than just %s? I don't particular see the need to use the repr() representation of a string argument.
This comment applies to many places in this code.
| SetDeviceName), or as absolute names if they contain a colon. | ||
|
|
||
| The target record must already have been created earlier in this | ||
| session.''' |
There was a problem hiding this comment.
Another use of the word "session" that I would like removed.
| # ----------------------------------------------------------------------- | ||
| # Support for AddDeviceAlias. Like SetDeviceName, this only affects | ||
| # records (and their own aliases) created after it is called. | ||
|
|
||
| # Alias prefixes registered for the current device. |
There was a problem hiding this comment.
This whole section is very closely related to the DeviceName, but being kept completely separate leads to a lot of complexity. I think an ideal solution would be to in some way combine this functionality into that feature - perhaps with an optional "alias" keyword on that function. But again that may require changes to epicsdbbuilder and not here.
2 participants
Oh not, the billionth AI slop PR on github. In my defense, your project does not have a clear LLM policy I could find, so I decided to just try how you would react, with a "Lol, nope. close ban" well within my expected response range.
For what it's worth, this is my first LLM-coauthored PR ever, and I spent a few iterations until I had something which I found comfortable even suggesting adding to a repository of human-readable code. (My main goal was to figure out what the bots can and can't do these days, so I got something out of this either way.)
--
For our use case, we have two different kinds of EPICS pathes:
Previously, we used epics gateways for these mappings, but now I want to switch to simple alias names. So far, pythonSoftIOC does not support aliases, adding them from user code requires keeping extra state.
Thus I have added three new functions to builder:
Alias(target, alias), which is the most general case.aliasargument inaOutand friends, for convenienceAddDeviceAlias(aliasPrefix)which is something not supported by EPICS natively (AFAIK). The idea is to populate two or more prefix namespaces at the same time (which would be most convenient for my use case).Sadly, unlike unix pathes and (to some extend) DNS, EPICS pathes do not have a clear way to distinguish absolute from relative pathes. Currently, I have the heuristic that names with a colon in them are absolute and names without them are relative, but on reflection this it would probably be better to have users prefix ':' (or another character) to indicate absolute pathes (a la unix), in alias context (though it does not work in epics generally), because PV names having additional colons in them (as in ch1:setVoltage) is a common.
This is the current state, let me know if this is something which you might be interested in or if it is a hard no for design or tool reasons.
Cheers,
Philipp
--
Claude-generated log message:
Adds three ways to create EPICS record aliases: an
aliaskeyword argument on record creation functions (aIn, aOut, etc.), a standalone Alias(name, alias_name) function for aliasing an existing record by name, and AddDeviceAlias(prefix) for automatically aliasing every record subsequently created under the current device with an alternate prefix. Device-alias-prefixed names are generated for a record's own aliases too, always pointing directly at the real record rather than chaining through another alias.Built entirely on epicsdbbuilder's existing alias primitives, with no changes needed to epicsdbbuilder or epicscorelibs. PV-naming mistakes (unknown alias targets, name collisions between records and aliases in either direction, oversized generated names, and cases where a device alias prefix can't sensibly apply to a name) are validated eagerly and raise immediately, rather than being deferred to LoadDatabase().
Also adds the
dblacommand to the interactive_ioc shell for listing record aliases, matching the existing dbl/dbgf/etc. commands.