Deprecated OpenMobility Replaced With Mobility Database #2631

pastk · 2022-05-27T06:30:52Z

fardeenfs commented

2022-05-27 06:30:52 +00:00

(Migrated from github.com)

Open Mobility Data is no longer regularly updated/maintained and its API will soon be depreciated. It is the perfect time to replace it with its modified GTFS-feeds aggregator, Mobility Database. Updated the current code to support Mobility Database.

Signed-off-by: Fardeen Faisal fardeenfaisal.fs@gmail.com

Open Mobility Data is no longer regularly updated/maintained and its API will soon be depreciated. It is the perfect time to replace it with its modified GTFS-feeds aggregator, Mobility Database. Updated the current code to support Mobility Database. Signed-off-by: Fardeen Faisal <fardeenfaisal.fs@gmail.com>

pastk reviewed 2022-05-27 06:30:52 +00:00

strump reviewed 2022-05-27 07:58:55 +00:00

tools/python/transit/gtfs/download_gtfs.py

					
				@ -4,3 +4,4 @@

				                            - Mobility Database (https://mobilitydata.org/)  

				Crawls all the urls, loads feed zips and extracts to the specified directory."""

				import argparse

strump commented

2022-05-27 07:58:41 +00:00

You use Pandas lib to parse CSV file. It adds extra dependency. While there is default Python 3 library csv.

You use Pandas lib to parse CSV file. It adds extra dependency. While there is default Python 3 library [csv](https://docs.python.org/3.8/library/csv.html).

tools/python/transit/gtfs/download_gtfs.py

					
				@ -12,3 +13,4 @@

				import csv

				import time

				import zipfile

strump commented

2022-05-27 07:55:50 +00:00

Why do you use short URL instead of a full URL https://storage.googleapis.com/storage/v1/b/mdb-csv/o/sources.csv?alt=media ?

Why do you use short URL instead of a full URL `https://storage.googleapis.com/storage/v1/b/mdb-csv/o/sources.csv?alt=media` ?

biodranik commented

2022-05-27 08:28:08 +00:00

(Migrated from github.com)

@pastk can you please also take a look, as a python-expert? :)

fardeenfs (Migrated from github.com) reviewed 2022-05-27 18:21:20 +00:00

tools/python/transit/gtfs/download_gtfs.py

					
				@ -12,3 +13,4 @@

				import csv

				import time

				import zipfile

fardeenfs (Migrated from github.com) commented

2022-05-27 18:21:20 +00:00

The link to the CSV file on their website used a link shortener.

The link target could potentially change when they go from v1 to say v2. The link shortener's target URL will probably be updated and feels like a better option. I can change it if there is an issue with it.

The link to the CSV file on their website used a link shortener. The link target could potentially change when they go from v1 to say v2. The link shortener's target URL will probably be updated and feels like a better option. I can change it if there is an issue with it.

fardeenfs (Migrated from github.com) reviewed 2022-05-27 18:37:44 +00:00

tools/python/transit/gtfs/download_gtfs.py

					
				@ -4,3 +4,4 @@

				                            - Mobility Database (https://mobilitydata.org/)  

				Crawls all the urls, loads feed zips and extracts to the specified directory."""

				import argparse

fardeenfs (Migrated from github.com) commented

2022-05-27 18:37:44 +00:00

Pandas is currently being used by another file in the project (/search/search_quality/scoring_model.py), so I was of the opinion that the module will already be installed to run the project, i.e. not an extra dependency.
I will look into a csv module alternative.

Pandas is currently being used by another file in the project (/search/search_quality/scoring_model.py), so I was of the opinion that the module will already be installed to run the project, i.e. not an extra dependency. I will look into a csv module alternative.

biodranik (Migrated from github.com) reviewed 2022-05-28 10:23:51 +00:00

tools/python/transit/gtfs/download_gtfs.py

					
				@ -4,3 +4,4 @@

				                            - Mobility Database (https://mobilitydata.org/)  

				Crawls all the urls, loads feed zips and extracts to the specified directory."""

				import argparse

biodranik (Migrated from github.com) commented

2022-05-28 10:23:51 +00:00

Our general rule: fewer dependencies are better than more. We introduce new 3party dependencies ONLY if there is no other comparable way to solve the problem.

biodranik (Migrated from github.com) reviewed 2022-05-28 10:37:54 +00:00

tools/python/transit/gtfs/download_gtfs.py

					
				@ -12,3 +13,4 @@

				import csv

				import time

				import zipfile

biodranik (Migrated from github.com) commented

2022-05-28 10:27:08 +00:00

It creates an additional HTTP round-trip to resolve the shortener. I would document in the comment the original web page where you get that shortener, and use the resolved URL. If the scheme changes, we'll need to update the parser too.

biodranik (Migrated from github.com) commented

2022-05-28 10:28:22 +00:00

    file = open(RAW_FILE_MOBILITYDB, encoding="UTF-8")

Please use consistent code formatting everywhere.

```suggestion file = open(RAW_FILE_MOBILITYDB, encoding="UTF-8") ``` Please use consistent code formatting everywhere.

biodranik (Migrated from github.com) commented

2022-05-28 10:31:03 +00:00

Will it close the file properly if write() above throws an exception?
After running this function, there will be a file somewhere in the file system. Is that place predictable and always the same? When should this file be deleted? Or it shouldn't? Does it make sense to reuse it if the HTTP connection fails?

1. Will it close the file properly if write() above throws an exception? 2. After running this function, there will be a file somewhere in the file system. Is that place predictable and always the same? When should this file be deleted? Or it shouldn't? Does it make sense to reuse it if the HTTP connection fails?

fardeenfs (Migrated from github.com) reviewed 2022-05-28 19:45:33 +00:00

tools/python/transit/gtfs/download_gtfs.py

					
				@ -12,3 +13,4 @@

				import csv

				import time

				import zipfile

fardeenfs (Migrated from github.com) commented

2022-05-28 19:45:33 +00:00

I have improved it. It should close now even if the write() fails.
The file can be reused even if the HTTP connection fails. The file is stored in the same location where the existing Transit Land URLs file is stored. It can be deleted but if it exists it will overwrite its content, so it will not cause any issues either way.

1. I have improved it. It should close now even if the write() fails. 2. The file can be reused even if the HTTP connection fails. The file is stored in the same location where the existing Transit Land URLs file is stored. It can be deleted but if it exists it will overwrite its content, so it will not cause any issues either way.

fardeenfs (Migrated from github.com) reviewed 2022-05-29 06:21:11 +00:00

tools/python/transit/gtfs/download_gtfs.py

					
				@ -4,3 +4,4 @@

				                            - Mobility Database (https://mobilitydata.org/)  

				Crawls all the urls, loads feed zips and extracts to the specified directory."""

				import argparse

fardeenfs (Migrated from github.com) commented

2022-05-29 06:21:10 +00:00

I have changed the code to work with the default csv module :)

biodranik (Migrated from github.com) approved these changes 2022-05-29 21:23:19 +00:00

tools/python/transit/gtfs/download_gtfs.py

biodranik (Migrated from github.com) commented

2022-05-29 21:15:32 +00:00

There could be temporary network errors, so I would avoid recommending URL update.

biodranik (Migrated from github.com) commented

2022-05-29 21:15:55 +00:00

    file = open(RAW_FILE_MOBILITYDB, encoding='UTF-8')

One interesting practice is to use single quotes for verbatim strings, and double quotes for strings with variables inside. It helps to read the code.

```suggestion file = open(RAW_FILE_MOBILITYDB, encoding='UTF-8') ``` One interesting practice is to use single quotes for verbatim strings, and double quotes for strings with variables inside. It helps to read the code.

biodranik (Migrated from github.com) commented

2022-05-29 21:23:12 +00:00

Another possible option is to always use double quotes like it's done already in the file (except in 'wb' above).

biodranik (Migrated from github.com) reviewed 2022-05-30 06:15:35 +00:00

tools/python/transit/gtfs/download_gtfs.py

biodranik (Migrated from github.com) commented

2022-05-30 06:15:35 +00:00

Why it was required before? What happens now if it is specified or not specified?

fardeenfs (Migrated from github.com) reviewed 2022-05-30 06:46:21 +00:00

tools/python/transit/gtfs/download_gtfs.py

fardeenfs (Migrated from github.com) commented

2022-05-30 06:46:21 +00:00

As per the earlier implementation, Transit Land API key was always necessary. This gives the flexibility to just crawl Mobility without the hassle of generating a Transit Land API key.

If the --source is set to 'all' or 'transitland' : the key is required. If it is not specified, line 255 will return an error.

If the --source is set to 'mobilitydb' : the key is not required and it will crawl just Mobility DB. Specifying the Transit Land key wouldn't matter since it is not being used.

As per the earlier implementation, Transit Land API key was always necessary. This gives the flexibility to just crawl Mobility without the hassle of generating a Transit Land API key. If the --source is set to 'all' or 'transitland' : the key is required. If it is not specified, line 255 will return an error. If the --source is set to 'mobilitydb' : the key is not required and it will crawl just Mobility DB. Specifying the Transit Land key wouldn't matter since it is not being used.