Skip to content

Changes for murko found from beamline testing#809

Merged
DominicOram merged 15 commits intomainfrom
530_only_stream_OAV_on_updates
Oct 24, 2024
Merged

Changes for murko found from beamline testing#809
DominicOram merged 15 commits intomainfrom
530_only_stream_OAV_on_updates

Conversation

@DominicOram
Copy link
Contributor

Fixes DiamondLightSource/mx-bluesky#530 and DiamondLightSource/mx-bluesky#147

Built on top of #808 so review/merge that first

Additionally this PR:

  • Changes the format of data we store in redis to be just the raw JPEG bytes. This was done after discussion with @aragaod to reduce the memory usage of the redis database.
  • Adds a timeout of 30s and so will not stream data for longer than this, FYI @aragaod. We can update this timeout later if needed

Instructions to reviewer on how to test:

  1. Confirm tests still pass
  2. Confirm device still connects with dodal connect i04

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

@DominicOram DominicOram changed the title 530 only stream oav on updates Only stream OAV to redis when OAV updates and other changes for murko found from beamline testing Sep 27, 2024
@DominicOram DominicOram changed the title Only stream OAV to redis when OAV updates and other changes for murko found from beamline testing Changes for murko found from beamline testing Sep 27, 2024
@codecov
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.59%. Comparing base (e673dbf) to head (0b2a05c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #809   +/-   ##
=======================================
  Coverage   95.58%   95.59%           
=======================================
  Files         125      125           
  Lines        5419     5422    +3     
=======================================
+ Hits         5180     5183    +3     
  Misses        239      239           

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

@DominicOram DominicOram marked this pull request as ready for review September 27, 2024 12:20
Base automatically changed from mx_bluesky_511_murko_fixes to main October 14, 2024 13:25
done_status = AsyncStatus(
asyncio.wait_for(self._stop_flag.wait(), timeout=self.TIMEOUT)
)
async for image_uuid in observe_value(self.counter, done_status=done_status):
Copy link
Contributor

Choose a reason for hiding this comment

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

this counter signal, is it really a counter? It looks like it's actually uuids? Or are they not really uuids and are just sequence numbers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Q: How frequently does this signal update, is it guaranteed that we will see every frame id that we need to collect?

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 is the frame counter on the device. We can use it as a UUID as it increments with every frame and we're only using the one camera so will be unique. I suspect there is a roll-over at something around the max int value but given we don't keep data more than a week I think it's unlikely to affect us.

It updates at the rate of the camera (~15Hz). You're right that we may end up missing some, though experimentally we haven't yet, but even if we do it's not critical we catch every frame.

Legitimate questions though, thanks for raising them, I will add some comments in the code to this affect.

Copy link

Choose a reason for hiding this comment

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

Re: uniqueness of the UUID

Unsure if it is overkill but can envisage during some operational issue a few camera IOC restarts could be needed. It is possible the counter always starts at one from IOC restart. In a combination of IOC restarts plus same sampleID being loaded and data collected on the same frame number (very unlikely combinations) then a UUID collision could technically happen. A possible solution would be to add a prefix of some kind randomly generated.

But unsure if it is worth the effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks. I will add a proper UUID section in too as a precaution.

Copy link
Contributor

@rtuck99 rtuck99 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 apart from maybe change the name of the counter and also query about whether we might get all the frames.

jpeg_bytes = await get_next_jpeg(response)
self.uuid_setter(image_uuid := str(uuid.uuid4()))
img = Image.open(io.BytesIO(jpeg_bytes))
image_data = pickle.dumps(np.asarray(img))
Copy link

Choose a reason for hiding this comment

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

Before we move forward and forget would like 5 minutes look and maybe A comment left in the code why we make this decision now (allo5wing to change in the future if necessary).

First we decided to not store numpy arrays of the raw images (jpg decompressed) after realising it would occupy enormous amounts of DB space with no real benefit on speed.

Question on current implementation: there 3 ways to store binary jpg into redis

I) directly as an bytesIO object using data.getvalues()
II) serialising it (as we make here with pickle)
III) using a redis optimize structure called redis bitmap for such objects

Happy we stay with (II) as I don't fully know disadvantages of the other ones Vs this one but maybe leave a comment this could be relooked if necessary in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, will add a comment and make an issue to look at a follow-up if we think useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue at DiamondLightSource/mx-bluesky#592 to investigate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I actually think this PR already removes the pickling and just stores the raw bytes. I will add a comment though.

@DominicOram DominicOram merged commit c89282a into main Oct 24, 2024
@DominicOram DominicOram deleted the 530_only_stream_OAV_on_updates branch October 24, 2024 12:50
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.

Murko: Only stream image to redis when the OAV updates

3 participants