Normalize config files by duizendnegen · Pull Request #3920 · Project-OSRM/osrm-backend

duizendnegen · 2017-04-08T20:13:23Z

Issue

Normalizing the usage & file names across config files by referring to a single config file.

Tasklist

duizendnegen · 2017-04-08T20:14:42Z

Currently pointed config files to use storage_config. This is slightly confusing as IsValid doesn't make sense now in all cases.

Alternatively we can create an io_config, either as a reference or as a parent class. Welcoming suggestions.

TheMarex · 2017-04-10T08:28:44Z

What is the motivation for using StorageConfig in all modules? It is meant as the configuration for the storage component, what is the relation with the other modules?

duizendnegen · 2017-04-11T15:41:12Z

No motivation - I misunderstood it at the start and then just got it to a working situation. I will replace that. What's your opinion of using io_config as a parent class (option A) or as a reference / variable (option B)?

duizendnegen · 2017-04-29T15:22:45Z

Ready for review!

daniel-j-h

What do we guarantee wrt. our public headers? (cc @TheMarex) I can see some modifications in publicly exposed config headers - not sure if this is fine for compatibility.

daniel-j-h · 2017-05-04T13:15:05Z

@@ -0,0 +1,82 @@
+#ifndef IO_CONFIG_HPP


Should probably be prefix with OSRM_

daniel-j-h · 2017-05-04T13:16:01Z

+{
+struct IOConfig
+{
+    IOConfig() = default;


Why is the default constructor needed? A default constructed IOConfig can't do anything, no?

Yes, I would prefer to drop this as a constructor, but in practice it doesn't work:

osrm-backend/src/tools/extract.cpp

Line 27 in bf6698f

return_code parseArguments(int argc, char *argv[], extractor::ExtractorConfig &extractor_config)

is how command line arguments are parsed, they require an instantiated version of the Config files. I started without that and without the UseDefaultOutputNames function, but I couldn't get it to work like that.

daniel-j-h · 2017-05-04T13:19:16Z

+        compressed_node_based_graph_path = {osrm_path.string() + ".cnbg"};
+        cnbg_ebg_mapping_path = {osrm_path.string() + ".cnbg_to_ebg"};
+        restriction_path = {osrm_path.string() + ".restrictions"};
+        intersection_class_data_path = {osrm_path.string() + ".icd"};


Hm is there a way we can make it so that we get a warning if we forget to initialize a path below?

Should this e.g. all be done in the constructor, as in

struct io { io(path base) : a(base + '.a'), b(base + '.b') { } path a; path b; };

?

See above - yes, should be the case, but didn't work out in practice.

Ok I see. Thanks for explaining! :)

daniel-j-h · 2017-05-04T13:21:38Z

Also needs a rebase onto master before we can merge 🎄

duizendnegen · 2017-07-07T13:35:21Z

From IRC: Hey _1009 there's a small backwards incompatible change in your current IOConfig PR - https://github.com/Project-OSRM/osrm-backend/pull/3920/files#diff-9b0fcadd9f362547a7dd4f631c4ec3fcR33 breaks the API. What you can do is make it protected and call the function in all sub-class constructors.

duizendnegen · 2017-07-11T18:59:18Z

Ping @daniel-j-h

daniel-j-h · 2017-07-12T16:37:58Z

    // Configure based on a .osrm base path, and no datasets in shared mem from osrm-datastore
    EngineConfig config;
-    config.storage_config = {argv[1]};
+    config.storage_config.UseDefaultOutputNames(argv[1]);


Hm user coder here needs to change this is still breaking the api

daniel-j-h · 2017-07-12T16:38:25Z

-            *v8::String::Utf8Value(Nan::To<v8::String>(args[0]).ToLocalChecked()));
+        std::string base_path(*v8::String::Utf8Value(args[0]->ToString()));
+        engine_config->storage_config = osrm::StorageConfig();
+        engine_config->storage_config.UseDefaultOutputNames(base_path);


daniel-j-h · 2017-07-12T16:38:30Z

-            osrm::StorageConfig(*v8::String::Utf8Value(Nan::To<v8::String>(path).ToLocalChecked()));
+        std::string base_path(*v8::String::Utf8Value(path->ToString()));
+        engine_config->storage_config = osrm::StorageConfig();
+        engine_config->storage_config.UseDefaultOutputNames(base_path);


daniel-j-h · 2017-07-12T16:38:43Z

    // Configure based on a .osrm base path, and no datasets in shared mem from osrm-datastore
    EngineConfig config;
-    config.storage_config = {argv[1]};
+    config.storage_config.UseDefaultOutputNames(argv[1]);


daniel-j-h · 2017-07-12T16:40:24Z

Ah I've just seen you added a constructor calling UseDefaultOutputNames. Can you switch all the public code to using that and make the UseDefaultOutputNames protected so only sub-classes can call it.

duizendnegen · 2017-07-13T04:18:09Z

But in previous versions most subclasses had UseDefaultOutputNames type of constructs, right...? This is why the initialisation from command line worked. Are you proposing making it protected (for storage_config) and public in customizer_config, extractor_config, ...?

daniel-j-h · 2017-07-13T08:52:26Z

Yeah the user should not see the UseDefaultOutputNames function on the publicly exposed configs. And especially should not have to adapt her code to call this function.

… to use new io_config constructor

duizendnegen · 2017-07-14T09:02:09Z

Addressed comments (waiting for builds to pass, might have to do some clang-formatting, feel free to leave additional comments)

daniel-j-h · 2017-07-14T10:14:16Z

Yep Travis complains about clang-formating include/storage/io_config.hpp - otherwise looks good!

duizendnegen · 2017-07-15T08:55:00Z

👍 formatted the file, now waiting for 📗 🍏

duizendnegen · 2017-07-15T11:13:36Z

One test seemed to have timed out, could you restart?

daniel-j-h · 2017-07-20T10:03:01Z

Thanks for this - it's making some I/O parts nicer for sure! I just merged it into master.

✨

daniel-j-h added Refactor Work In Progress labels Apr 11, 2017

duizendnegen force-pushed the feature/config-normalization branch 2 times, most recently from 9502ca7 to 1416aea Compare April 18, 2017 08:55

duizendnegen mentioned this pull request Apr 18, 2017

node_levels name introduces confusion #3499

Open

duizendnegen force-pushed the feature/config-normalization branch 7 times, most recently from b125b82 to c8a68e5 Compare April 24, 2017 21:00

duizendnegen force-pushed the feature/config-normalization branch 9 times, most recently from f4449fb to 447b534 Compare April 29, 2017 12:46

daniel-j-h reviewed May 4, 2017

View reviewed changes

duizendnegen force-pushed the feature/config-normalization branch 3 times, most recently from 9c94502 to 80c30b4 Compare May 5, 2017 09:03

daniel-j-h requested changes Jul 12, 2017

View reviewed changes

duizendnegen force-pushed the feature/config-normalization branch from 3224072 to c2dbf22 Compare July 14, 2017 05:50

duizendnegen added 11 commits July 14, 2017 10:55

superclass storage_config with io_config

9f209ee

superclass updater_config with io_config

917a2dc

superclass contractor_config with io_config

35bc9e4

superclass partition_config with io_config

816bedc

superclass customizer_config with io_config

18e6342

superclass extractor_config with io_config

5bd0c1c

rename osrm_input_path to osrm_path, clang-format

fc6d9db

Fix rebase problems

bf1174e

expose mandatory / optional / output files in io_config; config files…

ce16fb0

… to use new io_config constructor

Use GetPath with file names over accessing member variables

4461093

Addressed comments

d2cd3d1

duizendnegen force-pushed the feature/config-normalization branch from c2dbf22 to 3c2c1a5 Compare July 14, 2017 08:57

Maintain storage_config exposed API

5c520e4

duizendnegen force-pushed the feature/config-normalization branch from 3c2c1a5 to 5c520e4 Compare July 15, 2017 08:55

daniel-j-h approved these changes Jul 16, 2017

View reviewed changes

daniel-j-h merged commit 8da4041 into Project-OSRM:master Jul 20, 2017

duizendnegen deleted the feature/config-normalization branch July 20, 2017 18:44

afarber mentioned this pull request Jan 30, 2026

Add --list-inputs option to all OSRM tools for deployment scripts #7350

Merged

5 tasks

Uh oh!

Conversation

duizendnegen commented Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tasklist

Uh oh!

duizendnegen commented Apr 8, 2017

Uh oh!

TheMarex commented Apr 10, 2017

Uh oh!

duizendnegen commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duizendnegen commented Apr 29, 2017

Uh oh!

daniel-j-h 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-j-h commented May 4, 2017

Uh oh!

duizendnegen commented Jul 7, 2017

Uh oh!

duizendnegen commented Jul 11, 2017

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!

daniel-j-h commented Jul 12, 2017

Uh oh!

duizendnegen commented Jul 13, 2017

Uh oh!

daniel-j-h commented Jul 13, 2017

Uh oh!

duizendnegen commented Jul 14, 2017

Uh oh!

daniel-j-h commented Jul 14, 2017

Uh oh!

duizendnegen commented Jul 15, 2017

Uh oh!

duizendnegen commented Jul 15, 2017

Uh oh!

daniel-j-h commented Jul 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

duizendnegen commented Apr 8, 2017 •

edited

Loading

duizendnegen commented Apr 11, 2017 •

edited

Loading