Standardise record value setting/getting #60
Conversation
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
==========================================
+ Coverage 85.39% 88.49% +3.09%
==========================================
Files 13 13
Lines 815 904 +89
==========================================
+ Hits 696 800 +104
+ Misses 119 104 -15
Continue to review full report at Codecov.
|
|
PR parked pending removal of Python2 support: This will mean we won't need to handle Unicode in Python2. |
AlexanderWells-diamond
force-pushed
the
sort_setting_values
branch
from
09581d3 to
fd26bac
Compare
|
@Araneidae @thomascobb This PR is now ready for review. |
There was a problem hiding this comment.
Most of my comments so far are layout related.
I'm getting a bit distracted by inconsistent layout and use of spaces around =. I'm guessing that spaces in function definitions is a lost call, but you'll find that all the preexisting code always uses spaces in all other contexts, particularly in function calls.
Secondly, please use 4 character indent except in those incredibly rare cases where it comes out to be particularly ugly. You can always use a doc-string to make function argument indentation to look sensible.
I'd like the record device definitions to look more uniform and be easier to read, possibly even drop all(?) defaults? Anyhow, try and get a layout that's easier on the eye here.
There's plenty of functionality for me to look at ... I'll get to it ...
AlexanderWells-diamond
changed the title
|
The latest additions to the testware bring the number of tests up to ~500. This takes 5 minutes on my machine, and up to 15 minutes on a CI runner. This is getting to the stage where it's too long. There's also now non-trivial amounts of test code, to account for the wide variety of situations encountered (pythonSoftIOC issues, differing behaviour between In and Out records, differing behaviour of Waveforms, many individual tests skipped, etc.). I'll look at refactoring these tests into larger tests that should hopefully run quicker (but not needing to spawn a new |
Note the mechanism used needs expanding to all other record types.
Accept that passing integers into Waveforms always gives a float array Condense three disparate filterings into one list
See note in the print() statement for explanation. Also added tests for floating point list passed to waveform
Also check and abort writes that are too long for the waveform
Done after seeing a package version conflict in CI between aioca, p4p and softioc's desired version of the epicscorelibs library.
This reverts commit 42332fb.
Required due to dependency version conflicts: aioca 1.3 requiring epicscorelibs>=7.0.3.99.4.0 p4p 3.5.4 requiring epicscorelibs<7.0.6.99.2 and >=7.0.6.99.1.0 p4p does have pre-release versions in the 4.0* stream, so this issue may fix itself once those become full releases
This reverts commit 7ce0246.
Required due to dependency version conflicts: aioca 1.3 requiring epicscorelibs>=7.0.3.99.4.0 p4p 3.5.4 requiring epicscorelibs<7.0.6.99.2 and >=7.0.6.99.1.0 p4p does have pre-release versions in the 4.0* stream, so this issue may fix itself once those become full releases
…oc into sort_setting_values
Also cleaned up sim_records - the test_records test was failing due to test reorganization - there's issues with creating records during a module import so now we have a method to create the required records.
There was a problem hiding this comment.
This is looking very good.
I did notice a number of new lines not covered by tests. Given how comprehensive the tests already are now, I wonder if it would be worth adding a few more tests to cover these lines.
Finally, the documentation comment on byte strings makes me wonder whether we should have special support for bytes? I think this is in just one place in the code: https://github.com/dls-controls/pythonSoftIOC/blob/98de8f675cb51bd85ffda88dd162dfdbd83de2a3/softioc/device.py#L322-L325 --- I'm ever so tempted to simply drop this special case!
pyproject.toml
Outdated
| @@ -1,3 +1,3 @@ | |||
| [build-system] | |||
| requires = ["setuptools", "wheel", "setuptools_dso>=2.1", "epicscorelibs>=7.0.6.99.1.0"] | |||
| requires = ["setuptools", "wheel", "setuptools_dso>=2.1", "epicscorelibs==7.0.6.99.1.0"] | |||
There was a problem hiding this comment.
This strong tie to a very particular version of epicscorelibs rings an alarm bell for me. What's going on here? Surely we don't want this, do we?
Separately, the version number is in an alarmingly non-standard format!
There was a problem hiding this comment.
This strong tie to a very particular version of epicscorelibs rings an alarm bell for me. What's going on here?
There is an ordering issue here. The build process is this:
- deps from pyproject.toml are installed
python setup.py bdist_wheelis called to make a binary wheel, which is compiled against the exact version of epicscorelibs that has been installed, and records this fact in the wheel- the wheel is uploaded to PyPI
- anyone using this version of the binary wheel must have this exact version of epicscorelibs installed too (which pip will do automatically)
- anyone using the sdist is free to have whatever version of epicscorelibs they want
The >= that was there originally was correct, which would mean that pythonSoftIOC was compiled against the latest available epicscorelibs. Unfortunately in the tests we require p4p, which is also compiled against a particular version of epicscorelibs. If a new version of epicscorelibs is released, but p4p is not, then pip will fail to install dependencies. This is why we are using ==. Without ABI compliance for EPICS, I can't see a way around this, unless a release of p4p is made with every epicscorelibs release. Fortunately we don't care for aioca as we only need API compliance, not ABI compliance.
@mdavidsaver can you think of a way we can resolve this?
Separately, the version number is in an alarmingly non-standard format!
The first part is the EPICS release, but not sure about the other bits. Maybe @mdavidsaver could comment on this too?
There was a problem hiding this comment.
If a new version of epicscorelibs is released, but p4p is not
Sorry. I've been sloppy about this as I'm in the (so far long) process of introducing another dependency pvxslibs. I'll try to straighten this out. Though even if I do, there will always be some time when pypy.org has newer versions of parts of the dependency chain. Maybe only minutes, but never zero.
we only need API compliance, not ABI compliance.
Speaking strictly, API is important at build time (with a compiler), and ABI afterwards. So in an ideal world, pyproject.toml is expressing the build time dependency (BuildRequires in an RPM .spec file), while install_requires in setup.py expresses the runtime dependency (plain Requires in a .spec). As with a .spec runtime dependencies are partly generated. so...
So if epicscorelibs 7.0.6.99.1.0 is installed, this will expand to:
epicscorelibs >=7.0.6.99.1.0, <7.0.6.99.2
Separately, the version number is in an alarmingly non-standard format!
There are three pieces of information being encoded: upstream EPICS Base version (everything up to and including the .99), epicscorelibs ABI, and epicscorelibs revision. So the version range being emitted above allows for later revisions which (hopefully) don't include any ABI change.
There was a problem hiding this comment.
@mdavidsaver can you think of a way we can resolve this?
If the problem is only with GHA, you might be able to pass --no-binary p4p to pip when installing the dev dependencies prior to running tests.
There was a problem hiding this comment.
Thanks @mdavidsaver it looks like p4p 3.5.5 fixes the issue - I've restored the original epicscorelibs dependency in this commit and it seems to be working: adaa286
Spoke too soon, the Code CI works but the Docs CI breaks with this change... Investigating...
@Araneidae Much of the missed code was actually being run already, it just wasn't being picked up by Codecov due to it happening in a |
With the release of p4p 3.5.5 there is no longer a version conflict. See discussion here: #60 (comment)
|
@AlexanderWells-diamond , I'm happy for you to merge this when you're ready now. |
Looks like the last version of this file is very old - aioca was still at version 1.2 despite being 1.3 in setup.cfg for a long time.
Taken from email from Tom Cobb
This PR changes the handling of types for all records, on both setting and retrieval. Broadly, records are now more picky about what types they will and will not accept, and exceptions will generally be raised if invalid values are put to a record.
This PR also introduces two new records,
longStringInandlongStringOut, which are specialized versions ofWaveformrecords designed to handle Python Unicode strings. See documentation for more details.A complete list of valid types for each record is most easily found from looking at the tests - see
record_values_listfor the list of cases which are tested.Closes #39
Closes #40
Closes #43
Closes #55