Consolidate tools into a single command-line interface #753

dabreegster · 2021-09-15T17:24:07Z

#715 has the motivation. This is a pretty massive change; feel free to just focus on cli/src/main.rs, which uses structopt.

Doc changes are at https:/a-b-street/docs/tree/cli. I'll also need to update the abstr docs/vignettes.

Github actions is creating a build now; I'll try the map and scenario importers from the UI there to sanity check everything is named correctly.

…tools. #745

Remove the ability for the building procgen tool to generate a map from OSM. If we still need that, can just do two steps. It complicates the flags.

one_step_import call library functions directly now. #745

[rebuild]

dabreegster · 2021-09-15T17:25:33Z

cli/src/geojson_to_osmosis.rs

+
+use geom::LonLat;
+
+pub fn run(path: String) -> Result<()> {


I changed to take a file as a path, instead of read from STDIN. Much easier to use this as a library call from one_step_import.rs.

small nit/reminder: Path/PathBuffer are nice and should probably be preferred for this kind of thing... though maybe this method has sufficient interop to make that an undesirably cascading change.

dabreegster · 2021-09-15T17:27:25Z

cli/src/main.rs

+ },
+ /// Runs the main A/B Street importer, which manages maps and scenarios for many cities.
+ Import {
+ /// See the importer's source code for the defined flags. You should first pass a bare "--"


I gave up on defining all of the flags here, for now. :\ I need to learn more structopt to understand how to do enumerations of flags -- like if you pass --oneshot, only then does --oneshot_drive_on_left become a valid flag. Possibly the proper way to do this is simply split out two subcommands, for a "normal" import and for a one-shot.

dabreegster · 2021-09-15T17:28:04Z

cli/src/one_step_import.rs

- stdin.write_all(&geojson)?;
- assert!(cmd.wait()?.success());
-
+ crate::geojson_to_osmosis::run(geojson_path.clone())?;


No more running subprocesses and parsing STDOUT!

Robinlovelace · 2021-09-15T21:15:16Z

Looking like a mega refactor, look forward to giving it a spin!

dabreegster · 2021-09-18T01:29:18Z

Testing the .zip build went fine (by which I mean I found and fixed some bugs). Going to merge now ahead of the release, but happy to keep evolving things.

michaelkirk · 2021-09-22T18:46:27Z

#715 has the motivation

That's a long thread, so I might have missed it, but I couldn't find the part that was relevant to this CLI refactor - is that the right issue number? Or can you link to the conversation in question?

dabreegster · 2021-09-22T19:06:17Z

I meant #745, whoops, got the number totally wrong

michaelkirk

This is really great. I'm personally most excited about the discoverability of the commands and their parameters:

$ target/debug/cli
abcli 0.1.0
The A/B Street multi-tool

USAGE:
    cli <SUBCOMMAND>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

SUBCOMMANDS:
    augment-scenario       Modifies the schedule of every person in an existing scenario
    clip-osm               Clips an OSM file to a boundary. This is a simple Rust port of `osmconvert large_map.osm
                           -B=clipping.poly --complete-ways -o=smaller_map.osm`
    dump-json              Print a binary map or scenario file as JSON
    generate-houses        Procedurally generates houses along empty residential roads of a map
    geo-json-to-osmosis    Reads a GeoJSON file, extracts a polygon from every feature, and writes numbered files in
                           the https://wiki.openstreetmap.org/wiki/Osmosis/Polygon_Filter_File_Format format as
                           output
    help                   Prints this message or the help of the given subcommand(s)
    import                 Runs the main A/B Street importer, which manages maps and scenarios for many cities
    import-grid2-demand    Import a scenario from https:/asu-trans-ai-lab/grid2demand
    import-json-map        Transform a JSON map that's been manually edited into the binary format suitable for
                           simulation
    import-scenario        Import a JSON scenario in the https://a-b-
                           street.github.io/docs/tech/dev/formats/scenarios.html format
    minify-map             Removes nonessential parts of a Map, for the bike network tool
    one-step-import        Imports a one-shot A/B Street map in a single command
    pick-geofabrik         Prints the osm.pbf file from download.geofabrik.de that covers a given boundary
    random-scenario        Generates a random scenario using the proletariat robot travel demand model

$ target/debug/cli dump-json   
error: The following required arguments were not provided:
    <path>

USAGE:
    cli dump-json <path>

For more information try --help

$ target/debug/cli dump-json --help
cli-dump-json 0.1.0
Print a binary map or scenario file as JSON

USAGE:
    cli dump-json <path>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

ARGS:
    <path>

One note, cli is kind of a rough name and likely to conflict. Maybe renaming it to abs or abscli would help? But it's not a deal breaker.

michaelkirk · 2021-09-22T18:50:15Z

cli/src/geojson_to_osmosis.rs

+
+use geom::LonLat;
+
+pub fn run(path: String) -> Result<()> {


small nit/reminder: Path/PathBuffer are nice and should probably be preferred for this kind of thing... though maybe this method has sufficient interop to make that an undesirably cascading change.

michaelkirk · 2021-09-22T19:08:33Z

data/bbike.sh

@@ -1,17 +0,0 @@
-#!/bin/bash
-# This downloads all the cities from https://download.bbbike.org/osm/bbbike/
-# and tries to convert them. Don't eat bbike's bandwidth, please.


Did bbike.sh and mass_import.sh just go away? Old unused code?

Yup, these were from https:/a-b-street/docs/blob/ecce62e8bd59439d2d01e799e1dcba39418e71e6/book/src/tech/dev/mass_import.md, when I was trying to import a bunch of city centers for a demo to the OSM community. The scripts stopped working a while ago, and I don't have plans to attempt something like this again, so cleanup time

michaelkirk · 2021-09-22T19:12:56Z

cli/src/main.rs

@@ -0,0 +1,261 @@
+//! A collection of tools, mostly related to importing maps and scenarios. These are bundled as a
+//! single executable to reduce the cost of static linking in the release's file size.


ya know, I didn't really think about the binary size implications. But that makes sense. 👍

dabreegster · 2021-09-22T19:22:19Z

One note, cli is kind of a rough name and likely to conflict. Maybe renaming it to abs or abscli would help?

Added a TODO. This is a case when I sort of miss the org.abstreet.blah naming scheme from the JVM world. abst becomes a sort of prefix to lots of stuff.

Path/PathBuffer are nice and should probably be preferred for this kind of thing

Added to the TODO in #745. I think this one would be great to do, but it's a large refactoring project. To do it properly, lots of abstio things need to change.

dabreegster added 6 commits September 14, 2021 11:53

Start a consolidated CLI tool with a few one of the importer crate's …

226243e

…tools. #745

Folding more tools into the CLI tool. #745

f4ecc8c

The CLI consumes even more. #745

b12e0c5

Remove the ability for the building procgen tool to generate a map from OSM. If we still need that, can just do two steps. It complicates the flags.

Add a few more tools to the CLI, introducing tokio and making

50428cf

one_step_import call library functions directly now. #745

Make the importer itself a library, called from the CLI tool. #745

47b4c35

Fix up references to the CLI tools in shell scripts and code. #745

2b8df09

[rebuild]

dabreegster requested a review from michaelkirk September 15, 2021 17:24

dabreegster commented Sep 15, 2021

View reviewed changes

dabreegster mentioned this pull request Sep 15, 2021

Finish the 'Activity models' vignette a-b-street/abstr#76

Closed

4 tasks

dabreegster force-pushed the cli branch from a3e28f2 to 360dc1e Compare September 15, 2021 17:55

Delete 3 old scripts used to mass-import maps. #745 [rebuild]

b4f9a7f

dabreegster force-pushed the cli branch from 360dc1e to b4f9a7f Compare September 15, 2021 19:25

dabreegster merged commit ca4ddeb into master Sep 18, 2021

dabreegster deleted the cli branch September 18, 2021 01:29

michaelkirk reviewed Sep 22, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate tools into a single command-line interface #753

Consolidate tools into a single command-line interface #753

dabreegster commented Sep 15, 2021

dabreegster Sep 15, 2021

michaelkirk Sep 22, 2021

dabreegster Sep 15, 2021

dabreegster Sep 15, 2021

Robinlovelace commented Sep 15, 2021

dabreegster commented Sep 18, 2021

michaelkirk commented Sep 22, 2021

dabreegster commented Sep 22, 2021

michaelkirk left a comment

michaelkirk Sep 22, 2021

michaelkirk Sep 22, 2021

dabreegster Sep 22, 2021

michaelkirk Sep 22, 2021

dabreegster commented Sep 22, 2021

		@@ -0,0 +1,261 @@
		//! A collection of tools, mostly related to importing maps and scenarios. These are bundled as a
		//! single executable to reduce the cost of static linking in the release's file size.

Consolidate tools into a single command-line interface #753

Consolidate tools into a single command-line interface #753

Conversation

dabreegster commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robinlovelace commented Sep 15, 2021

dabreegster commented Sep 18, 2021

michaelkirk commented Sep 22, 2021

dabreegster commented Sep 22, 2021

michaelkirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster commented Sep 22, 2021