GTFS flex support: flag stops, deviated-route service, call-and-ride

[sdjacobs]

As part of the US DOT's Mobility on Demand Sandbox Grant, we added support for flexible transit to OpenTripPlanner, based on the emerging GTFS-flex spec. This work was done on behalf of VTrans, the Vermont Department of Transportation.

It includes:

• flag stops, in which a passenger can flag down the bus along its route or get dropped off in between stops
• deviated-route service, in which the bus can deviate from its route within an area or radius to do a dropoff or pickup
• call-and-ride, which is an entirely deviated segment.

The GTFS flex specification is available here.

The overall implementation is to run a "graph modifier" before the main graph search which does some preliminary graph searches around the origin and destination in order to create temporary edges and vertices to represent the flex options, which are disposed of after the request.

The VTrans project required a few other changes to be made to OTP which are not included in this PR:

• Allow itinerary in response to be sorted by duration #2593, allowing itineraries to be sorted by duration
• serviceDayLookout: add parameter to look more days out in future (or past, for arriveBy=true) #2592, including more service days in the transit search. (Many of the rural services in Vermont are extremely rare, e.g. a frequency of every other Wednesday, and we wanted those services to be easily discoverable by users.)

This is currently usable via VTrans's website here: https://plan.govermont.org/

[sdjacobs]

I merged the Temporary Vertex cleanup code, extended it to work with GTFS-Flex, and added checks to ensure that only TemporaryEdges created by a particular request are used.

The approach is:

• all temporary vertices get added to RoutingContext.temporaryVertices. For temporary vertices at the endpoints (e.g. in all normal, non-flex searches), the temporary vertices get added via a search RoutingContext.init. I adapted TemporaryVertexDestroy to be able to simply search for the subgraphs, not destroy them.
• Temporary vertices for GTFS-Flex get directly added to the collection at their creation time.
• disallow creation of a state if it is to a temporary vertex which is not contained in RoutingContext.temporaryVertices
• All temporary vertices in the collection are destroyed in RoutingContext.destroy(). For code reuse and separation-of-concerns reasons, the edge removal still happens in TemporaryVertexDestroy, even though the extra search for subgraphs isn't strictly necessary because the search already happens at the beginning.

@t2gran I'd appreciate your thoughts on this since it changes your fix in #2655 a bit!

I think this is that last part of the PR that will require more code changes if there are concerns. Aside from this, I've got more documentation to do.

[sdjacobs]

I added more documentation, including a new overview page for GTFS-Flex. Open for comments! @thomastrillium, if you want to take a look: https://github.com/opentripplanner/OpenTripPlanner/blob/a275257a6afc8cfc3fd2f6d7bd8cafb0f1633b79/docs/Flex.md

@abyrd, you've asked a few times about the distinction between "flexible" service and "demand-response transit" (DRT). "Flexible" transit is anything that's defined in the GTFS-Flex spec. I think DRT is specifically off-route flexible transit, e.g. call-and-ride and deviated-route. But in this PR, the word "DRT" only appears when it's part of a term that comes from the spec. For example, flexIgnoreDrtAdvanceBookMin is a new routing parameter that allows graph searches to ignore options that would otherwise be excluded by the drt_advance_book_min calculation, drtAvgTravelTime and drtMaxTravelTime are new members of TripTimes that are created from drt_avg_travel_time and drt_max_travel_time in the spec, and so forth. I was trying to use terminology that's consistent with the spec. I suppose there's a balance between consistency with the spec and unnecessary new terminology -- happy to change names around if that would be helpful.

[abyrd]

Thanks @sdjacobs for all the additional Javadoc and user documentation. Looks good. I now understand that the terms Flex and DRT are coming from the Flex spec, and it makes sense to follow the conventions and terms defined in that spec. We may need to add more definitions to the Flex spec.

I'm a little hesitant about the combined temporary vertex/edge removal approach - if we're explicitly recording all the created temporary objects so we can remove them one by one, it seems strange to use a graph search to find them instead of just registering them when they're created. But I recognize that we can't change / fix everything at once. @t2gran if you approve of this combined approach we can leave any simplification for a subsequent refactor.

I'm also hesitant about the method of avoiding temporary objects in threads other than the one that created them. We originally used a similar approach for avoiding temporary origin/destination edges in threads that shouldn't see them, but eliminated that approach it in favor of just making it topologically impossible for those temporary edges to be on successful paths in other threads (e.g. starting vertices only have edges leading out of them, so no search can enter them). It gives me pause to see active tracking and avoidance of temporary edges return to OTP because it introduces another type-checking conditional into every edge traversal (which may have performance implications as an inner loop "hot spot"), and because there are now more chances for the creation and removal of these temporary objects to be interleaved in unexpected ways with searches attempting to access them, e.g. another thread can be trying to access the incoming edges of a temporary vertex at exactly the moment those edges are removed. Critically, it might be able to access them as part of non-dead-end paths that actually lead toward the destination. The use of copy-on-write edge lists should continue to minimize these problems, but I'm not as convinced as with the topological avoidance approach. Also, if we're returning to active tracking and avoidance we should question why we are applying two different approaches, one to endpoint temporary objects and one to Flex temporary objects. If we are reverting to an older method of tracking (and removal) for Flex temporary objects, should we continue simultaneously using different methods for origin/destination temporary objects? Unfortunately these effects are not fully confined to Flex searches: a new conditional is hit on every traversal of an edge in every search.

I discussed the current state of this whole PR with @t2gran a few days ago. We agreed that the PR plus all the changes are getting large enough that it's increasingly hard to have a full understanding of what we're merging. Most or all of the review comments have been addressed though, so the practical way forward is to merge this PR and then revise later if we spot any more things that need attention.

We also discussed two main concerns about this PR, the depth of which only became apparent after working on it for a while:

• The GTFS-Flex spec that it implements seems to be in a draft stage and will probably require extensive changes before it can be accepted as an official extension to GTFS. The OTP Flex support will then need to be reworked to match the final form of the spec. The full process of finalizing the spec and revising the software will require a significant amount of additional effort.
• While effort has been made to modularize the new functionality in the sense that it is disabled by default and is designed not to affect OTP output unless explicitly enabled ("feature flags"), a significant amount of code used only by the flex implementation is mixed directly into existing code used by all deployments of OTP, which may complicate readers' understanding of the core routing functionality.

All that said, we do want to get this merged without dragging it out longer, and incremental change can happen after the merge if needed. This is destined for the dev-1.x branch, and 2.x transit routing will be different enough that we'll eventually have to re-implement flex functionality, at which point we can learn from what's already been done here, and benefit from a GTFS-Flex spec in a more advanced state.

[abyrd]

I just adjusted the PR base to dev-1.x which should not change the content, because that's a recent branch off master. @gmellemstrand the final PR approval is up to you, please try to integrate any input from @t2gran about the suitability of the final temp object cleanup approach.

[sdjacobs]

Thanks Andrew. I share your concerns, but for the most part I agree that the path forward is to merge and address issues as they come up.

For temporary edges, the changes I made are to track and remove ALL TemporaryEdges and to check they if they were created by the proper request on every edge traversal. A more conservative approach would be to keep the existing (solely topological, no tracking) behavior for all non-flex edges and only track-check-remove for flex TemporaryEdges. (The fundamental problem is that flex TemporaryEdges, as implemented, are not topologically impossible for a separate request to access, and they are not discoverable by the dispose method in master). These changes would involve:

• rename request.temporaryEdges to request.flexTemporaryEdges
• avoid adding regular endpoint TemporaryEdges to that collection, and revert to the graph-search disposal.
• Dispose flexTemporaryEdges as it's done currently in the PR.
• move the edge-is-in-request check from StateEditor to all the GTFS-Flex implementations of TemporaryEdge

The implementation above is more verbose than the current state of the PR, and it moves back toward treating flex TemporaryEdges and non-flex TemporaryEdges differently. But, it keeps existing behavior the same and avoids the possible inner loop hot spot.

Would folks prefer the above? I believe I have some similar code in a branch from when I was experimenting with this, so it would not be much trouble to switch.

In any case, once we determine what the solution in the PR is, I'll create an issue to come back to the TemporaryEdge problem and determine a better long-term solution.

[abyrd]

The implementation above is more verbose than the current state of the PR, and it moves back toward treating flex TemporaryEdges and non-flex TemporaryEdges differently. But, it keeps existing behavior the same and avoids the possible inner loop hot spot.

There is another much simpler option with the same resource-consumption characteristics as the current system in this PR: we could just not add temporary edges and vertices to the graph at all. The method that fetches the outgoing or incoming edges of a vertex would then check the main graph (shared between all threads) as well as a map in the request context (known only to the current thread) to find additional temporary edges. This has been done in the past in OTP and it works, it was just eliminated as part of other changes over the years because on the surface it looks inefficient. But this inefficiency is just one additional hash lookup per Dijkstra iteration, on a very small table that is likely to remain in processor cache, which is the same additional work created by the current approach in this PR.

With the solution I describe above, we don't need to track and delete temporary objects at all because only request-scoped objects have references to them, and they are invisible to the shared graph. They are just garbage collected when the request is finished.

It's a significant change but in many ways much less fragile and complicated. Instead of inserting objects into a complicated data structure shared across threads and detecting when to ignore them, we just don't add them to the shared data structure, and detect when to make them visible.

Ideally we would prototype and profile this before accepting it, to determine whether it causes significant slowdown. Although if we need to profile my proposed approach, we should also profile the one currently in the PR because they add roughly the same amount of extra computation.

[abyrd]

In any case, once we determine what the solution in the PR is, I'll create an issue to come back to the TemporaryEdge problem and determine a better long-term solution.

I'm hesitant to make such a huge change as I just described as part of this PR. So I'd prefer to merge this one, then improve the temporary edge handling in a subsequent step.

A problem I foresee though is that if we make the big temporary edge handling change, I'd like it to apply to both 1.x and 2.x, and after merging this PR, 1.x and 2.x will have diverged significantly.

[t2gran]

@abyrd I agree to let this PR pass and fix the temporary vertex problem in its own Issue/PR. I don't think it will be that difficult to merge such PR into 2.x (even with a lot of changes).

I will mark my latest review as resovled, since it is on stuff that will be deleted or refactored anyway. I just want to keep the comments in case we decide to keep some of it.

[drewda]

All: sounds like there's sufficient agreement to merge now. Thanks to everyone for sticking with this review and for Simon for making many rounds of improvements.

@sdjacobs can you resolve the current merge conflicts? Afterwards go ahead and merge, if you have permissions, or if not, let me know and I can do it.

[abyrd]

Actually @sdjacobs and @drewda please do not merge this PR immediately. I am intentionally finalizing and merging a round of small PRs into dev-1.x which I will then merge into dev-2.x, before merging these big changes (Flex, ride hailing) into dev-1.x. Waiting to merge these big 1.x-only PRs until after that round of smaller PRs is merged will save us a lot of cherry-picking commits and potential merge conflicts.

I'm not talking about a long wait, I imagine we should be able to merge this PR by next week. You may want to wait until all those smaller changes are merged into 1.x before resolving merge conflicts between 1.x and this branch - though I don't expect there to be many new conflicts.

[drewda]

@abyrd O.K. Can you put together a checklist of the "small PRs into dev-1.x" and tell us a day next week when you want to aim to have it complete? (Your plans on dev-1.x and dev-2.x make sense -- I just want to make sure we don't let this and other PRs go stale in the process.)

[abyrd]

Yes @drewda, good idea to make a checklist. I think I've got everything merged except for the following two:

• Remove R5 and gtfs-lib dependencies, using kryo-tools dependency #2700 (Remove R5 and gtfs-lib dependencies)
• Upgrade Geotools, steps toward targeting JDK 11 #2714 (Upgrade Geotools, allowing compilation under JDK11)

In fact #2714 builds upon #2700, so if someone can test routing results from #2714 that should allow approval of both PRs. Someone should be able to check out branch java-11-geotools and run any standard batch of OTP routing tests - these changes are expected to be invisible from outside OTP.

[drewda]

@abyrd great, thanks for the (short) list. Let's aim to ideally resolve those blockers by the end of next week.

[abyrd]

I merged #2700 and #2714 today then caught dev-2.x up to dev-1.x, and then merged this one into dev-1.x with a relatively simple patch to adapt it to the new Locationtech JTS dependency. Thanks @sdjacobs @t2gran and @drewda for working through all this.