-
Notifications
You must be signed in to change notification settings - Fork 9
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
Autosave support #163
Autosave support #163
Conversation
Just tested with all the builder record types, looks like it's not happy with waveform record types (numpy array not serialisable to json), will either have to cast these to lists or try dumping to a yaml file, possibly with custom representers. Or use some sort of pickle-type approach, but probably preferable to be human readable. |
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.
Looks good overall, but I've raised a few questions about what exactly the API could/should be in various comments.
There's several bits that need doing:
- Tests; at least a couple of system-level tests, then as much unit testing as you feel appropriate. Any test that starts an IOC will need to use the
multiprocessing
mechanism, as various parts of EPICS won't allow multiple IOC initializations in a single thread so a new one must be used for each IOC. A simple template is probablytest_record_wrapper_str
intest_records.py
. - Providing example code, either inside the existing
docs/examples/example_*.ioc.py
file(s) or in a separate new one - Docs; Probably fine to make a new
how-to
document explaining how to configure and use autosave. - A
CHANGELOG
entry - take a look at previous entries to see the format/syntax.
I have added support for autosaving non-VAL fields, which is done by calling EDIT: Have realised that alternatively we could force DISP=0 for In type records that require fields to be tracked in autosave. |
a0fbd0f
to
fac2508
Compare
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.
The overall code and structure seems pretty good now. Unfortunately there's a number of bits causing problems, and a handful of tasks that still need doing.
There's a lot of CI test failures, some of which are due to the Pipfile.lock change, and some of them to do with the autosave code.
For the Pipfile changes, there's a comment discussing whether they are needed or not.
For the other CI fails, I found some errors like this in a few places:
----------------------------- Captured stderr call -----------------------------
2024-07-16T09:07:11.552312123 WARN pvxs.tcp.setup Server unable to bind port 5075, falling back to [::]:39597
Process ForkServerProcess-5:
Traceback (most recent call last):
File "/opt/python/cp38-cp38/lib/python3.8/multiprocessing/process.py", line 315, in _bootstrap
self.run()
File "/opt/python/cp38-cp38/lib/python3.8/multiprocessing/process.py", line 108, in run
self._target(*self._args, **self._kwargs)
File "/project/tests/test_record_values.py", line 384, in run_ioc
builder.LoadDatabase()
File "/tmp/tmp.ZezuRo2hXU/venv/lib/python3.8/site-packages/softioc/builder.py", line 303, in LoadDatabase
autosave.load()
File "/tmp/tmp.ZezuRo2hXU/venv/lib/python3.8/site-packages/softioc/autosave.py", line 70, in load
Autosave._load()
File "/tmp/tmp.ZezuRo2hXU/venv/lib/python3.8/site-packages/softioc/autosave.py", line 186, in _load
cls._backup_sav_file()
File "/tmp/tmp.ZezuRo2hXU/venv/lib/python3.8/site-packages/softioc/autosave.py", line 117, in _backup_sav_file
sav_path = cls._get_current_sav_path()
File "/tmp/tmp.ZezuRo2hXU/venv/lib/python3.8/site-packages/softioc/autosave.py", line 139, in _get_current_sav_path
return cls.directory / f"{cls.device_name}.{SAV_SUFFIX}"
TypeError: unsupported operand type(s) for /: 'NoneType' and 'str'
So it seems there are some cases where the autosave code is being initialized with invalid values.
The extra bits of work that still need doing:
- Documentation.
api.rst
needs extending to include the new keywords added to record initialization, and then we probably also need a newhow-to
doc to explain autosave and its mechanisms. - A CHANGELOG entry - copy a previous one and change the info, as the syntax is quite picky!
- Possibly a new example python IOC using autosave (or modifications to an existing one)
- At least one full-system IOC test that actually starts an IOC from a canned autosave file and proves it loads the new values. Feel free to ask me when you get to this task as it is quite complicated to make it work.
121364d
to
eb4025a
Compare
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.
Looks good, have a few questions to look at.
# Create records, set some of them to autosave, also save some of their fields | ||
|
||
builder.aOut("AO", autosave=True) | ||
builder.aIn("AI", autosave_fields=["PREC", "SCAN"]) |
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.
An interesting thought here: builder
IN records must have .SCAN
set to "I/O Intr"
for proper operation (otherwise .set()
won't work properly), so using "SCAN"
as a saved field isn't the best example! I'd suggest either "EGU"
or some of the alarm threshold fields.
softioc/autosave.py
Outdated
self._last_saved_time = datetime.now() | ||
|
||
@classmethod | ||
def _backup_sav_file(cls): |
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.
Why not use __
prefix for local names? This is the normal Python style for member names, and it triggers automatic name mangling to hide the names from outside the class.
I will spend some time this afternoon looking back over original autosave and see if there's any features of that worth implementing. I'm not sure if there's any meaningful reason to try and implement things like the asVerify and the pass0/pass1 restoring of values before and after record initialization, but there are a few things like manual save restore that could be more relevant. I wonder if using the name autosave implies a sort of feature parity that we don't necessarily want |
9b800b0
to
1b4497f
Compare
One last feature request: It may well be useful to provide a context manager, that can automatically do Autosave for all PVs declared within it. Possibly also supporting a specific list of fields, something like this:
And this will autosave |
I'll push fixes to the above comments soon, I've got lots of things in branches at the moment! The most straightforward way I could think to do this was to define a context manager in builder.py like this
and have aOut etc call _set_user_defaults(fields). This is generic enough that it could be used for any EPICS field like EGU etc, would that be something we want? Or would it be better to rework the Autosave interface to work as a context manager?
|
I would prefer to see the Autosave class used as the context manager. As we don't have to run any code during the creation of each PV, what we can do is track how many PVs were created during the lifetime of the context manager and then add them in bulk to the Autosave list. Something like this (untested) code:
|
Got it, I think this specific implementation would be problematic because of circular imports, but I can see if I can come up with something that achieves the same thing. Of course could maybe avoid the import problem by importing within a class/function scope but could be a bit of a code smell. |
The simplest fix for that would be to replace from softioc.device_core import LookupRecordList with from softioc import device_core and invoke as |
reset device name between tests
stop autosave IOC test from running forever
55bb23c
to
4b76b8d
Compare
4248754
to
8b6413b
Compare
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.
Looks good to me!
The test failures are due to p4p
issues with later versions of Python and numpy2.0. They will be fixed when p4p
is updated.
Great! I don't seem to have permissions to merge but happy to have it squashed with a message like "add support for saving and restoring pv fields with autosave" |
3b0bef0
into
DiamondLightSource:master
Addressing #162
Adding in support for EPICS autosave style periodic saving of PV values and fields to a yaml-based backup file, from which initial values can be loaded on a restart.