Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make pv names consistent with web gui names #26

Merged
merged 20 commits into from
Sep 1, 2023
Merged

Make pv names consistent with web gui names #26

merged 20 commits into from
Sep 1, 2023

Conversation

evalott100
Copy link
Contributor

A copy of #18, but branched off of the improved testing branch.

@evalott100 evalott100 self-assigned this Aug 11, 2023
@evalott100 evalott100 marked this pull request as draft August 11, 2023 09:24
@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch 2 times, most recently from 6e2f9bc to 8efd743 Compare August 14, 2023 13:15
@codecov
Copy link

codecov bot commented Aug 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 95.91% and project coverage change: +6.88% 🎉

Comparison is base (e292130) 83.34% compared to head (20bac0b) 90.23%.

Additional details and impacted files 
@@            Coverage Diff             @@
##              dev      #26      +/-   ##
==========================================
+ Coverage   83.34%   90.23%   +6.88%     
==========================================
  Files           7        7              
  Lines        1075     1096      +21     
  Branches        0      173     +173     
==========================================
+ Hits          896      989      +93     
+ Misses        179       76     -103     
- Partials        0       31      +31
Files Changed Coverage Δ
src/pandablocks_ioc/ioc.py 88.76% <91.66%> (+10.05%) ⬆️
src/pandablocks_ioc/_pvi.py 94.44% <100.00%> (+24.44%) ⬆️
src/pandablocks_ioc/_tables.py 95.60% <100.00%> (-0.26%) ⬇️
src/pandablocks_ioc/_types.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@evalott100 evalott100 mentioned this pull request Aug 15, 2023
Merged
@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch from 001c3da to 0474cef Compare August 16, 2023 10:35
coretl and others added 13 commits August 16, 2023 11:39
@coretl @evalott100
Doesn't work on python 3.11, put container back to 3.10
7ded051
@coretl @evalott100
Slim down and correct dev container
88e6a1a
@coretl @evalott100
Correct PVI entries
d2434e2
@coretl @evalott100
Fix table support, needs tables branch of pvi
3cf9b62
@coretl @evalott100
Hack PV names to look like the web UI ones
acd05a4
@coretl @evalott100
Add support for long PV prefix
8e9488f
@GDYendell @evalott100
Update to use pvi 0.5
ea711d2
@evalott100
changed pvi dependency to pvi[cli]
4227263
@evalott100
adjusted unit tests to fit the new naming scheme
4727024
@evalott100
Changed the pyproject.toml to python 3.10/3.11
0c8696c 
Changed the CI to run python 3.10/3.11
@evalott100
Changed tests which use the command_queue
515c709 
to use waits, changed the
mocked_panda_standard_responses to wait
until the MockedAsyncioClient is set up
before proceeding with the test.
@evalott100
Added a wait on test_bobfiles_created
1bd195e 
to allow bobfiles time to be written
in the subprocess. Changed to pvi>=0.5
and python>=3.10
@evalott100
Fixed incorrect changes from cherry-pick
b921eb1
@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch from 0474cef to b921eb1 Compare August 16, 2023 10:42
@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch from bcfe9f8 to a3a2623 Compare August 16, 2023 14:29
@evalott100
Removed
eb0ea2a 
`_ensure_block_number_present(label)`
and moved the suffix block calculation
out of an inner loop
@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch from a3a2623 to eb0ea2a Compare August 16, 2023 14:29
@evalott100 evalott100 marked this pull request as ready for review August 16, 2023 14:30
@evalott100 evalott100 mentioned this pull request Aug 16, 2023
Closed
AlexanderWells-diamond
AlexanderWells-diamond requested changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@AlexanderWells-diamond AlexanderWells-diamond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes all look fine to me, although note I don't know much about PVI.

There's a few bits I think need cleaning up regarding testing. If I understand it correctly, we now have no tests that include numbered blocks. I think we need at least one, just to prove that mechanism works.

src/pandablocks_ioc/_tables.py Show resolved Hide resolved
src/pandablocks_ioc/ioc.py Show resolved Hide resolved
src/pandablocks_ioc/ioc.py Show resolved Hide resolved
tests/test_ioc_system.py Show resolved Hide resolved
tests/test_ioc_system.py Outdated Show resolved Hide resolved
tests/test_ioc_system.py Show resolved Hide resolved
@AlexanderWells-diamond
Copy link
Contributor

On a related note, have you seen the discussions happening in Ophyd at the moment regarding similar changing of block names? See this comment.

@rosesyrett
Copy link

rosesyrett commented Aug 21, 2023 •
edited
Loading

Yep; just to chime in, I do think it makes more sense for the PandA to have either names and numbers on their blocks or just names, e.g.

PCAP
PULSE1
PULSE2
SEQ1
SEQ2

but not, for example,

sfp2_sync_in
sfp2_sync_in1

as seems to be happening here

In fact if you have a look at that whole thread it looks like there's a mismatch between the panda blocks on malcolm and what we see via PVI. And I wonder if we can explain why 🤔

@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch from 81e8ece to f20a05b Compare August 23, 2023 10:50
@evalott100
Copy link
Contributor Author

Yep; just to chime in, I do think it makes more sense for the PandA to have either names and numbers on their blocks or just names, e.g.

PCAP PULSE1 PULSE2 SEQ1 SEQ2

but not, for example,

sfp2_sync_in sfp2_sync_in1

as seems to be happening here

In fact if you have a look at that whole thread it looks like there's a mismatch between the panda blocks on malcolm and what we see via PVI. And I wonder if we can explain why 🤔

I'll chat to @coretl about what we want our behaviour to be. In this PR I've caused the softioc to throw an error if the user tries to have a number in the block name (rather than passing number=).

@evalott100
Made metadata label split into multiple PVs and added tests for it, g…
a3861ce 
…oing to clean it up before we're ready to merge
@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch from 2e0e19b to a3861ce Compare August 24, 2023 09:46
@coretl
Copy link
Contributor

coretl commented Aug 29, 2023

To update this ticket, the outcome was that we should never have both sfp2_sync_in and sfp2_sync_in1, that is a bug. All entries should be under sfp2_sync_in. @evalott100 is working on a PR to fix this.

@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch from 3763ec7 to 68639ee Compare August 31, 2023 12:49
@evalott100 evalott100 force-pushed the make_pv_names_consistent_with_web_gui_names branch from 68639ee to c13d7aa Compare August 31, 2023 12:55
AlexanderWells-diamond
AlexanderWells-diamond approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@AlexanderWells-diamond AlexanderWells-diamond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! A couple of minor nitpicks but nothing show stopping.

src/pandablocks_ioc/_pvi.py Show resolved Hide resolved
src/pandablocks_ioc/_tables.py Outdated Show resolved Hide resolved
src/pandablocks_ioc/_types.py Outdated Show resolved Hide resolved
src/pandablocks_ioc/ioc.py Outdated Show resolved Hide resolved
tests/test_ioc.py Show resolved Hide resolved
tests/test_ioc.py Show resolved Hide resolved
@evalott100 evalott100 mentioned this pull request Sep 1, 2023
Merged
@evalott100 evalott100 merged commit 1f8caba into PandABlocks:dev Sep 1, 2023
9 of 10 checks passed
@evalott100 evalott100 deleted the make_pv_names_consistent_with_web_gui_names branch September 1, 2023 13:07
@AlexanderWells-diamond AlexanderWells-diamond mentioned this pull request Sep 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coretl coretl coretl left review comments

@AlexanderWells-diamond AlexanderWells-diamond AlexanderWells-diamond approved these changes

@GDYendell GDYendell Awaiting requested review from GDYendell

Assignees

@evalott100 evalott100

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@evalott100 @AlexanderWells-diamond @rosesyrett @coretl @GDYendell