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

PERF: Remove buffer size limitation #310

Merged

Conversation

greglucas
Copy link
Collaborator

Change Summary

Overview

No need to explicitly overwrite the default size.

Similar to #309 and seeing things got slower, but a different solution here.

python -m cProfile -o orig.prof -m pytest imap_processing/tests/glows
Goes from 60s down to 20s on my local machine.

From the docs: https://space-packet-parser.readthedocs.io/en/latest/autoapi/space_packet_parser/parser/index.html

buffer_read_size_bytes (int, Optional) – Number of bytes to read from e.g. a BufferedReader or socket binary data source on each read attempt. Default is 4096 bytes.

I wonder if this should be set globally for everyone if we do want to update the size of the reads...

Letting the read buffer be determined by the interpreter helps
with speed of the tests locally. No need to explicitly set the size
for now.
@greglucas greglucas added Ins: MAG Related to the MAG instrument Ins: GLOWS Related to the GLOWS instrument Level: L0 Level 0 processing labels Jan 9, 2024
@greglucas greglucas requested a review from maxinelasp January 9, 2024 02:13
@maxinelasp
Copy link
Contributor

I'm still a little concerned about this, because it's not clear to me what the right value should be. But I'm ok with using the default for now until we figure out if we need to overwrite this value. Just want to note that the test file is very small, so it isn't necessarily a perfect test for this value.

@greglucas
Copy link
Collaborator Author

Great question, I just asked this to the space packet parser Slack channel.

@greglucas greglucas merged commit 8586da0 into IMAP-Science-Operations-Center:dev Jan 9, 2024
17 checks passed
@greglucas greglucas deleted the generator-buffer branch January 9, 2024 23:10
@greglucas
Copy link
Collaborator Author

@maxinelasp , I just merged this in per our conversation that it should be closer to packet size than file size, but the suggestion was to just leave it default until we find that limiting.

I think follow-up work could be to set this value to the fixed packet size for instruments if that makes sense or is needed. Maybe there are some utilities on the xtce parser to get that value for us before reading the stream...

laspsandoval pushed a commit to laspsandoval/imap_processing that referenced this pull request Apr 2, 2024
…generator-buffer

PERF: Remove buffer size limitation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ins: GLOWS Related to the GLOWS instrument Ins: MAG Related to the MAG instrument Level: L0 Level 0 processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants