unspecified attributes on load are no longer defaulted by DanielLacina · Pull Request #2688 · Pometry/Raphtory

DanielLacina · 2026-07-22T17:39:12Z

What changes were proposed in this pull request?

Synced config to disk on Graph.load, replaced Config with ConfigArgs so program knows what attributes are specified
to help get rid of unspecified attributes on load are no longer defaulted undesirable behavior.
Also, added tests to test graph config.

Why are the changes needed?

Unspecified attributes on load are defaulted. It automatically overrides previous config attributes if they're not specified which is undesirable. Config should be synced to disk after load.

Does this PR introduce any user-facing change? If yes is this documented?

Yes. Changing Config with ConfigArgs causes API change.

How was this patch tested?

Tested Graph.load and Graph behavior with config specified and used assertions to verify the config is in the right state.

Are there any further changes required?

Maybe

…alues and new config flush on graph load

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite	Current: `3e65345`	Previous: `9823ef7`	Ratio
`lotr_graph/num_edges`	`3` ns/iter (`± 0`)	`0` ns/iter (`± 0`)	`+∞`
`lotr_graph/num_nodes`	`4` ns/iter (`± 0`)	`1` ns/iter (`± 0`)	`4`
`lotr_graph/graph_latest`	`3` ns/iter (`± 0`)	`0` ns/iter (`± 0`)	`+∞`
`lotr_graph_materialise/materialize`	`7634180` ns/iter (`± 41997`)	`1564816` ns/iter (`± 35303`)	`4.88`
`lotr_graph_window_100/num_nodes`	`11` ns/iter (`± 0`)	`5` ns/iter (`± 0`)	`2.20`
`lotr_graph_window_100_materialise/materialize`	`7620110` ns/iter (`± 107762`)	`1669150` ns/iter (`± 10700`)	`4.57`
`lotr_graph_window_10_materialise/materialize`	`2987981` ns/iter (`± 10377`)	`971980` ns/iter (`± 4278`)	`3.07`
`lotr_graph_subgraph_10pc/num_nodes`	`17` ns/iter (`± 0`)	`4` ns/iter (`± 0`)	`4.25`
`lotr_graph_subgraph_10pc_materialise/materialize`	`1996701` ns/iter (`± 23596`)	`334634` ns/iter (`± 1287`)	`5.97`
`lotr_graph_subgraph_10pc_windowed_materialise/materialize`	`1175941` ns/iter (`± 17638`)	`230399` ns/iter (`± 2617`)	`5.10`
`lotr_graph_window_50_layered/has_node_existing`	`361` ns/iter (`± 19`)	`129` ns/iter (`± 12`)	`2.80`
`lotr_graph_window_50_layered_materialise/materialize`	`25670356` ns/iter (`± 160777`)	`3488825` ns/iter (`± 24948`)	`7.36`
`lotr_graph_persistent_window_50_layered/num_edges_temporal`	`516766` ns/iter (`± 5048`)	`192686` ns/iter (`± 1569`)	`2.68`
`lotr_graph_persistent_window_50_layered/has_node_existing`	`386` ns/iter (`± 320`)	`174` ns/iter (`± 83`)	`2.22`
`lotr_graph_persistent_window_50_layered_materialise/materialize`	`43449211` ns/iter (`± 187043`)	`5298035` ns/iter (`± 147912`)	`8.20`

This comment was automatically generated by workflow using github-action-benchmark.

fabubaker

Most methods in raphtory-graphql should just take Config, since the server will parse the args and convert them into a Config on startup. The server exclusively uses MaterializedGraph which is not exposed in the python API, so the constructors for MaterializedGraph should also just take a Config.

In core raphtory, the user-facing constructor methods in Graph and PersistentGraph are the only ones that need to take a ConfigArgs.

TLDR; ConfigArgs are only needed at the end-user boundary which are the constructor methods exposed through python and the ServerArgs on raphtory-graphql.

fabubaker · 2026-07-28T17:02:53Z

@@ -521,33 +522,36 @@ impl ValidWriteableGraphFolder {
    pub fn write_graph_data(


Unrelated to changes in this PR, but can you add a newline above to separate methods?

fabubaker · 2026-07-28T18:23:12Z


    fn new(config: Self::Config, graph_dir: Option<&Path>) -> Result<Self, StorageError>;

    fn load(graph_dir: &Path) -> Result<Self, StorageError>;


Rather than associate type ConfigArgs with PersistenceStrategy, I think it makes sense to move it to ConfigOps.

fabubaker · 2026-07-28T18:26:40Z

+}
+
 pub trait ConfigOps: Serialize + DeserializeOwned + Args + Sized {
+    type NewConfigArgs: ConfigArgsOps;


To follow up on #2688 (comment), you would change this to type ConfigArgs and use () for BaseConfig and WriteAndMergeConfigArgs for WriteAndMergeConfig.

fabubaker · 2026-07-28T20:55:43Z

-    pub fn load_with_config(path: impl AsRef<Path>, config: Config) -> Result<Self, GraphError> {
+    pub fn load_with_config(
+        path: impl AsRef<Path>,
+        config_args: ConfigArgs,


I'd like Storage to take an already built Config (we seem to be doing that with new anyways).

In graph.rs, have ConfigArgs get converted into a Config and pass it down to the relevant storage. This is the right boundary for the conversion to happen.

You'll have to change PersistenceStrategy::load_with_config to also take in a Config. I recommend creating a ConfigArgs::load_from_path method that internally calls update and combines the passed-in args + args on disk. The caller can then call into_config and pass the generate Config into Storage.

fabubaker · 2026-07-28T20:58:37Z

    ) -> Result<MaterializedGraph, ClientError> {
        let encoded = self.receive_graph(path).await?;
-        url_decode_graph(encoded, Config::default()).map_err(ClientError::from)
+        url_decode_graph(encoded, ConfigArgs::default()).map_err(ClientError::from)


This should take a Config.

fabubaker · 2026-07-28T20:58:49Z

        let folder_clone = folder.clone();
        let g: MaterializedGraph = blocking_compute(move || {
-            url_decode_graph_at(graph, folder_clone.graph_folder(), config)
+            url_decode_graph_at(graph, folder_clone.graph_folder(), config.into_args())


This should be a Config.

fabubaker · 2026-07-28T21:01:27Z

+        let server = block_on(GraphServer::new(
+            work_dir,
+            app_config,
+            ConfigArgs::default(),


This should be a Config and ServerArgs.graph_config should be a ConfigArgs.

fabubaker · 2026-07-28T21:02:23Z

                blocking_compute(move || {
-                    let (is_dirty, new_graph) = writeable_folder.write_graph_data(graph, config)?;
+                    let (is_dirty, new_graph) =
+                        writeable_folder.write_graph_data(graph, config.into_args())?;


This should just take Config.

fabubaker · 2026-07-28T21:03:00Z

            &cache,
            create_index,
-            config,
+            config_args,


This should just take Config.

fabubaker · 2026-07-29T15:42:09Z

+    fn max_node_page_len(&self) -> Option<u32>;
+
+    fn max_edge_page_len(&self) -> Option<u32>;
+
+    fn node_types(&self) -> &[String];
+
+    fn with_max_node_page_len(self, page_len: u32) -> Self;
+
+    fn with_max_edge_page_len(self, page_len: u32) -> Self;
+
+    fn with_node_types(&self, node_types: impl IntoIterator<Item = impl AsRef<str>>) -> Self;


I don't think you need these methods, all you need in this trait is into_config and load_from_path.

I think we could simplify it even further, all this needs is the update method to apply the config args to an existing config (where that may be loaded from disk or default) with signature (I would probably call this apply instead of update as well) fn apply(&self, config: &mut Self::Config)

with that, we can have a default implementation for into_config which is simply

fn into_config(self) -> Self::Config { let mut config = Self::Config::default(); self.apply(&mut config); config }

fabubaker · 2026-07-29T15:49:05Z

    }

-    fn update(&mut self, new: Self);
+    fn update(&mut self, new_args: Self::ConfigArgs);


We should move this update method into ConfigArgs.

fabubaker · 2026-07-29T15:52:02Z

+    max_edge_page_len: Option<u32>,
+}
+
+impl ConfigArgsOps for BaseConfigArgs {


I think you can remove this, we only need to implement ConfigArgsOps on WriteAndMergeConfigArgs.

fabubaker · 2026-07-29T15:52:13Z

+    }
+}
+
+impl ConfigArgsOps for () {


Remove this.

I think either this or the BaseConfigArgs will be needed if load_with_config ends up existing in the OS raphtory apis

fabubaker · 2026-07-29T16:43:15Z

+}
+
 pub trait ConfigOps: Serialize + DeserializeOwned + Args + Sized {
+    type ConfigArgs: ConfigArgsOps;


If we move load_from_dir and save_to_dir to ConfigArgs, it can be responsible for the full lifecycle of loading/saving config from disk.

I would keep those here (they should always return/save a full config, not a ConfigArgs), see my comment on ConfigArgs, it would only have a way to update an existing, config, nothing else.

once that is the case, this probably doesn't actually need the associated type at all

fabubaker · 2026-07-29T16:44:36Z

-    fn update(&mut self, new: Self);
+    fn update(&mut self, new_args: Self::ConfigArgs);
+
+    fn into_args(self) -> Self::ConfigArgs;


I think most places that call into_args should just pass a Config anyways?

We could then get rid of into_args.

agreed, this shouldn't be needed

The main current use for this is in the graphql server, maybe the better answer for that would be for the server to store config args instead of config?

fabubaker · 2026-07-29T16:48:48Z

    pub fn load_with_config(
        path: &(impl GraphPaths + ?Sized),
-        config: Config,
+        config_args: ConfigArgs,


This should be a Config but you'll run into problems below with Graph::load_with_config and PersistentGraph::load_with_config since those take ConfigArgs.

The way around this is to create a Storage object which takes Config and then convert that into a Graph or PersistentGraph.

I disagree on this one, this is an end-user facing function and should take ConfigArgs to update only the desired fields of the original graph on disk. This is used in rust when loading a graph where you want to preserve the type of the graph (Event or Persistent).

Co-authored-by: Fadhil Abubaker <fadhil.abubaker@pometry.com>

…h_config

This reverts commit 2d8fcac.

This reverts commit 14e05af.

… attributes

…to graph_config

ljeub-pometry

A bit more cleanup required

ljeub-pometry · 2026-08-06T08:29:51Z

    pub fn load_with_config(
        path: &(impl GraphPaths + ?Sized),
-        config: Config,
+        config_args: ConfigArgs,


I disagree on this one, this is an end-user facing function and should take ConfigArgs to update only the desired fields of the original graph on disk. This is used in rust when loading a graph where you want to preserve the type of the graph (Event or Persistent).

ljeub-pometry · 2026-08-06T08:45:19Z

+    fn max_node_page_len(&self) -> Option<u32>;
+
+    fn max_edge_page_len(&self) -> Option<u32>;
+
+    fn node_types(&self) -> &[String];
+
+    fn with_max_node_page_len(self, page_len: u32) -> Self;
+
+    fn with_max_edge_page_len(self, page_len: u32) -> Self;
+
+    fn with_node_types(&self, node_types: impl IntoIterator<Item = impl AsRef<str>>) -> Self;


I think we could simplify it even further, all this needs is the update method to apply the config args to an existing config (where that may be loaded from disk or default) with signature (I would probably call this apply instead of update as well) fn apply(&self, config: &mut Self::Config)

ljeub-pometry · 2026-08-06T08:47:17Z

+}
+
 pub trait ConfigOps: Serialize + DeserializeOwned + Args + Sized {
+    type ConfigArgs: ConfigArgsOps;


I would keep those here (they should always return/save a full config, not a ConfigArgs), see my comment on ConfigArgs, it would only have a way to update an existing, config, nothing else.

ljeub-pometry · 2026-08-06T08:47:50Z

-    fn update(&mut self, new: Self);
+    fn update(&mut self, new_args: Self::ConfigArgs);
+
+    fn into_args(self) -> Self::ConfigArgs;


agreed, this shouldn't be needed

ljeub-pometry · 2026-08-06T08:54:30Z

+}
+
 pub trait ConfigOps: Serialize + DeserializeOwned + Args + Sized {
+    type ConfigArgs: ConfigArgsOps;


once that is the case, this probably doesn't actually need the associated type at all

ljeub-pometry · 2026-08-06T08:57:25Z

+    }
+}
+
+impl ConfigArgsOps for () {


I think either this or the BaseConfigArgs will be needed if load_with_config ends up existing in the OS raphtory apis

ljeub-pometry · 2026-08-06T09:16:52Z

            } else {
-                let new_graph = graph.materialize_at_with_config(self.graph_folder(), config)?;
+                let new_graph =
+                    graph.materialize_at_with_config(self.graph_folder(), config.into_args())?;


This is an interesting one, one possibility is that the server should actually store a ConfigArgs so it can only override some of the fields and leave everything else as it was on the original graph. The other thing is that materialize_at_with_config should actually be able to override the page sizes as it is creating a new graph. If we want this to be as type safe as possible, we would need separate args for this case and the load case where the page sizes cannot be overridden. Alternatively, we could have separate apply methods for the two scenarios and just ignore/error at runtime for the page sizes when they cannot be changed.

ljeub-pometry · 2026-08-06T09:24:15Z

        path: impl AsRef<Path>,
        path_for_decoded_graph: Option<&Path>,
-        config: Config,
+        config_args: ConfigArgs,


This will mean that all the decode methods can just take a config.

ljeub-pometry · 2026-08-06T09:44:28Z

+    fn max_node_page_len(&self) -> Option<u32>;
+
+    fn max_edge_page_len(&self) -> Option<u32>;
+
+    fn node_types(&self) -> &[String];
+
+    fn with_max_node_page_len(self, page_len: u32) -> Self;
+
+    fn with_max_edge_page_len(self, page_len: u32) -> Self;
+
+    fn with_node_types(&self, node_types: impl IntoIterator<Item = impl AsRef<str>>) -> Self;


with that, we can have a default implementation for into_config which is simply

fn into_config(self) -> Self::Config { let mut config = Self::Config::default(); self.apply(&mut config); config }

ljeub-pometry · 2026-08-06T09:52:22Z

-    fn update(&mut self, new: Self);
+    fn update(&mut self, new_args: Self::ConfigArgs);
+
+    fn into_args(self) -> Self::ConfigArgs;


The main current use for this is in the graphql server, maybe the better answer for that would be for the server to store config args instead of config?

We already have unit tests in rust in cli.rs and integration-tests in python in test_cli_parsing.py

made it so config values not specified are not overriden by default v…

079f20d

…alues and new config flush on graph load

github-actions Bot reviewed Jul 22, 2026

View reviewed changes

DanielLacina added 7 commits July 24, 2026 10:36

recent

5476c77

change load api to accept config args instead of config

aa28a53

change load api to accept config args instead of config

623b906

made changes

8b0746b

Merge remote-tracking branch 'origin/db_v4' into graph_config

a6ff870

making changes

fb8f50f

more configurations

6c3015f

DanielLacina changed the title ~~made it so config values not specified are not overriden by default v…~~ unspecified attributes on load are no longer defaulted Jul 27, 2026

DanielLacina requested a review from fabubaker July 27, 2026 17:12

DanielLacina added 7 commits July 27, 2026 13:11

fix graphql error

c7f135b

format

e7ba250

working

3dc7489

fmt

296f532

fmt

b0ed925

fixing up interface

d234013

working

480e832

fabubaker reviewed Jul 28, 2026

View reviewed changes

DanielLacina and others added 2 commits July 28, 2026 18:03

working

56acd73

chore: apply tidy-public auto-fixes

22d6b95

fabubaker reviewed Jul 29, 2026

View reviewed changes

DanielLacina and others added 8 commits July 29, 2026 12:18

Update db4-storage/src/lib.rs

bd981c8

Co-authored-by: Fadhil Abubaker <fadhil.abubaker@pometry.com>

m

09bfbce

working

06ecbdf

working

6ca14a3

working

8d3be50

working

a13b70b

Merge branch 'db_v4' of https://github.com/Pometry/Raphtory into grap…

edae2ba

…h_config

working

cd6424d

DanielLacina and others added 22 commits July 30, 2026 19:52

Revert "m"

14250ae

This reverts commit 2d8fcac.

m

14e05af

Revert "m"

d0a8885

This reverts commit 14e05af.

working

40efc95

fix issues

9215c80

Rename ConfigArgs to Args

8f9b852

Move config input logic into Args

921f7b4

Split Config and Args into separate modules

cb73014

Run fmt

5696c05

Switch all public interfaces to use Args

44f5e35

Rename some config -> args

20df834

Run fmt

0a0811a

Merge branch 'db_v4' of github.com:Pometry/Raphtory into graph_config

69f7942

Run fmt

e21dd7d

Fix ConfigOps import

75e86ea

Run fmt

73cf8d1

chore: apply tidy-public auto-fixes

1270d78

fixed PyArgs parsing so it actually contains the passed in BaseConfig…

c64fe82

… attributes

Merge branch 'graph_config' of https://github.com/Pometry/Raphtory in…

6c29567

…to graph_config

fmt

759bae8

Bring back base.update

b015969

Save args to dir in constructors

ea908db

ljeub-pometry requested changes Aug 6, 2026

View reviewed changes

Use serde(default) on all Args

3f7dd81

fabubaker mentioned this pull request Aug 7, 2026

Test tracing and config for the server #2665

Open

2 tasks

fabubaker added 5 commits August 7, 2026 14:58

Move node_types out of args and back into config

a07c4d2

Remove raphtory-server cli parsing tests

921a426

We already have unit tests in rust in cli.rs and integration-tests in python in test_cli_parsing.py

Move graph assertion methods to assertions.rs

d1822c8

Get rid of defaults on Graph/PersistentGraph/Storage

e01a95e

Run fmt

3e65345

		@@ -521,33 +522,36 @@ impl ValidWriteableGraphFolder {
		pub fn write_graph_data(


		fn new(config: Self::Config, graph_dir: Option<&Path>) -> Result<Self, StorageError>;

		fn load(graph_dir: &Path) -> Result<Self, StorageError>;

Uh oh!

Conversation

DanielLacina commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change? If yes is this documented?

How was this patch tested?

Are there any further changes required?

Uh oh!

github-actions Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

fabubaker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljeub-pometry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DanielLacina commented Jul 22, 2026 •

edited

Loading

github-actions Bot left a comment •

edited

Loading

fabubaker left a comment •

edited

Loading