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

Align provider and agency APIs #271

Closed
lionel-panhaleux opened this issue Mar 26, 2019 · 28 comments · Fixed by #506
Closed

Align provider and agency APIs #271

lionel-panhaleux opened this issue Mar 26, 2019 · 28 comments · Fixed by #506
Assignees
Labels
Agency Specific to the Agency API enhancement New feature or request Policy Specific to the Policy API Provider Specific to the Provider API State Machine Changes in the vehicle state events and state machine diagram
Milestone

Comments

@lionel-panhaleux
Copy link
Contributor

Hello,

The Provider and Agency APIs list different event_type and event_type_reason values.
This is cumbersome (and error-prone) on the provider side and the agency side alike. We could maintain a single list of event_type and share it between the two APIs ? I could submit a PR if you are interested.

AFAICT the agency side has a more elaborate list of events, so it makes sense to use this list as a first version. However this would be a breaking change for the Provider API. Another approach would be to begin with a non breaking list including all events from the Provider and Agency APIs, then indicate some values to be deprecated in the next major version.

What do you think ?

@marie-x
Copy link
Collaborator

marie-x commented Mar 27, 2019

@lionel-panhaleux Agreed that they should be as similar as possible. Agency has more requirements than Provider, which is the source of the differences. While we ("we" meaning all of us with a stake in MDS) learn how the expanded list of status-changes plays out in the real-world, E&A decided it would be (in some ways) more expedient to be a superset of Provider rather than forcing Provider to be exactly the same.

@marie-x
Copy link
Collaborator

marie-x commented Mar 27, 2019

I would say that we should keep this issue warm until, say, mid-May, and revisit. That will give us a solid month of production experience with the permitted providers. Would like to hear from other stakeholders e.g. @hunterowens, @thekaveman, etc.

@rf-
Copy link
Contributor

rf- commented Mar 28, 2019

It might make sense to treat this as two separate requests:

  1. Harmonize the names and structure of the events where they currently overlap
  2. Expand the list of status changes in Provider to cover some or all of the extra events from Agency

It makes sense to take a wait-and-see approach with the second one, but it doesn't seem like there's much reason for something like a user dropoff event or a maintenance pickup to be represented differently in Agency vs. Provider.

@marie-x
Copy link
Collaborator

marie-x commented Mar 28, 2019

@rf- smart

@hunterowens hunterowens added the State Machine Changes in the vehicle state events and state machine diagram label Apr 9, 2019
@hunterowens hunterowens added this to the 0.4.0 milestone Jun 27, 2019
@yoimnik
Copy link
Contributor

yoimnik commented Aug 29, 2019

hey so what's the traction on this? we should get this added in provider asap

specifically, the problem with the nomenclature of event type "reserved" for provider being different from the event type "reserved" in agency

also, agency's "service_end" puts the bird in an unavailable state, which is in the PROW, while provider's "service_end" is an event_type_reason for making it in a removed state, which is off the PROW

we need to align these. and we need to get provider unavailable off_hours events and service_start on_hours events (or whatever more specific naming)

what do you guys think?

@marie-x
Copy link
Collaborator

marie-x commented Aug 30, 2019

There's definitely traction. It's on me and @hunterowens (mostly me) to make the canonical list of discrepancies and start knocking them down. Once we have the canonical list we can go one-by-one and adjudicate which approach to take (e.g. change provider, change agency, change both, agree to leave different until a later rev).

Should we merge #271 and #345?

@yoimnik
Copy link
Contributor

yoimnik commented Aug 30, 2019

@Karcass yeah they're essentially the same threads

can we see a roadmap of what's being decided so that we can better plan for these when designing more robust engineering systems to handle these requirements?

@marie-x
Copy link
Collaborator

marie-x commented Aug 30, 2019

@yoimnik The roadmap for Provider, for Agency, or all of MDS? Much of the roadmap is provisional, awaiting the contribution to OMF, and having the OMF member cities lay out their priorities.

@hunterowens hunterowens modified the milestones: 0.4.0 , 0.4.1 Oct 16, 2019
@sarob sarob added Agency Specific to the Agency API enhancement New feature or request Policy Specific to the Policy API Provider Specific to the Provider API labels Dec 19, 2019
@antoinewg
Copy link
Contributor

I am happy to follow up on this, list the incoherences + overlaps

@HenriJ HenriJ changed the title Align provider and agency event_type and event_type_reason Align provider and agency APIs Jan 3, 2020
@marie-x
Copy link
Collaborator

marie-x commented Jan 3, 2020

@TonioGarcia07 I have a list as well. I can add you to the Google doc, or we can port it to a wiki, or something. What is your preference?

@antoinewg
Copy link
Contributor

@Karcass I've created a doc as well (I can allow to edit if someone need). Please tell me if you find any discrepancies with yours. See below:

Here are the current discrepancies between the Agency and Provider "events".
Note: an agency event is uniquely defined by its event_type and event_type_reason whilst the Provider event is uniquely defined by its event_type_reason (its event_type can be derived from its event_type_reason).

Given that the definitions of the Agency status corresponds to the Provider event_type, I performed a "full join" between the Agency & Provider events (thus the gaps) in the table below (ordered by status).
Note: "full join" meaning a mapping between the Agency events and Provider events given that their respective status on success or event_type should be equal.

Here's a link to the Google sheet.

row Agency event_type_reason Agency event_type Agency status Provider event_type Provider event_type_reason Remarks (and proposed mapping if applicable)
1 service_start available available service_start
2 provider_drop_off available Two possible events: rebalance_drop_off & maintenance_drop_off. We use (rebalance_drop_off, available)
3 available rebalance_drop_off No direct corresponding event in agency. We use (provider_drop_off,)
4 available maintenance_drop_off No direct corresponding event in agency. We use (provider_drop_off, maintenance)
5 available agency_drop_off No corresponding event in agency. We use (agency_drop_off,)
6 cancel_reservation available No corresponding event in provider. We use (user_drop_off, available)
7 trip_end available available user_drop_off
8 trip_leave elsewhere No corresponding event in provider. We use (service_end, removed)
9 deregister inactive No corresponding event in provider. We use (service_end, removed)
10 missing deregister inactive No corresponding event in provider. We use (service_end, removed)
11 decommissioned deregister inactive No corresponding event in provider. We use (service_end, removed)
12 register removed No corresponding event in provider. We use (service_end, removed)
13 provider_pick_up removed removed service_end Conflict in nomenclature with agency's service_end
14 rebalance provider_pick_up removed removed rebalance_pick_up
15 maintenance provider_pick_up removed removed maintenance_pick_up
16 charge provider_pick_up removed No corresponding event in provider. We use (maintenance_pick_up, removed)
17 compliance provider_pick_up removed No corresponding event in provider. We use (service_end, removed)
18 removed agency_pick_up No corresponding event in agency. We use (agency_pick_up,)
19 city_pick_up removed No corresponding event in provider. We use (agency_pick_up, removed)
20 reserve reserved reserved user_pick_up Why "user_pick_up" and "reserve" ?
21 trip_start trip No corresponding event in provider. We use (user_pick_up, reserved)
22 trip_enter trip No corresponding event in provider. We use (user_pick_up, reserved)
23 service_end unavailable No corresponding event in provider. We use (maintenance, unavailable)
24 low_battery service_end unavailable unavailable low_battery
25 maintenance service_end unavailable unavailable maintenance
26 compliance service_end unavailable No corresponding event in provider. We use (maintenance, unavailable)
27 off_hours service_end unavailable No corresponding event in provider. We use (maintenance, unavailable)

Noticeable Problems

If we go past the fact that:

  1. There is a mismatch in the namings: an Agency status corresponds to a Provider event_type, an Agency event_type corresponds to a Provider event_type_reason and an Agency event_type_reason has no Provider equivalent.

We notice that:

  1. There are gaps:
  • Agency events with no Provider equivalent: rows 2, 6, 8, 9, 10, 11, 12, 16, 17, 19, 21, 22, 23, 26 & 27
  • Provider events with no Agency equivalent: rows 3, 4, 5 & 18.
  1. Although most matches are coherent, the Provider service_end clashes with the definition of the Agency service_end (11 vs. row 23 to 27)

  2. There are more Agency statuses (7) than Provider event_type (4).

  3. Agency's reserve corresponds to Provider's user_pick_up (row 20).

Thoughts

For each of the points above:

  1. Rename Provider event_type => status and Provider event_type_reason to event_type although that would potentially be a breaking change.

  2. We could add events or choose the most sensible event to map to.

  • Example: For row 6: map Agency (cancel_reservation,) to user_drop_off since both events result in available.
  • Example: For row 4: add (provider_drop_off, maintenance) in Agency events to match maintenance_drop_off from Provider API.
  1. Provider's service_end should be renamed provider_pick_up for clarity although that could also be breaking.

  2. Should not be problematic as long as we have a one-to-one mapping between the Agency and Provider "events".

  3. Not problematic, just misleading.

@Retzoh
Copy link
Contributor

Retzoh commented Jan 30, 2020

#421 could propose a reconciliation as far as trip and reservations are concerned.

@thekaveman
Copy link
Collaborator

@thekidder
Copy link
Contributor

thekidder commented Feb 5, 2020

I put some thought into this in preparation for the meeting tomorrow. It's important to fully address what @TonioGarcia07 pointed out: there's a mismatch in naming and in specificity. Any fix should clarify exactly what level of detail each field represents, and be aligned across agency/provider

I put together a proposal that attempts to do this, as well as fixes and clarifies some other event edge cases that I see from my perspective as an implementer. Note that this is a very large proposal; it is definitely breaking, it may be too large to accept in whole, but hopefully some of the ideas are helpful for the discussion.

Proposal:

  1. Change the term event_type to vehicle_state, and event_type_reason to event_types. This clarifies the level of detail; vehicle_state is the what: it encompasses the state of the vehicle from a compliance perspective (parked & rentable, on-trip, parked & unrentable, etc). The why is captured by event_types. In addition, event_types is now an array: multiple pieces of metadata can be attached to a single event. I use the in-progress SAE micromobility definitions verbatim for vehicle_state.
  2. Only allow a single event per timestamp. Making event_types an array allows for multiple pieces of metadata in a single state change, so there's no benefit in having multiple events per timestamp. In addition, we can now enforce that a vehicle can only be in a single state at a particular point in time. As an example, the event sequence (on_trip; [trip_start]) followed by (non_operable; [trip_end, low_battery]) is legal, and represents a vehicle that has ended a trip, and is no longer rentable because the battery is low.
  3. Allow for an event between two identical vehicle_states, but with differing event_types. As an example, the sequence (on_trip; [trip_start]) followed by (on_trip; [leave_municipal_boundary]) represents the current agency trip_leave event.
  4. Remove status from agency. I believe now that vehicle_state is a first-class citizen, this is mostly redundant. This lets us reduce the complexity in the spec.

I've also adjusted the naming of various events to be more consistent throughout agency/provider. And started to address a few other related issues such as #421.

I wrote up the full proposed mapping and definitions in this spreadsheet: https://docs.google.com/spreadsheets/d/1rs6pGI3LudXIFQfZLkqE5sJ4A6_WfM1439hG4IK6B0Y/edit#gid=0.

@Retzoh
Copy link
Contributor

Retzoh commented Feb 5, 2020

Hi Adam, thank you very much ! This looks great, I am looking forward to discuss this tomorrow.

@thekaveman
Copy link
Collaborator

@thekidder thank you for working on this and putting together your highlights above and the spreadsheet, very helpful!

Speaking to the Provider side specifically:

As an example, the event sequence (on_trip; [trip_start]) followed by (non_operable; [trip_end, low_battery]) is legal, and represents a vehicle that has ended a trip, and is no longer rentable because the battery is low.

If I am understanding this example correctly and based on the spreadsheet, it sounds like the proposal would remove/flatten the hierarchical relationship between event_type (proposed: vehicle_state) and event_type_reason (proposed: event_types). Both notions are elevated to be somewhat independent of one another, in the sense that a vehicle_state no longer prescribes the exact list of event_types that are allowed for that state. Is that accurate?

@thekidder
Copy link
Contributor

@thekaveman Yes, that's exactly correct. There's some more work to do here; I still think it's useful to formalize which vehicle_state, event_types combinations are valid. For example, it doesn't make any sense to have an event of (on_trip, trip_end). But my thought is that we can minimize the complexity a little bit by considering vehicle_states and event_types separately instead of only viewing them as a hierarchy.

I think this particular piece of the proposal can be discussed independently: the new states and definitions could be implemented in the existing, hierarchal world. But as long as we're re-working events, I think it's useful to look for other opportunities for simplicity.

@jiffyclub
Copy link
Contributor

jiffyclub commented Feb 5, 2020

We here at Populus were just talking about how the end_trip event type is only associated with the available status, which doesn't sync well with the case when a vehicle becomes unavailable mid-trip due to a low battery or operating hours. We as users of MDS data would like to be able to, for any given point in time, look at only the most recent event for a vehicle in order to determine that vehicle's status, so being able to pair (non_operational, trip_end) could help providers both report the ends of trips and correctly indicate that the vehicle is not returning to a rentable state.

@Retzoh
Copy link
Contributor

Retzoh commented Feb 6, 2020

This would also be a way to resolve #274

@thekidder
Copy link
Contributor

I've started to formalize my proposal and have updated the Provider README with the new event types and definitions. You can take a look here: https://github.com/thekidder/mobility-data-specification/tree/align-provider-and-agency/provider.

I haven't started working on the agency README yet, nor any JSON schema changes.

@jfh01
Copy link
Contributor

jfh01 commented Feb 18, 2020

We've set up a mailing list for ongoing coordination around this issue, including regular meetings:

https://groups.google.com/a/groups.openmobilityfoundation.org/forum/#!forum/mds-reconciliation

Please subscribe if you're interested in the outcome of this work.

@margodawes
Copy link

@Karcass and @thekidder - I have two new suggested event_type_reasons to add from SDOT and I'm not sure if this is the best place to bring them up:

  • Add an event_type_reason “user_parked” to the event_type “reserved” to capture car share trips that are stopovers (e.g., picking up groceries) and facilitate parking revenue collection
  • Add an event_type_reason for device charging (e.g., to the event_type “unavailable”)

Should I submit separate issues or pull requests for these?

@marie-x
Copy link
Collaborator

marie-x commented Apr 10, 2020

HI @margodawes,

You will want to fold these requests into the reconciliation proposal described in these documents:

@billdirks
Copy link
Contributor

I have a question about the state diagram. The state diagram shows the only ways to exit the trip state is to the available or elsewhere state. What should the appropriate transition be if a trip ends and the vehicle doesn't go elsewhere but it also doesn't become available again? Examples could include:

  1. A trips ends with a vehicle in a low battery state and it never becomes available again.
  2. A trip ends because the vehicle crashes or is somehow damaged and does not become available.

@Retzoh
Copy link
Contributor

Retzoh commented May 6, 2020

Hi @billdirks, from what I understand, the reconciliation outcome would enable you to solve this issue by sending the following event:
{vehicle_state: unavailable, event_type: [trip_end, non_operational]} (or unavailable instead of non_operational, depending on the outcome of the naming discussion). I guess there should be a trip_id corresponding to the trip_end too. This is however not reflected in the state diagram.
See notes section about multiple event_types and note section about renaming status and event_type

@Karcass is that correct?

@marie-x
Copy link
Collaborator

marie-x commented May 6, 2020

@Retzoh @billdirks yes, that's correct. (I think event_types will probably be plural in the PR.)

@billdirks
Copy link
Contributor

@Retzoh @Karcass Thanks for the answer and highlighting that section of the notes document! I'll give the notes document a read.

@schnuerle
Copy link
Member

schnuerle commented Jun 26, 2020

Resolved now in #506!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agency Specific to the Agency API enhancement New feature or request Policy Specific to the Policy API Provider Specific to the Provider API State Machine Changes in the vehicle state events and state machine diagram
Projects
None yet
Development

Successfully merging a pull request may close this issue.