Fix start of osrm-contract #469

greenscientist · 2022-11-24T19:34:53Z

osrm-contract expect the file prefix when starting. I don't know how the previous command worked

tahini · 2022-11-25T02:55:40Z

Really? Here's the output of osrm-contract --help:

$ osrm-contract --help
Usage: osrm-contract <input.osrm> [options]:
[...]
$ osrm-contract --version
v5.27.1

greenscientist · 2022-11-25T03:10:24Z

Yep, that's wrong

tahini · 2022-11-25T03:46:29Z

What's wrong? the osrm-contract help? If so, since when? Was there a recent change? Would this fix fail on some versions?

It works here because the file is called ${mode}.osrm after extract, so by setting [mode] for <input.osrm> it looks for <mode>.osrm?

greenscientist · 2022-11-25T14:57:03Z

So I don't have a file $mode.osrm anymore here. Might be a new version thing.
I guess we should test with older version of osrm.
I based this on the output of osrm-extract which said:
"To prepare the data for routing, run: ./osrm-contract "/app/examples/runtime/osrm/walking/walking""
Need to go check the code histoire in osrm

greenscientist · 2022-11-25T15:34:58Z

So checking the behavior of the osrm code:
It takes the parameter and first strip out any known extention before using it as a base path

So basically, mode.osrm is the same as mode or mode.osm
It does not check that the specific file actually exist, but the rebuild the path to require file base in this base path.

We can still investigate why we do not have an "mode".osrm file generated anymore.

Otherwise, we can keep the diff as is, or we can make it so the cmd is with "$mode.osrm" instead of just "$mode".

greenscientist · 2022-11-25T16:20:07Z

commit 21888334dd1e0fbbd25cc62c725eb33bb7968147
Author: Siarhei Fedartsou siarhei.fedartsou@gmail.com
Date: Fri Sep 30 14:29:10 2022 +0200

Do not generate intermediate .osrm file in osrm-extract. (#6354)

greenscientist · 2022-11-25T16:26:02Z

Project-OSRM/osrm-backend#6354

greenscientist · 2022-11-25T16:26:48Z

it's super confusing and documentation is not well updated yet.

So I'll change this PR to be mode.osrm

tahini · 2022-11-25T21:42:07Z

With version 5.25, both the current and preceding version work. With 5.27, the contraction works, but the runtime doesn't. Don't you need to update also the osrm-routed path in the OSRMProcessManager.ts file?

greenscientist · 2022-11-25T21:58:55Z

hum, probably yes

greenscientist · 2022-11-26T00:05:56Z

updated, @tahini if you have the chance to test on your side, let me know

tahini · 2022-11-28T15:35:42Z

packages/chaire-lib-backend/src/utils/processManagers/OSRMServicePreparation.ts

+        // contract parameter need to match the prefix of the osm file passed to extract
+        // in our case it's the mode name. The .osrm is not generated in the latest osrm-extract
+        // but the name is still accepted as a base path
+        const osrmProcess = spawn('osrm-contract', [`${mode}.osrm`], {


There's another one in the restart function, which is the one that used. But I confirm that ${mode}.osrm works with both 5.25 and 5.27.1, but *.osrm fails with 5.27.1. Maybe call the start from the restart, or do we need both?

I guess might have a leftover .osrm file locally that made my test pass.

There's a subtle difference between them. But I've now put the args in a common function.

tahini · 2022-11-28T15:36:35Z

packages/chaire-lib-backend/src/utils/processManagers/OSRMProcessManager.ts

@@ -130,7 +130,7 @@ const start = function(parameters = {} as any): Promise<any> {
    return new Promise((resolve, reject) => {
        const command = 'osrm-routed';
        const commandArgs = [
-            osrmDirectoryPath + '/*.osrm',
+            osrmDirectoryPath + `/${mode}.osrm`,


The other comment applies to this line, not the one from OSRMServicePreparation, sorry.

greenscientist · 2022-11-28T18:33:37Z

I've made sure to erase the old .osrm in my latest test round.

tahini

It corresponds to what was working here when tested with both 5.25 and 5.27.1.

Just remove the whitespace so we don't need to add a commit to run yarn format later and it's good

tahini · 2022-11-28T19:19:42Z

packages/chaire-lib-backend/src/utils/processManagers/OSRMProcessManager.ts

@@ -118,6 +118,14 @@ const configureAllOsrmServers = async function(startServers = true): Promise<voi
    }
 };

+function getOsrmRoutedStartArgs(osrmDirectory: string, mode: string, port: string) : string[] {


yarn format would remove a whitespace before the ':' on this line

osrm-contract expect the file prefix when starting. The latest osrm-extract don't generate a .osrm file anymore by default (only in debug mode) so we cannot use the *.osrm trick anymore. We have to pass the actual file name prefix

greenscientist · 2022-11-28T21:31:32Z

Fixed my yarn format command, so should be good

greenscientist requested a review from tahini November 24, 2022 19:34

greenscientist force-pushed the startosrmcontract branch from 40e621e to 59ee3e5 Compare November 25, 2022 16:44

greenscientist force-pushed the startosrmcontract branch from 59ee3e5 to 11b3b4e Compare November 26, 2022 00:05

tahini requested changes Nov 28, 2022

View reviewed changes

tahini reviewed Nov 28, 2022

View reviewed changes

greenscientist force-pushed the startosrmcontract branch from 11b3b4e to a3ddb48 Compare November 28, 2022 18:32

tahini requested changes Nov 28, 2022

View reviewed changes

Fix start of osrm-contract

7876015

osrm-contract expect the file prefix when starting. The latest osrm-extract don't generate a .osrm file anymore by default (only in debug mode) so we cannot use the *.osrm trick anymore. We have to pass the actual file name prefix

greenscientist force-pushed the startosrmcontract branch from a3ddb48 to 7876015 Compare November 28, 2022 21:30

tahini approved these changes Nov 28, 2022

View reviewed changes

tahini merged commit 80b895a into chairemobilite:main Nov 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix start of osrm-contract #469

Fix start of osrm-contract #469

greenscientist commented Nov 24, 2022

tahini commented Nov 25, 2022

greenscientist commented Nov 25, 2022

tahini commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 25, 2022

tahini commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 26, 2022

tahini Nov 28, 2022

greenscientist Nov 28, 2022

greenscientist Nov 28, 2022

tahini Nov 28, 2022

greenscientist commented Nov 28, 2022

tahini left a comment

tahini Nov 28, 2022

greenscientist commented Nov 28, 2022

Fix start of osrm-contract #469

Fix start of osrm-contract #469

Conversation

greenscientist commented Nov 24, 2022

tahini commented Nov 25, 2022

greenscientist commented Nov 25, 2022

tahini commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 25, 2022

tahini commented Nov 25, 2022

greenscientist commented Nov 25, 2022

greenscientist commented Nov 26, 2022

tahini Nov 28, 2022

Choose a reason for hiding this comment

greenscientist Nov 28, 2022

Choose a reason for hiding this comment

greenscientist Nov 28, 2022

Choose a reason for hiding this comment

tahini Nov 28, 2022

Choose a reason for hiding this comment

greenscientist commented Nov 28, 2022

tahini left a comment

Choose a reason for hiding this comment

tahini Nov 28, 2022

Choose a reason for hiding this comment

greenscientist commented Nov 28, 2022