BlueBrain / libsonata

A python and C++ interface to the SONATA format
https://libsonata.readthedocs.io/en/stable/
GNU Lesser General Public License v3.0
12 stars 12 forks source link

CircuitConfig.expanded_json does not expand relative paths #152

Open joni-herttuainen opened 3 years ago

joni-herttuainen commented 3 years ago

E.g., with a config file having nodes_file and edges_file defined as follows:

{
    "version": 2,
    "networks": {
        "nodes": [
            {
                "nodes_file": "./nodes.h5",
                ...
            }
        ],
        "edges": [
            {
                "edges_file": "edges.h5",
                ...
                }
            }
        ]
    }
}

I can access the data in the the files but when getting the expanded_json but I don't have the full paths as I would've expected: '{"networks":{"edges":[{"edges_file":"edges.h5",...}],"nodes":[{"nodes_file":"./nodes.h5",...}]},"version":2}'

NadirRoGue commented 3 years ago

Hello, The expanded JSON function replaces the manifest variables anywhere in the file where they are being used, like in this example:

{
  "target_simulator": "NEURON",
  "manifest": {
    "$BASE_DIR": "./",
    "$COMPONENT_DIR": "$BASE_DIR",
    "$NETWORK_DIR": "../"
  },
  "components": {
    "morphologies_dir": "$COMPONENT_DIR/morphologies",
    "biophysical_neuron_models_dir": "$COMPONENT_DIR/biophysical_neuron_models"
  },
  "node_sets_file": "$BASE_DIR/node_sets.json",
  "networks": {
    "nodes": [
      {
        "nodes_file": "$NETWORK_DIR/nodes1.h5",
        "node_types_file": null
      }
    ],
    "edges": [
      {
        "edges_file": "$NETWORK_DIR/edges1.h5",
        "edge_types_file": null
      }
    ]
  }
}

In this case, after replacing, the paths would be still relative, since the manifest variables are as well.

mgeplf commented 3 years ago

Yeah, I asked @joni-herttuainen to open this in case we wanted to discuss whether it should return an absolute path or not.

I can see the benefits for both, and the downsides; I'm not sure what the best solution is though. Having an extra argument to return_absolute_paths doesn't seem right.

However, pushing resolution down the library consumer is annoying - I doubt that the pwd when reading the files contained in the config is set to the same dir as the config, so relative paths will have to be manipulated and how best to address this is the goal of the ticket.

How are you handling this @NadirRoGue?

NadirRoGue commented 3 years ago

I have prepared the fix for this as well in case its decided to be commited. I only have to modify the nlohman::json object everytime a path is converted to absolute during parsing, and the returned paths by the expand JSON function returns the absolute ones.

I can open a PR if needed.

mgeplf commented 3 years ago

Wow, that's cool; if you already have something, I wouldn't mind seeing it - if you could push a branch, that would be helpful. Doesn't have to be polished or anything, just to get a sense of how it would work.

NadirRoGue commented 3 years ago

Here is the branch: https://github.com/BlueBrain/libsonata/tree/config_file_not_expanding (edit: its a bad branch name for what it fixed, apologies for that)