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.

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

[WIP] P11 develop upgrade #854

Closed
wants to merge 165 commits into from
Closed

Conversation

agruzinov
Copy link
Contributor

Here is a repeat of PR 800, hopefully without rebase issues and updated to the current state.

Andrey Gruzinov added 30 commits September 18, 2023 12:11
…y for shutter-like objects inside P11 HO (Bixente)
…y for shutter-like objects inside P11 HO (Bixente)
… at P11 even if everything was ok (Bixente).
@agruzinov agruzinov requested review from beteva, marcus-oscarsson and fabcor-maxiv and removed request for marcus-oscarsson February 20, 2024 10:57
Copy link
Contributor

@fabcor-maxiv fabcor-maxiv left a comment

Choose a reason for hiding this comment

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

I only did a quick review of the parts that are not specific to DESY.

I am wondering why we did not squash the commits, since we opened a new pull request. For people like me who look at the git history often, 159 commits of back and forth are horrible.

@@ -9,6 +9,7 @@
from pprint import pformat
from collections import namedtuple
from datetime import datetime
import warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

Already imported at line 8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@@ -101,6 +103,11 @@ def update_value(self, value=None):
Args:
value (float): value
"""
if self._updating_value:
return # Already updating, avoid recursion
Copy link
Contributor

Choose a reason for hiding this comment

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

That is quite unusual for me to see something like this... I wonder if there is a better solution to prevent recursion.

Where does the recursion happen?

Why is it a problem if we call this more than once? What is the issue?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, where does the recursion happen? The update mechanism is quite largely used and wed did not observe any problem. This is worrying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This recursion error happens from time to time with "recursion limit has reached" error but It was not very reproducinble. Since this fix I did not encounter it so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2023-08-21 09:54:42,912 |HWR |DEBUG | queue_entry - about to start data collection
2023-08-21 09:54:42,912 |HWR |DEBUG | queue_entry - collect_dc
2023-08-21 09:54:42,913 |HWR |DEBUG | queue_entry - standard collect
2023-08-21 09:54:42,913 |HWR |DEBUG | queue_entry - standard collect
2023-08-21 09:54:42,913 |HWR |DEBUG | PathTemplate (DESY) - (to be defined) directory is /gpfs/current/raw/tt_2
2023-08-21 09:54:42,913 |HWR |DEBUG | queue_entry - now calling collect hwo
2023-08-21 09:54:42,913 |user_level_log|INFO | Collection: Preparing to collect
2023-08-21 09:54:42,928 |HWR |INFO | Collection parameters: {'comments': '', 'take_video': False, 'take_snapshots': 4, 'fileinfo': {'directory': '/gpfs/current/raw/tt_2', 'prefix': 'puck20_sample08_w1', 'run_number': 1, 'archive_directory': '/gpfs/current/raw', 'process_directory': '/gpfs/current/processed/tt_2', 'template': 'puck20_sample08_w1_1_%05d.h5', 'compression': False}, 'in_queue': False, 'in_interleave': None, 'detector_binning_mode': None, 'detector_roi_mode': 'disabled', 'shutterless': True, 'sessionId': '61749', 'do_inducedraddam': False, 'sample_reference': {'spacegroup': '', 'cell': '0,0,0,0,0,0', 'blSampleId': -1}, 'processing': 'True', 'processing_offline': True, 'processing_online': False, 'residues': 200, 'dark': False, 'resolution': {'upper': 1.83}, 'transmission': 24.1504, 'energy': 11.9995, 'oscillation_sequence': [{'exposure_time': 0.008, 'kappaStart': 0.0, 'phiStart': 0.0, 'start_image_number': 1, 'number_of_images': 1440, 'overlap': 0, 'start': 0.0, 'range': 0.25, 'number_of_passes': 1, 'number_of_lines': 1, 'mesh_range': (), 'num_triggers': 0, 'num_images_per_trigger': 0}], 'group_id': None, 'EDNA_files_dir': '/gpfs/current/processed/tt_2', 'xds_dir': '', 'anomalous': False, 'experiment_type': 'OSC', 'skip_images': False, 'motors': {'phi': -1.539453422532113e-05, 'phiz': 0.0, 'phiy': -595.25, 'phix': None, 'sampx': -279.183, 'sampy': -147.468, 'zoom': None, 'kappa': None, 'kappa_phi': None, 'microy': None, 'microz': None}, 'status': 'Running', 'collection_start_time': '2023-08-21 09:54:42'}
2023-08-21 09:54:42,929 |user_level_log|INFO | Collection: Storing data collection in LIMS
2023-08-21 09:54:42,929 |user_level_log|INFO | Collection: Creating directories for raw images and processing files
2023-08-21 09:54:42,929 |user_level_log|INFO | Collection: Getting sample info from parameters
2023-08-21 09:54:42,929 |user_level_log|INFO | Collection: Storing sample info in LIMS
2023-08-21 09:54:42,929 |user_level_log|INFO | Collection: Moving to centred position
2023-08-21 09:54:42,929 |HWR.P11NanoDiff|DEBUG | first moving translation motor 'phiy' to position -595.25
2023-08-21 09:54:42,930 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /phiy to -595.25
2023-08-21 09:54:42,943 |HWR.P11NanoDiff|DEBUG | first moving translation motor 'phiz' to position 0.0
2023-08-21 09:54:42,943 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /phiz to 0.0
2023-08-21 09:54:43,046 |HWR.P11NanoDiff|DEBUG | translation movements DONE
2023-08-21 09:54:43,047 |HWR.P11NanoDiff|DEBUG | then moving alignment motor 'sampx' to position -279.183
2023-08-21 09:54:43,047 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /sampx to -279.183
2023-08-21 09:54:43,081 |HWR.P11NanoDiff|DEBUG | then moving alignment motor 'sampy' to position -147.468
2023-08-21 09:54:43,082 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /sampy to -147.468
2023-08-21 09:54:43,217 |HWR.P11NanoDiff|DEBUG | alignment movements DONE
2023-08-21 09:54:43,217 |HWR.P11NanoDiff|DEBUG | finally moving motor 'phi' to position -147.468
2023-08-21 09:54:43,217 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /omega to -1.539453422532113e-05
2023-08-21 09:54:43,484 |HWR.TangoMotor|DEBUG | reading motor state for /phiy is ON
2023-08-21 09:54:43,492 |HWR.TangoMotor|DEBUG | reading motor state for /phiz is ON
2023-08-21 09:54:43,573 |HWR.P11NanoDiff|DEBUG | phi move DONE
2023-08-21 09:54:43,573 |HWR.P11NanoDiff|DEBUG | moving remaining motor phix to position None
2023-08-21 09:54:43,574 |HWR.P11NanoDiff|DEBUG | moving remaining motor zoom to position None
2023-08-21 09:54:43,574 |HWR.P11NanoDiff|DEBUG | moving remaining motor kappa to position None
2023-08-21 09:54:43,574 |HWR.P11NanoDiff|DEBUG | moving remaining motor kappa_phi to position None
2023-08-21 09:54:43,574 |HWR.P11NanoDiff|DEBUG | moving remaining motor microy to position None
2023-08-21 09:54:43,574 |HWR.P11NanoDiff|DEBUG | moving remaining motor microz to position None
2023-08-21 09:54:43,574 |user_level_log|INFO | Collection: Taking 4 sample snapshot(s)
2023-08-21 09:54:43,574 |HWR.P11Collect|DEBUG | #COLLECT# taking crystal snapshot.
2023-08-21 09:54:43,574 |HWR.P11Collect|DEBUG | #COLLECT# take_snapshot. moving to centring phase
2023-08-21 09:54:43,575 |HWR.P11NanoDiff|DEBUG | SETTING CENTRING PHASE
2023-08-21 09:54:43,575 |HWR.P11NanoDiff|DEBUG | - close detector cover
2023-08-21 09:54:43,610 |HWR.P11NanoDiff|DEBUG | - setting backlight in
2023-08-21 09:54:43,661 |HWR.P11NanoDiff|DEBUG | PHASE (Centring) NOT REACHED YET
2023-08-21 09:54:43,661 |HWR.P11NanoDiff|DEBUG | waiting for: lightin,collim_down,bstop_out
2023-08-21 09:54:43,661 |HWR.P11NanoDiff|DEBUG | - putting collimator down
2023-08-21 09:54:43,661 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /collimy to -360
2023-08-21 09:54:43,674 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /collimz to -13000
2023-08-21 09:54:43,686 |HWR.P11NanoDiff|DEBUG | - setting beamstop out
2023-08-21 09:54:43,686 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /bstopx to 69000
2023-08-21 09:54:43,689 |HWR.P11NanoDiff|DEBUG | - moving yag down
2023-08-21 09:54:43,689 |HWR.TangoMotor|DEBUG | TangoMotor.py - Moving motor /yagz to -12550
2023-08-21 09:54:43,689 |HWR.P11NanoDiff|DEBUG | - moving pinhole down
2023-08-21 09:54:43,689 |HWR.P11NanoDiff|DEBUG | WAITING PHASE STARTED
2023-08-21 09:54:43,692 |HWR.TangoMotor|DEBUG | reading motor state for /sampx is ON
2023-08-21 09:54:43,693 |HWR.TangoMotor|DEBUG | reading motor state for /sampy is ON
2023-08-21 09:54:44,488 |root |ERROR | Uncaught exception : Traceback (most recent call last):

File "/gpfs/local/shared/MXCuBE/backup/22-05-2023/mxcubecore_edna_tests/mxcubecore/dispatcher.py", line 27, in __my_robust_apply
return robustapply._robust_apply(*args, **kwargs)

File "/gpfs/local/shared/MXCuBE/mxenv/lib/python3.7/site-packages/pydispatch/robustapply.py", line 55, in robustApply
return receiver(*arguments, **named)

File "/gpfs/local/shared/MXCuBE/backup/22-05-2023/mxcubecore_edna_tests/mxcubecore/HardwareObjects/MotorsNPosition.py", line 198, in motor_value_changed
self.update_multi_value()

File "/gpfs/local/shared/MXCuBE/backup/22-05-2023/mxcubecore_edna_tests/mxcubecore/HardwareObjects/MotorsNPosition.py", line 210, in update_multi_value
current_pos[motorname] = self.motor_hwobjs[motorname].get_value()

File "/gpfs/local/shared/MXCuBE/backup/22-05-2023/mxcubecore_edna_tests/mxcubecore/HardwareObjects/TangoMotor.py", line 149, in get_value
value = self.chan_position.get_value()

File "/gpfs/local/shared/MXCuBE/backup/22-05-2023/mxcubecore_edna_tests/mxcubecore/Command/Tango.py", line 349, in get_value
self.update(value)

File "/gpfs/local/shared/MXCuBE/backup/22-05-2023/mxcubecore_edna_tests/mxcubecore/Command/Tango.py", line 331, in update
self.emit("update", value)

File "/gpfs/local/shared/MXCuBE/backup/22-05-2023/mxcubecore_edna_tests/mxcubecore/CommandContainer.py", line 144, in emit
dispatcher.send(signal, self, *args)

File "/gpfs/local/shared/MXCuBE/mxenv/lib/python3.7/site-packages/pydispatch/dispatcher.py", line 332, in send
for receiver in liveReceivers(getAllReceivers(sender, signal)):

File "/gpfs/local/shared/MXCuBE/mxenv/lib/python3.7/site-packages/pydispatch/dispatcher.py", line 256, in liveReceivers
for receiver in receivers:

File "/gpfs/local/shared/MXCuBE/mxenv/lib/python3.7/site-packages/pydispatch/dispatcher.py", line 286, in getAllReceivers
if receiver: # filter out dead instance-method weakrefs

File "/gpfs/local/shared/MXCuBE/mxenv/lib/python3.7/site-packages/pydispatch/saferef.py", line 149, in nonzero
return self() is not None

File "/gpfs/local/shared/MXCuBE/mxenv/lib/python3.7/site-packages/pydispatch/saferef.py", line 167, in call
target = self.weakSelf()

RecursionError: maximum recursion depth exceeded while calling a Python object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After updating to the latest development, the recursion error happens from time to time again. It seems to be inside the MotorsNPosition.py when it is calling get_value() from the TangoMotor.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the log:

2024-03-04 15:21:48,399 |HWR.P11Pinhole|DEBUG | - position for pinholey is -421.353
2024-03-04 15:21:48,400 |HWR.P11Pinhole|DEBUG | - position for pinholez is 13468.007
2024-03-04 15:21:48,400 |HWR.P11Pinhole|DEBUG | - motor pinholey is at -421
2024-03-04 15:21:48,400 |HWR.P11Pinhole|DEBUG | - motor pinholez is at 13468
2024-03-04 15:21:48,400 |HWR.P11NanoDiff|DEBUG | - checking gonio tower position
2024-03-04 15:21:48,400 |HWR.P11NanoDiff|DEBUG | WAITING PHASE STARTED
2024-03-04 15:21:48,578 |HWR.TangoMotor|DEBUG | reading motor state for /omega is ON
2024-03-04 15:21:48,600 |HWR.MotorsNPosition|DEBUG | - motor collimy is at -360
2024-03-04 15:21:48,601 |HWR.MotorsNPosition|DEBUG | - motor collimz is at -13000
2024-03-04 15:21:48,712 |HWR.MotorsNPosition|DEBUG | - motor collimy is at -360
2024-03-04 15:21:48,713 |HWR.MotorsNPosition|DEBUG | - motor collimz is at -13000
2024-03-04 15:21:48,781 |root |ERROR | Uncaught exception : Traceback (most recent call last):

File “/gpfs/local/shared/MXCuBE/mxcubecore/mxcubecore/dispatcher.py”, line 27, in __my_robust_apply
return robustapply._robust_apply(*args, **kwargs)

File “/home/p11user/.pyenv/versions/mxenv3.8/lib/python3.8/site-packages/louie/robustapply.py”, line 57, in robust_apply
return receiver(*arguments, **named)

File “/gpfs/local/shared/MXCuBE/mxcubecore/mxcubecore/HardwareObjects/MotorsNPosition.py”, line 189, in motor_value_changed
self.update_multi_value()

File “/gpfs/local/shared/MXCuBE/mxcubecore/mxcubecore/HardwareObjects/MotorsNPosition.py”, line 201, in update_multi_value
current_pos[motorname] = self.motor_hwobjs[motorname].get_value()

File “/gpfs/local/shared/MXCuBE/mxcubecore/mxcubecore/HardwareObjects/TangoMotor.py”, line 151, in get_value
value = self.chan_position.get_value()

File “/gpfs/local/shared/MXCuBE/mxcubecore/mxcubecore/Command/Tango.py”, line 342, in get_value
value = self.device.read_attribute(self.attribute_name).value

File “/home/p11user/.pyenv/versions/mxenv3.8/lib/python3.8/site-packages/tango/green.py”, line 207, in greener
executor = get_object_executor(obj, green_mode)

File “/home/p11user/.pyenv/versions/mxenv3.8/lib/python3.8/site-packages/tango/green.py”, line 182, in get_object_executor
green_mode = get_object_green_mode(obj)

RecursionError: maximum recursion depth exceeded

2024-03-04 15:21:48,785 |HWR.P11Pinhole|DEBUG | - position for pinholey is -421.353
2024-03-04 15:21:48,786 |HWR.P11Pinhole|DEBUG | - position for pinholez is 13468.007
2024-03-04 15:21:48,786 |HWR.P11Pinhole|DEBUG | - motor pinholey is at -421
2024-03-04 15:21:48,786 |HWR.P11Pinhole|DEBUG | - motor pinholez is at 13468

smp._set_loaded(True)
self.log.debug(f" Found sample {smp} is loaded")
self.log.debug(f" getting loaded {self.get_loaded_sample()}")
# if self.get_loaded_sample() == smp:
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the code, do not comment it out.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the line should not be commented out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately if this is uncomented, it crushes with the error sample not loaded, although it is perfectly loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2023-10-17 17:34:06,952 |HWR.MotorsNPosition|DEBUG | - motor yagz is at -12550
2023-10-17 17:34:06,954 |user_level_log|INFO | Sample changer: Sample loaded (total time: 29.94912600517273)
2023-10-17 17:34:06,957 |GUI |ERROR | Error loading sample, ('Sample not loaded', '')
2023-10-17 17:34:06,960 |GUI |ERROR | Traceback (most recent call last):
File "/gpfs/local/shared/MXCuBE/mxcubeqt/mxcubeqt/widgets/dc_tree_widget.py", line 497, in mount_sample_task
self.sample_centring_result
File "/gpfs/local/shared/MXCuBE/mxcubecore/mxcubecore/queue_entry/base_queue_entry.py", line 839, in mount_sample
raise QueueSkippEntryException("Sample not loaded", "")
mxcubecore.queue_entry.base_queue_entry.QueueSkippEntryException: ('Sample not loaded', '')

2023-10-17 17:34:54,454 |HWR.MotorsNPosition|DEBUG | - motor collimy is at -360
2023-10-17 17:34:54,454 |HWR.MotorsNPosition|DEBUG | - motor collimz is at -13000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was hard to trace, but @vrey01 found it. It is left there because it is hard to tell whether it is for everyone. Could someone try on other beamline what will happen? What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe because the get_loaded_sample output is different from smp. Could you, please, check this, as we do not have the same error, but not the same sample changer neither.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick check shows that it was not redefined in the P11SampleChanger class. I can check it when I have access to the hardware though.

@fabcor-maxiv fabcor-maxiv mentioned this pull request Feb 20, 2024
Comment on lines 801 to 803

osc_start = motor_positions.get("phi", params["osc_start"])

Copy link
Member

Choose a reason for hiding this comment

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

Actually both the old and the new version will rais a KeyError exception if osc_start is not in the params dictionary. I believe the correct code should be:
osc_start = params.get("osc_start") or motor_positions.get("phi")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected, thanks!

@beteva
Copy link
Member

beteva commented Feb 20, 2024

Hi @agruzinov. The DESY specific hardware objects look fine to me. Maybe additional cleaning could be to use super(). instead of super(classname, self), but this depends on you.
I am concerned about the recursion you mention and find important to understand why and where it happens.
Also thanks to you changing the Beamline.py, we've spotted a bug, which is very unlikely to happen, but still it is nice to have a clean code.
I would not merge yet the PR, until we understand the recursion problem.

@agruzinov
Copy link
Contributor Author

Thanks everyone for the review and comments. Would it make sense to split this PR now into HO/DESY part and abstract ones since DESY specific hardware objects are ok for everyone?

Digging into recursion problem might take more time.

@beteva
Copy link
Member

beteva commented Feb 23, 2024

Yes, @agruzinov, very good idea. We could merge your local HO and than try to find the origin of the recursion and the problem with the sample changer

@agruzinov agruzinov marked this pull request as draft February 26, 2024 13:20
@agruzinov agruzinov changed the title P11 develop upgrade [WIP] P11 develop upgrade Feb 28, 2024
@marcus-oscarsson
Copy link
Member

@agruzinov, did I understand you correctly, that you wanted to close this PR ?

@agruzinov
Copy link
Contributor Author

Yes, as soon as #859 will be merged. I've manually merged all the HO objects, so no-one need to go through >160 commits in the history.

@marcus-oscarsson
Copy link
Member

:), thanks thats thoughtfull of you ;)

@agruzinov
Copy link
Contributor Author

This PR was split to two (PR856, 859) as discussed. Closing.

@agruzinov agruzinov closed this Mar 8, 2024
@agruzinov agruzinov deleted the p11_develop branch October 1, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants