Johnfreeman/file formats refactor#497
Draft
jcfreeman2 wants to merge 24 commits into
Draft
Conversation
added 21 commits
June 23, 2026 12:51
…compile I call this a "save-my-work" commit because I've basically done the bare minimum to abstract out much of the functionality of HDF5DataStore, but in a frankly sloppy, non-coding-standards-compliant kind of way (e.g., I'm using protected data members in an intermediate base class). Still, by committing this, it'll be clearer what refinements I make moving forward (probably const getters for the now-only-protected member data, e.g.)
… the less-bad name FileHandleClass, removing commented-out code, replacing std::numeric_limits<size_t>::max() with an explicit "the record is unset" static member
…pace used in both the trigger record and timeslice-writing functions
…index increment scenarios Prior to this commit, increment_file_index_if_needed only incremented the file index for reasons related to file size; (identical) logic for incrementing the file index based on the timelice or trigger number was implemented after the call to increment_file_index_if_needed in both write() functions. Note that resets of various tracking variables occur if the file index is bumped for size reasons, but not record number reasons. This is what's in the original code (i.e., this commit is a pure refactor), but I should check to see if this is actually the logic that we want.
… a template parameter and not hardwired as the HDF5-specific appmodel::DataStoreConf
… own repeat-timeslice exception, put it in DataStoreImpl. Note for this to build you'll need to also build hdf5libs using its johnfreeman/file_formats_refactor branch
…ther than the body of the constructor; this enables logically-constant data to be declared const
… exception, not just construct it...
…ke a DataStoreImpl-derived data store off of a null configuration
Beyond just the concept of "member data shouldn't directly be part of a class's interface but should always be private and accessed through getters", the advantage here is that variables which are non-constant at the level of DataStoreImpl can be made constant for implementations of open_new_file - e.g., the run number and the file index have const getters. As you can see from this commit, this *does* require adding several getter functions. An alternative would be to have a (very) long signature to open_new_file. Note that I didn't use [[nodiscard]] in the getter functions because the cost of the lowered readability of the source code doesn't seem worth protection against a fairly implausible scenario that a developer would call a function called "get_XXXXX()" and then not do anything with the return value.
…f a low-level class needed for writing data in a DataStoreImpl-derived class (e.g., HDF5RawDataFile from hdf5libs)
Rather than writing out a full trigger record or time slice to an HDF5 file, this will just write out TriggerRecord # or TimeSlice # to a text file. The real point of this is to test the DataStoreImpl intermediate base class (soon to be renamed FileDataStoreImpl, btw). Two things to think about moving forward: * Some of the tests in HDF5Write_test may have been rendered redundant * I should update the code to flexibly handle the extension; currently the text file also has the extension "hdf5"
…ame "FileDataStoreImpl"
…plementors of its derived class to deal with file opening flags
…tension to use be hdf5 and make it the passed-as-template-parameter file object's responsibility to define the extension
…ption of the DataStore interface into closer alignment with its current reality
…space safety factor to 1.1
added 2 commits
July 11, 2026 09:54
…unit testing purposes should still throw ERS exceptions
…t DataStore to throw; note this also prodded me to realize that GeneralDataStoreProblem isn't used, so I deleted it
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Addresses issue #496
As discussed during my talk at the Dataflow meeting on Wednesday morning (Jul-8-2026), I'm filing a draft PR which (with a couple small exceptions which I'll describe below) is a refactor of
dfmodules. The main focus of the refactor has been to separate the file-format-specific parts ofHDFDataStore(e.g., constructinghdf5libs'HDF5RawDataFileclass directly, or hardwiring in thehdf5extension to the output filename) from the file-format-agnostic parts (e.g., deciding when to close a file based on the number of bytes written to it).Note that there are some finishing touches I've left out - e.g., reordering
privatevs.protectedvs.publicdeclarations to adhere to our standards, sticking implementation details in*.hxxfiles, etc. The logic there is that by not doing that, it's easier to compare the intermediate base class I've created,FileDataStoreImpl, with the originalHDFDataStoreon thedevelopbranch.Some of the changes made include:
HDF5FileUtils.hppand its unit tests.#included inHDF5DataStoreon thedevelopbranch, it was last touched in 2021 and none of its functions are used.HDF5DataStorewhich doesn't concernhdf5specifically into an intermediate base class,FileDataStoreImpl, which other file-format-specific implementations can inherit from. How to do this is described in the comment at the top ofFileDataStoreImpl, but you can also see this "in action" by looking atplugins/HDF5DataStore.hppDataStore,StubDataStorewhich inherits fromFileDataStoreImpland writes a very simple one line output to a text file when itswritefunction is called for either trigger records or time slicesStubDataStoreare also added inStubWrite_test; they're basically theHDF5DataStoreunit tests withhdf5-specific stuff stripped out, and are de-facto unit tests forDataStoreImpl.Some of the decisions I made in this Draft PR I imagine (hope) are uncontroversial - getting rid of a no-longer-used header, refactoring the code from
HDF5DataStorewhen it was moved intoFileDataStoreImplto avoid Don't Repeat Yourself, making logically-constant member data declaredconst, etc.Others are open to discussion; I'll point some out here:
developbranch,hdf5libs::HDF5RawDataFile::writefor a timeslice can throw aTimeSliceException. However, because I don't want every file-format-specific class to have to supply its ownTimeSliceException(which would then need to be supplied as a template parameter toFileDataStoreImpl) I require that the class which satisfies theFileHandleConcepthave a boolean telling you if you passed it a duplicate time slice.FileDataStoreImplconstructor where if the free space safety factor is below 1.1 it gets forced to 1.1. I did so thinking the configuration should tell you straightforwardly what's going on, and if we want to set a lower limit, have that enforced in the configuration or at least get the constructor to throw rather than continuing on with a different value than the one it was givenprepare_for_runis called) is an exception thrown in this case. Curious why we don't just throw from the constructor?HDF5Write_testwith its original name, though it could be calledHDFDataStore_test. Perhaps this raises the question, though, as to whetherDataWritewouldn't be a more accurate name thanDataStore?DataStore::finish_with_runbutHDFDataStore's implementation on the develop branch (and henceFileDataStoreImpl's implementation on this feature branch) don't use it. Do we want to keep this in the signature? Should we be using it right now?developbranch'sHDF5DataStore::writefunction for the time slice - and the spirit of "this is just a refactor" -FileDataStoreImpl::writewill check for duplicate time slices after determining whether to open a new file. Should this be before?Also, moving forward, things to think about:
DataStoremay not work long term, and that we'd need to literally just supply the instance of theConfigurationManager; this is an issue that goes beyond the refactor and indeed beyondDataStore.FileDataStoreImpl::open_file_if_needed,m_file_handle.reset();is wrapped in atry-catchblock. However by default destructors arenoexcept, meaning no throw would ever make it into thecatch.To test, of course, check that this branch builds (it should be done alongside branches of the same name in
hdf5libsandtrigger, though in the case oftriggerwe're talking about a one-line change because I switched an argument inhdf5libsfrom a pointer to a reference).The latest test build for this branch is
DFRFD_DEV_260711_A9, which used commit84843de915bc51(Sat Jul 11 10:12:31 2026 -0500).Type of change
Testing checklist
dbt-build --unittest)pytest -s minimal_system_quick_test.py)dunedaq_integtest_bundle.sh)python -m pytest)pre-commit run --all-files)Comments here on the testing
Further checks
dbt-build --lint, and/or see https://dune-daq-sw.readthedocs.io/en/latest/packages/styleguide/)(Indicate issue here: # (issue))