Skip to content

Conversation

@hungngyenn
Copy link
Contributor

@hungngyenn hungngyenn commented Nov 6, 2025

Closes #

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • CHANGELOG.md has been updated (for bug fixes / features / docs)

What kind of change does this PR introduce?

Introduces Raw Output format per pipeline.

What was changed?

Added Raw Output Class

Related issues

(Add links to related issues)

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)

Other information:

Copy link
Member

@AnthonyCvn AnthonyCvn left a comment

Choose a reason for hiding this comment

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

I see the problem with SpooledTempFile. We used this buffer to accumulate (potentially) large MCAP files that don't fit in memory. But for storing raw data you can create a simple in-memory buffer (an array), accumulate small records up to a certain size, store to ReductStore when it passes a threshold (like 10MB), and if you get large records, stream them directly to ReductStore.

Copy link
Member

@AnthonyCvn AnthonyCvn left a comment

Choose a reason for hiding this comment

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

@hungngyenn I did the refactoring and fixed the timer issue, but the current test for raw output can be simplified and we need an additional one to test the upload of batches.

@AnthonyCvn AnthonyCvn requested a review from Copilot November 8, 2025 06:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces support for raw binary CDR output format as an alternative to MCAP files, along with a significant refactoring of the output writer architecture to support multiple output formats.

  • Refactors the output writing system into a pluggable architecture with abstract base class and format-specific implementations
  • Adds raw binary CDR output format alongside existing MCAP support
  • Reorganizes test structure into unit, integration, config, and linting categories with proper documentation

Reviewed Changes

Copilot reviewed 21 out of 28 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
reductstore_agent/models.py Adds OutputFormat enum with MCAP and RAW options; renames config_models module to models
reductstore_agent/writer/base.py Introduces OutputWriter abstract base class defining unified interface for output writers
reductstore_agent/writer/mcap.py Extracts MCAP-specific writing logic into McapOutputWriter class implementing OutputWriter interface
reductstore_agent/writer/raw.py Implements RawOutputWriter for binary CDR output with batching and streaming for large messages
reductstore_agent/writer/factory.py Adds factory function to create appropriate writer based on pipeline configuration
reductstore_agent/writer/init.py Provides module exports for writer components
reductstore_agent/recorder.py Refactors to use writer factory and delegates format-specific logic to writer implementations
reductstore_agent/state.py Converts PipelineState from Pydantic model to plain class with OutputWriter reference
reductstore_agent/downsampler.py Updates import from config_models to models
reductstore_agent/utils.py Adds ns_to_us utility function for timestamp conversion
test/unit/test_si_units.py Updates import from config_models to models
test/unit/test_downsampling.py Updates import from config_models to models
test/unit/init.py Adds copyright header and module documentation
test/config/test_recorder_params.py Adds parametrized tests for all downsampling modes and output formats; fixes docstrings
test/config/init.py Adds copyright header and module documentation
test/integration/test_topic_subscription.py Adds integration tests for topic subscription filtering and regex matching
test/integration/test_recorder_state.py Adds integration tests for recorder state management with large messages and timers
test/integration/test_raw_output.py Adds integration tests verifying raw output streaming for large records
test/integration/test_mcap_output.py Adds comprehensive integration tests for MCAP output functionality
test/integration/test_entry_point.py Adds test for main entry point with keyboard interrupt handling
test/integration/init.py Adds copyright header and module documentation
test/linting/test_pep257.py Adds PEP257 docstring compliance test
test/linting/test_flake8.py Fixes config path to point to correct setup.cfg location
test/linting/test_copyright.py Adds copyright compliance test
test/linting/init.py Adds copyright header and module documentation
test/init.py Adds copyright header and module documentation
README.md Documents new output_format parameter with mcap and raw options
.gitignore Adds .env file pattern for environment variable files
Comments suppressed due to low confidence (3)

test/config/test_recorder_params.py:167

  • Typo in mode name: "none," should be "none" (remove trailing comma)
    test/config/test_recorder_params.py:199
  • Typo in docstring: "Recoder" should be "Recorder"
    test/config/test_recorder_params.py:110
  • Docstring describes wrong parameter type: should be "Return a list of valid output_format params for 'raw'" instead of "'none'"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@AnthonyCvn AnthonyCvn left a comment

Choose a reason for hiding this comment

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

A couple of suggestions to improve integration tests and i would like to change "raw" to "cdr" as a parameter name.

Copy link
Member

@AnthonyCvn AnthonyCvn left a comment

Choose a reason for hiding this comment

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

Looking good ! Small changes and it's ready to merge

Copy link
Member

@AnthonyCvn AnthonyCvn left a comment

Choose a reason for hiding this comment

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

Perfect!

@tuanhungngyn tuanhungngyn merged commit 3666382 into main Nov 18, 2025
5 checks passed
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