Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Draft] Changes needed to add TTC #577

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

[Draft] Changes needed to add TTC #577

wants to merge 12 commits into from

Conversation

EddyIonescu
Copy link
Member

@EddyIonescu EddyIonescu commented Feb 22, 2020

Makes progress on adding TTC. Improves error messages for adding future agencies.

Proposed changes

  • Replacing the default TRYNAPI_URL is required for running compute_new.py locally; however the error thrown when it is not doesn't suggest this. This PR catches the trynapi request error in trynapi.py and prints an error message that says it could not connect to TrynAPI and asks whether the environment variable was provided and exits. The stacktrace resulting from raising there is very long and not very useful, so it seems reasonable to simply exit.

  • Tolerance for broken stop geometry is increased from 100 to 300 metres, as to account for route terminals that are moved without the geometry being updated. In the image below the stop was moved farther back as the airport decided that Uber/Lyft needed more space near the arrivals hall.
    IMG_3617

  • Skipping over the route's geometry resulted in the dashboard being all grey as the initial metrics API call failed due to an exception thrown in the average speed calculation as the geometry for a stop was missing. Now save_routes.py raises an exception if the stop geometry is bad, which has not caused issues for Muni, Trimet, or TTC.

  • Continue working on the config for TTC, this PR adds one route and changes the start time for the overnight bus routes.

  • Implement Add direction groups for routes in agency routeConfig #574, which adds custom default directions, allowing east-west routes to be specified in less lines than custom_directions would require.

...

Link to demo, if any

https://eddy-opentransit.herokuapp.com
(will be pre-computed when #423 is resolved)
...

except Exception as exc:
print(exc)
print('Ensure tryn-api is running and that you set the TRYNAPI_URL environment variable')
exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Library functions like this should raise exceptions when errors occur, so that the caller can handle them as needed. Calls to exit() should be in the top-level scripts like compute_new.py . If the top-level script only should exit in certain cases, you could raise a particular class of exception here and catch that type in the top-level script.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, normally it shouldn't be necessary to set TRYNAPI_URL, since docker-compose.yml sets TRYNAPI_URL to http://tryn-api.opentransit.city . This also applies when running scripts from the command line via docker-shell.sh or docker-shell.bat.

continue
if stop_geometry['offset'] > 300:
# Throw as skipping it will result in speed metrics API calls failing
raise Exception(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should raise an exception -- if the offset is ever greater than the threshold, it's not clear what to do to fix it, other than by continuing to increase the threshold until the exception stops being raised. At that point, what is the purpose of having this check at all?

I think it would be better to keep the original behavior and then fix the exception that is being thrown by GraphQL API when stop geometry is not available.

backend/agencies/ttc.yaml Outdated Show resolved Hide resolved
@hathix hathix mentioned this pull request Jun 18, 2020
@EddyIonescu EddyIonescu marked this pull request as draft July 9, 2020 02:45
@EddyIonescu EddyIonescu changed the title Changes needed to add TTC [Draft] Changes needed to add TTC Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants