Use configuration for I/O, plus general cleanup #22

barnaliy · 2025-08-06T21:26:37Z

This PR reorganizes the configuration system that maps data products to storage technologies and file destinations.

What the Configuration System Does:
The configuration system allows users to specify:

Which storage technology to use for each data product (ROOT TTree, ROOT RNtuple, HDF5)
Which file each data product should be written to
Runtime flexibility rather than compile-time hardcoded storage decisions.

Tagging @gemmeren and @aolivier23
Requesting reviews from @knoepfel and @pcanal

Needed

Update factories.hpp

Update storage.cpp

form/util/factories.hpp

form/form/form.cpp

form/persistence/persistence.cpp

test/form/CMakeLists.txt

test/form/config.hpp

test/form/config.cpp

test/form/writer.cpp

test/form/reader.cpp

…ests

form/form/parse_config.cpp

gemmeren

Mostly minor things now :)

form/form/parse_config.hpp

form/mock_phlex/phlex_toy_config.cpp

form/mock_phlex/phlex_toy_config.hpp

form/util/factories.hpp

form/mock_phlex/phlex_toy_core.hpp

…per constants to technology.hpp and update factories.hpp to use named constants instead of hardcoded values. Addresses code review feedback.

form/util/factories.hpp

…sues

barnaliy · 2025-08-22T00:50:41Z

Thanks @pcanal , @gemmeren for the detailed code reviews and thoughtful feedback! I couldn't respond to each comment individually, but I believe most or all of your suggestions have been addressed in the last few commits. Please let me know if I missed anything.

form/form/technology.hpp

- Replace magic numbers with constexpr constants and Combine() helper - Add HDF5 placeholder - Remove redundant form/ directory from include paths Update CMake and source files to match simplified directory structure

gemmeren

Thanks Barnali, this looks good.

pcanal

Thanks. Let's squash on merge.

knoepfel

@barnaliy, just some minor comments for your consideration.

form/root_storage/root_tfile.cpp

knoepfel · 2025-08-22T18:16:00Z

form/storage/storage_associative_container.hpp

+    const std::string& top_name();
+    const std::string& col_name();


Should be const-qualified member functions:

Suggested change

const std::string& top_name();

const std::string& col_name();

const std::string& top_name() const;

const std::string& col_name() const;

knoepfel · 2025-08-22T18:17:17Z

form/storage/storage_container.cpp


 Storage_Container::Storage_Container(const std::string& name) : m_name(name), m_file(nullptr) {}

-Storage_Container::~Storage_Container() {}


Same comment as before. Putting the implementation of the destructor in the .cpp makes sense. If you want it to be the default implementation, just specify it here:

Storage_Container::~Storage_Container() = default;

knoepfel · 2025-08-22T18:19:28Z

form/persistence/persistence.cpp

+    const form::experimental::config::parse_config& config) //factory takes form config
  {
-    return std::unique_ptr<IPersistence>(new Persistence());
+    return std::unique_ptr<IPersistence>(new Persistence(config));


If you're adjusting this line of code anyway, I suggest using std::make_unique:

Suggested change

return std::unique_ptr<IPersistence>(new Persistence(config));

return std::make_unique<Persistence>(config);

knoepfel · 2025-08-22T18:23:51Z

form/persistence/persistence.cpp

+  m_store(nullptr), m_config(config) // constructor takes form config
+{
+  m_store = createStorage();
+}


Suggested change

m_store(nullptr), m_config(config) // constructor takes form config

{

m_store = createStorage();

}

m_store(createStorage()), m_config(config) // constructor takes form config

{}

@knoepfel: Thanks for the feedback! I've implemented your suggestions.

std::find_if() and simplified returning unique_ptr<>s with std::make_unique(). Many of these changes were lost from PR #22, so I'm going to make another commit later that restores the PR #22 changes and still brings in technology configuration.

technology-specific configuration system.

* First import of latest FORM developments from Phlex. This will eventually become a PR to add per-container configuration attributes. * Restored include guard formatting that didn't all make it to the FORM development repository. * Restored CMakeLists.txt formatting to match the main branch when possible. * Restored formatting in a CMakeLists.txt that I missed. * Change include guards to _HPP__ from _H__ to match file names after reaching consensus with FORM team. * Changed configuration examples to use enums from technology header instead of magic numbers. * Removed member variable of form interface and other small text changes from PR review. * Fixed most of the requests from PR #36. * Restored PR #22 upgrades to Persistence while keeping new technology-specific configuration system. * Committing clang-format changes --------- Co-authored-by: Andrew Olivier <aolivier@anl.gov> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

barnaliy and others added 18 commits August 6, 2025 12:13

Preparing for PR

4f9670f

Preparing for PR

db046a0

Preparing for PR

50ee225

Preparing for PR

fde8891

Preparing for PR

5e07cd7

Preparing for PR

e35ba3c

Immediate Changes by P

6c320ad

Changes addressed by P

5a49fba

Changes addressed by P

e98e7bf

Minor fix

c6f3b15

Monir fix

770815b

Minor fix

2f0f7f6

Delete form/phlex_toy_config.hpp

c409192

Needed

Delete form/phlex_toy_config.cpp

8d48af2

Needed

Update factories.hpp

a3cd373

Update storage.cpp

bcea03f

Merge pull request #1 from gemmeren/patch-1

649f900

Update factories.hpp

Merge pull request #2 from gemmeren/patch-2

3d75f90

Update storage.cpp

barnaliy requested review from knoepfel and pcanal August 6, 2025 21:26

gemmeren reviewed Aug 11, 2025

View reviewed changes

form/util/factories.hpp Show resolved Hide resolved

barnaliy added 2 commits August 12, 2025 13:27

Moving the 'toy' phlex files to mock_phlex directory

4e7c7c9

Clean up

8d3a9a2

gemmeren assigned gemmeren, aolivier23 and barnaliy Aug 12, 2025

aolivier23 reviewed Aug 12, 2025

View reviewed changes

form/form/form.cpp Outdated Show resolved Hide resolved

gemmeren reviewed Aug 12, 2025

View reviewed changes

form/form/form.cpp Outdated Show resolved Hide resolved

gemmeren reviewed Aug 12, 2025

View reviewed changes

form/form/form.cpp Outdated Show resolved Hide resolved

gemmeren reviewed Aug 12, 2025

View reviewed changes

form/persistence/persistence.cpp Outdated Show resolved Hide resolved

gemmeren reviewed Aug 20, 2025

View reviewed changes

test/form/CMakeLists.txt Outdated Show resolved Hide resolved

gemmeren reviewed Aug 20, 2025

View reviewed changes

test/form/config.hpp Outdated Show resolved Hide resolved

gemmeren reviewed Aug 20, 2025

View reviewed changes

test/form/config.hpp Outdated Show resolved Hide resolved

gemmeren reviewed Aug 20, 2025

View reviewed changes

test/form/config.cpp Outdated Show resolved Hide resolved

Minor fix

f40177f

gemmeren reviewed Aug 21, 2025

View reviewed changes

test/form/writer.cpp Outdated Show resolved Hide resolved

gemmeren reviewed Aug 21, 2025

View reviewed changes

test/form/reader.cpp Outdated Show resolved Hide resolved

barnaliy added 2 commits August 21, 2025 13:18

Minor fix

aafcff0

Include Technology constants and mock_phlex config in reader/writer t…

5ad749b

…ests

gemmeren reviewed Aug 21, 2025

View reviewed changes

form/form/parse_config.cpp Outdated Show resolved Hide resolved

gemmeren reviewed Aug 21, 2025

View reviewed changes

form/form/parse_config.hpp Outdated Show resolved Hide resolved

form/mock_phlex/phlex_toy_config.cpp Outdated Show resolved Hide resolved

form/mock_phlex/phlex_toy_config.hpp Outdated Show resolved Hide resolved

form/util/factories.hpp Show resolved Hide resolved

pcanal reviewed Aug 21, 2025

View reviewed changes

form/mock_phlex/phlex_toy_core.hpp Outdated Show resolved Hide resolved

Replace magic numbers in factories with Technology constants. Add hel…

3de659d

…per constants to technology.hpp and update factories.hpp to use named constants instead of hardcoded values. Addresses code review feedback.

pcanal reviewed Aug 21, 2025

View reviewed changes

form/util/factories.hpp Outdated Show resolved Hide resolved

pcanal reviewed Aug 21, 2025

View reviewed changes

form/util/factories.hpp Outdated Show resolved Hide resolved

pcanal reviewed Aug 21, 2025

View reviewed changes

form/util/factories.hpp Outdated Show resolved Hide resolved

Implement Technology helper functions and fix mock_phlex namespace is…

5e0e099

…sues

gemmeren reviewed Aug 22, 2025

View reviewed changes

form/form/technology.hpp Outdated Show resolved Hide resolved

pcanal reviewed Aug 22, 2025

View reviewed changes

form/form/technology.hpp Outdated Show resolved Hide resolved

Modernize technology constants with constexpr and named values

fff4cf7

- Replace magic numbers with constexpr constants and Combine() helper - Add HDF5 placeholder - Remove redundant form/ directory from include paths Update CMake and source files to match simplified directory structure

gemmeren approved these changes Aug 22, 2025

View reviewed changes

pcanal approved these changes Aug 22, 2025

View reviewed changes

knoepfel reviewed Aug 22, 2025

View reviewed changes

Move destructors to source files and improve initialization

9f1b0c8

knoepfel merged commit 3cd9de7 into Framework-R-D:main Aug 25, 2025
1 check passed

aolivier23 mentioned this pull request Sep 24, 2025

FORM container config #36

Merged

aolivier23 added a commit that referenced this pull request Sep 24, 2025

Restored PR #22 upgrades to Persistence while keeping new

45639de

technology-specific configuration system.

		const std::string& top_name();
		const std::string& col_name();


		Storage_Container::Storage_Container(const std::string& name) : m_name(name), m_file(nullptr) {}

		Storage_Container::~Storage_Container() {}

	return std::unique_ptr<IPersistence>(new Persistence(config));
	return std::make_unique<Persistence>(config);

Use configuration for I/O, plus general cleanup #22

Use configuration for I/O, plus general cleanup #22

Uh oh!

Conversation

barnaliy commented Aug 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemmeren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barnaliy commented Aug 22, 2025

Uh oh!

Uh oh!

Uh oh!

gemmeren left a comment

Choose a reason for hiding this comment

Uh oh!

pcanal left a comment

Choose a reason for hiding this comment

Uh oh!

knoepfel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knoepfel Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

knoepfel Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

knoepfel Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

knoepfel Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

barnaliy Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants