`Punctuality` key missing (sometimes)

[YarmoM]

When running the nederlandse_spoorwegen integration on the home-assistant platform, I got the following error message:

Traceback (most recent call last):
  File "/home/yarmo/gh/home-assistant/homeassistant/helpers/entity_platform.py", line 299, in _async_add_entity
    await entity.async_device_update(warning=False)
  File "/home/yarmo/gh/home-assistant/homeassistant/helpers/entity.py", line 461, in async_device_update
    await self.hass.async_add_executor_job(self.update)
  File "/usr/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/home/yarmo/gh/home-assistant/homeassistant/util/__init__.py", line 240, in wrapper
    result = method(*args, **kwargs)
  File "/home/yarmo/gh/home-assistant/homeassistant/components/nederlandse_spoorwegen/sensor.py", line 208, in update
    2,
  File "/home/yarmo/gh/home-assistant/venv/lib/python3.7/site-packages/ns_api.py", line 932, in get_trips
    return self.parse_trips(raw_trips, requested_time)
  File "/home/yarmo/gh/home-assistant/venv/lib/python3.7/site-packages/ns_api.py", line 843, in parse_trips
    newtrip = Trip(trip, requested_time)
  File "/home/yarmo/gh/home-assistant/venv/lib/python3.7/site-packages/ns_api.py", line 606, in __init__
    trip_part = TripSubpart(part)
  File "/home/yarmo/gh/home-assistant/venv/lib/python3.7/site-packages/ns_api.py", line 448, in __init__
    if part_dict['punctuality'] != 100.0:
KeyError: 'punctuality'

So it seems it is missing the punctuality key. Interestingly, this happens for only 1 out of 4 routes that I tried. It doesn't appear like the route is invalid, because by the time you reach line 448, NSAPI is already diving deep in the API's response. Also, line 446 has a similar check and it apparently passes.

Could the NS API be so inconsistent it doesn't show some keys on some routes?

[YarmoM]

Confirmed using postman, punctuality may be missing:

Trip A:

                    "crowdForecast": "HIGH",
                    "punctuality": 91.7,
                    "shorterStock": false,

Trip B:

                    "crowdForecast": "MEDIUM",
                    "shorterStock": false,

I found this specific example on the Rijswijk-Leiden route.

Surely a bug on their part, but we need to account for this.

[aquatix]

What do you propose using as default value? Would None work? Or 0.0?

[YarmoM]

They deserve 0.0 on this one ;-) but None seems a technically more correct answer. That way, NSAPI users could check whether or not it was originally provided and act accordingly.

[Squixx]

I'd agree on the None. Would allow for more consistent use in HA aswel.

Must have gotten lucky when writing this and only test routes that gave a full response..

[Squixx]

actually, i think https://github.com/aquatix/ns-api/commit/4fa15fb70aa38803907fa74c788f92755f122edb fixes this, while still leaving the possiblity to update has_delay based on other values.

Would be cool if we could have another v including this fix

edit:

Upon some further testing it seems the punctuality is mostly only included when it isnt 100% So the above commit is the right fix

[aquatix]

It is a fix for not having punctuality. However, I'm a bit unsure about the check in the first place. Isn't the punctuality the average punctuality of that train line? Now it is used to check if the current train has a delay, but if you look at the NS app, I think that value denotes the percentage shown next to the train, about what the traveller can expect of that train on that line at that time in general (I'll attach a screenshot).

[aquatix]

[Squixx]

right. So it is supposed to be the average punctuality over the last 3 months. Which can be nice, to know. But should be presented in a different way.

I'd say we remove this check, but wonder if we still capture delay's in any meaning full way.

As when im testing on a delayed train. actualDateTime isnt included either...

[Squixx]

I propose to 'solve' this by actually returning None value for actual times / actual platforms, instead of filling these values with planned time / platform.

To make it fully transparent for users. see PR #25

Edit: I see you actually had this implementation of actuals before. Why did you change them to always read planned times?

[YarmoM]

Edit: I see you actually had this implementation of actuals before. Why did you change them to always read planned times?

Because the "actual" should show the "actual" time and platform, not None. Otherwise it should be named "changed" or "delayed" or something similar. Also it doesn't "always read planned times", it either shows delayed time if it exists, else it falls back to planned time.

I'm curious why None would be more transparent, that's what we have the boolean for.

[Squixx]

Main thing is that the boolean is partly based on the punctuality, which doesn't actually reflect any state of the current trip. Just the average over the last 3 months.

Removing that, I tested some trips which showed in the app as being delayed. But I could find no reliable value to determine they were delayed. As even for the delayed trips, in many cases the actual time was not found in the request.

So I'm trying to look for a fallback where even if the actual time isn't found, we still have a reliable way to determine a train is delayed.

Mainly I want my damn hall light to blink when my train is late ;) and have that be as accurate as possible.

Then again maybe it's just a temporary glitch in the api, and actual times will be reliable most of the time. I'll have to test a bit more

Edit: some spelling.

[YarmoM]

Thanks, I get it now. What a hot mess… Alright, we'll wait a bit more, hopefully you'll find reliable cues.

Anyone contacted NS before about their API?

[Squixx]

So, upon some more testing today I get actuals 100% of the time, both for delayed and on time trips.

This leads me to believe their backend can partly fail, returning trip info, but not actual info.

If this is the case, I suggest we move forward as in #25 , as returning None for actuals is the most transperant thing we can do and lets the recieving end know we dont know the actual times and/ or delays.

What do you think?

[YarmoM]

That sounds fair. The logic of using either planned or delayed time will have to be implemented on the client side.

[aquatix]

Agreed, the library shouldn't try to be smarter than a client that is using it. I'll take a look at the PR later today. Thanks for your work, both!

[aquatix]

I left a few comments on the PR, but nothing serious; looking good in my opinion. I really should do a rewrite of my notifications script (always was a good test of the library), but as I nowadays take my bike to work, the urgency isn't really there O:)

[aquatix]

There, merged a bunch of PR's. You guys are great :)

@squixx maybe you want to refork to get an identical history, you then can create a branch for a new feature/fix, from which you can then create a pull request. When a pull request is merged, you can then pull master from the aquatix/ns-api origin, and be up-to-date. (I just noticed that you created your pull request from your master branch, which blocks you from having multiple pull requests at once, and having that one branch that is exactly the same as upstream, which can be quite useful).

[aquatix]

Just released https://pypi.org/project/nsapi/3.0.3/ with the above fixes/changes.

[YarmoM]

I'm starting to see new error logs, also on the Home Assistant Community pages. It seems any attribute is susceptible to being omitted.

Without glancing at the source code, any chance we could implement a check for every attribute whenever they're used? Do we even have another choice?

If deemed feasible, I'll have a go somewhere this week.

[aquatix]

Do you have a few links to those forum threads?

[jperquin]

Just released https://pypi.org/project/nsapi/3.0.3/ with the above fixes/changes.

Copied this latest ns_api.py to /usr/local/lib/python3.7/site-packages in my HA container (0.105.1) on HASSOS (3.9) and am seeing following update errors (and a few new sensors that are failing to create on start-up):

2020-02-10 08:26:35 ERROR (MainThread) [homeassistant.helpers.entity] Update for sensor.groningen_leiden fails Traceback (most recent call last): File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 279, in async_update_ha_state await self.async_device_update() File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 461, in async_device_update await self.hass.async_add_executor_job(self.update) File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run result = self.fn(*self.args, **self.kwargs) File "/usr/src/homeassistant/homeassistant/util/__init__.py", line 240, in wrapper result = method(*args, **kwargs) File "/usr/src/homeassistant/homeassistant/components/nederlandse_spoorwegen/sensor.py", line 208, in update 2, File "/config/ns_api.py", line 929, in get_trips return self.parse_trips(raw_trips, requested_time) File "/config/ns_api.py", line 839, in parse_trips for trip in obj['trips']: KeyError: 'trips'

Seems like it is not just the "punctuality" attribute that is wreaking havoc..

[aquatix]

Hm, 'trips' sounds like it should always be there? Interesting. I hope I have some time today to dive into the matter. I need to write a test program :)

[YarmoM]

From HA community: https://community.home-assistant.io/t/nederlandse-spoorwegen-sensor-list-index-out-of-range/169823/7

[Squixx]

Trips should basically always be there yeah.. Otherwise the call can be seen as a functional fail.

I'd expect with the storm last night again part of the ns infra got overloaded and the api partly(?) fails.

Knowing this, we should find a way to fail more gracefully.

But what would be acceptable as a response?

None for everything? And have the lib user determine what to do with that?

[YarmoM]

None for everything? And have the lib user determine what to do with that?

That would be my preferred solution. One has to assume they'll figure this one out (their own apps surely use similar APIs) and fix this bug. By having None for everything, lib users will continue to work in the future, just with more consistent data.

Other solution would be a custom error where if some attribute is missing, tell the user the NS data was incomplete and repeat the API call if they really need the info.

Or something inbetween. Add an attribute telling the user whether the data is deemed complete or not, then the user can decide whether to try again or just go with it.

[jperquin]

After a HA restart at 08:35 today (with the latest ns_api.py) I am not seeing any more errors. :-)

Fingers crossed.

[aquatix]

@jperquin could be that the NS servers were really busy, or that the library update was sufficient. In any case, good news to hear it works correctly for you again. I think the library should try to catch the case where it does not get a correct response from the API (timeout? It should at least be detectable), and 'just' throw a corresponding error. The client can then decide what it wants to do.

[jperquin]

Just had to restart for an update and had this:

Error doing job: Task exception was never retrieved Traceback (most recent call last): File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 284, in async_update_ha_state self._async_write_ha_state() File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 328, in _async_write_ha_state attr.update(self.device_state_attributes or {}) File "/usr/src/homeassistant/homeassistant/components/nederlandse_spoorwegen/sensor.py", line 190, in device_state_attributes if self._trips[1].departure_time_actual is not None: IndexError: list index out of range

[aquatix]

Hm, that sounds like the HA sensor code does not check how many trips the library returns, that has to be changed in the HA part. Maybe @YarmoM can help with that.

[YarmoM]

Not able to check right now, but I think I already fixed this one… If not, I'll make a PR tomorrow. In any case, this particular error message is unrelated to this repo and should be discussed further over at the HA repo :)