Skip to content

g.begin() should work even if DBSVR is unreachable #24

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
Open
1 task
Tom-Willemsen opened this issue Feb 20, 2025 · 8 comments · May be fixed by #25
Open
1 task

g.begin() should work even if DBSVR is unreachable #24

Tom-Willemsen opened this issue Feb 20, 2025 · 8 comments · May be fixed by #25
Assignees
@Tom-Willemsen

Comments

@Tom-Willemsen
Copy link
Contributor

Tom-Willemsen commented Feb 20, 2025 •
edited
Loading

As a user on WISH I'd like to be able to begin a run.

WISH's DBSVR fell over, usually this should not be critical, but it was causing a failure during g.begin().

Here we set defaults for various sample parameters to "" (empty string):

genie/src/genie_python/genie.py

Lines 792 to 794 in c4c5871

meas_type: str = "",
meas_subid: str = "",
sample_id: str = "",

Here we check those values against None and set them if not None

genie/src/genie_python/genie_dae.py

Lines 325 to 327 in c4c5871

for pv, value in sample_pars.items():
if value is not None:
self.api.set_sample_par(pv, str(value))

This means that an argument-less g.begin() depends on the DBSVR being up.

Probably the correct fix is to default the pars in genie.py level to None rather than empty string

Acceptance criteria

  • g.begin() should begin a run even with a DBSVR that's down
@Tom-Willemsen Tom-Willemsen linked a pull request Feb 20, 2025 that will close this issue
Open
6 tasks
@rerpha rerpha moved this to In Progress in PI_2025_02 Feb 20, 2025
@rerpha
Copy link
Contributor

rerpha commented Feb 20, 2025

i'll add some release notes

@FreddieAkeroyd
Copy link
Member

FreddieAkeroyd commented Feb 21, 2025 •
edited
Loading

If we default to None then rather than "" then that means we do not clear a previous sample par value on a simple begin() as we did before - it's a change of behaviour, but maybe OK?

@FreddieAkeroyd
Copy link
Member

FreddieAkeroyd commented Feb 21, 2025 •
edited
Loading

The sample parameter values are actually in the DAE ioc - is it just a name mapping it reads from DBSRV? Is it worth caching this lookup?

@Tom-Willemsen
Copy link
Contributor Author

The sample parameter values are actually in the DAE ioc - is it just a name mapping it reads from DBSRV? Is it worth caching this lookup?

Yes I think that's right -

genie/src/genie_python/genie_epics_api.py

Lines 619 to 635 in 925f743

def set_sample_par(self, name: str, value: "PVValue") -> None:
"""
Set a new value for a sample parameter.
Args:
name: the name of the parameter to change
value: the new value
"""
names = self.blockserver.get_sample_par_names()
if names is not None:
for n in names:
m = re.match(".+:SAMPLE:%s" % name.upper(), n)
if m is not None:
# Found it!
self.set_pv_value(self.prefix_pv_name(n), value)
return
raise Exception("Sample parameter %s does not exist" % name)

@Tom-Willemsen
Copy link
Contributor Author

@FreddieAkeroyd are you happy with the change in behaviour or want me to investigate other approaches e.g. more caching, or just hard-coded fallbacks?

@FreddieAkeroyd
Copy link
Member

I pushed a simple caching - does that look OK? I am in two minds about the "" to None change - doing it make a plain begin() command behave like the begin GUI button press, but it is a change in behaviour and may mean sample pars could get carried over to the wrong experiment, but i'm not sure how widely this is used. My guess would be maybe sans/reflectometry?

@Tom-Willemsen
Copy link
Contributor Author

I guess the tradeoff is even with a cache we depend on DBSVR being up for at least the first begin command in a new python session. Frequency of recreating python session does vary a lot by instrument. Maybe that's an acceptable reduction though.

@FreddieAkeroyd
Copy link
Member

the first begin should be from a user initiated action, so hopefully obvious to somebody at that point if it doesn't work. I think the original change you suggested may be good to have too, i was just cautious about the change in behaviour and accidentally propagating values to future runs that may not be related, hence a bit of prior investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Tom-Willemsen Tom-Willemsen

Labels
None yet
Projects
PI_2025_02
Status: In Progress
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Default to None rather than empty string ISISComputingGroup/genie
3 participants
@FreddieAkeroyd @rerpha @Tom-Willemsen