feat: abort on error

[lionel-nj]

Summary:

This PR provides support to abort the validation process on the first encountered ERROR as proposed in #760.

Expected behavior:

Theoretically:

• Validator should gracefully be shut down after the first ERROR in validation process.
• Said notice should be added to the noticeContainer and exported in the final .json file.

At present:

• the validator exits with non-zero exit value 1
• the ERROR notice is not added to the noticeContainer
• tests are broken

Please make sure these boxes are checked before submitting your pull request - thanks!

• Run the unit tests with gradle test to make sure you didn't break anything
• Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
• Linked all relevant issues
• Include screenshot(s) showing how this pull request works and fixes the issue(s)

[lionel-nj]

As discussed @barbeau, @maximearmstrong the first changes to this PR. Careful! Tests do not pass yet, and the functionality are not all ready.
The validator can be run with the following configuration:
-u http://webapps.thebus.org/transitdata/Production/google_transit.zip -o output -t 4 -f us-hawai

Future<Type>.get() throws an Exception on ErrorDetectedException: would it be better if we implemented an Observer pattern in order to avoid using exceptions? The Observer would reside in each NoticeContainer, if the Observer detects an ERROR then it executes some code that will trigger a system exit. But something is unclear to me: what would happen if we exited the system right after an ERROR is added to a noticeContainer? Would this call to .get block the reste of the processus since what would be returned would be an ExecutionException caught later:

gtfs-validator/core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsFeedLoader.java

Line 147 in f02636f

TableAndNoticeContainers containers = f.get();

@aababilov Do you have some recommendations to approach this?

[aababilov]

I am unsure if I understand the rationale behind that change.

@lionel-nj I would be glad to discuss your the new ideas on changing the core before you start implementation. It is fine if you just have an idea but the code is not ready yet.

Validator should gracefully be shut down after the first ERROR in validation process.

Speaking of the "first ERROR": why would we want that and why would GTFS provider want that?

Said notice should be added to the noticeContainer and exported in the final .json file.

We already add all notices to noticeContainer and export them to .json files.

At present:
the validator exits with non-zero exit value 1
the ERROR notice is not added to the noticeContainer
tests are broken

All those statements seem to be wrong. Do you mind to provide sample datasets when some of those statements are true?

[barbeau]

Speaking of the "first ERROR": why would we want that and why would GTFS provider want that?

@aababilov In theory, the goal is to cut down on noisy and potentially confusing/invalid output from the validator that may be unpredictable after the first semantic ERROR occurs. It's the same reason why the validator currently aborts if the feed isn't parsed successfully.

@lionel-nj I think this new implementation in this PR would only apply to rules that extend FileValidator, though, right? Looking at GtfsFeedLoader Line 166, currently in master branch if the feed fails to parse from any of the earlier validation failures then the file validators aren't invoked. Those rules cover most of the coherency issues like missing keys, invalid values, etc. Are there good examples of existing semantic rules that are FileValidators that would cause chained ERROR events that we would want to prevent?

One challenge I see here is that we don't currently enforce any logical ordering on FileValidators. So the validator doesn't have any information about what semantic ERRORS could cause other semantic ERRORs. We'd need to define this ordering if we want to guarantee that upstream errors are the first to execute and halt the validator, and not the downstream errors - if this isn't enforced, the output would be potentially even more confusing to producers, as then they wouldn't see the root cause (upstream ERRORs). This also has implications on multi-threading and performance, as some threads may need to wait for others if there are order dependencies between them. So I'm not sure that the overall additional complexity and potential performance hit are worth effectively filtering the semantic error output, unless we can come up with clear cases that would cause producers to waste a lot of time chasing red herrings. If we can identify these cases, it may be better to simply code around them in the *Validator classes to avoid triggering errors in these cases.

[lionel-nj]

@lionel-nj I think this new implementation in this PR would only apply to rules that extend FileValidator, though, right?

Exactly!

Are there good examples of existing semantic rules that are FileValidators that would cause chained ERROR events that we would want to prevent?

For now I see these ones:

• StopTimeArrivalAndDepartureTimeValidator -> BlockTripsWithOverlappingStopTimesValidator
• MissingTripEdgeValidator -> BlockTripsWithOverlappingStopTimesValidator

[lionel-nj]

We already add all notices to noticeContainer and export them to .json files.

@aababilov Indeed, I tried to explicit what should be witnessed when I should have explained what this PR should do: abort on the first error.

All those statements seem to be wrong. Do you mind to provide sample datasets when some of those statements are true?

Running this branch in IntelliJ with the following config returns the output I described:

-u http://webapps.thebus.org/transitdata/Production/google_transit.zip -o output -t 4 -f us-hawai

#765 (comment)

[aababilov]

For now I see these ones:
StopTimeArrivalAndDepartureTimeValidator -> BlockTripsWithOverlappingStopTimesValidator
MissingTripEdgeValidator -> BlockTripsWithOverlappingStopTimesValidator

GTFS Validator is designed to run its validations in parallel - this allows to validate faster and benefit from modern multithreaded environments. Engineers should not rely on any particular order of invoking the FileValidators. All FileValidators must be independent from each other and they also should not rely on custom SingleEntityValidators. Remember that there will be more validations added in future. That would be hard to keep in mind relations between validation rules and to debug and maintain them.

@aababilov Indeed, I tried to explicit what should be witnessed when I should have explained what this PR should do: abort on the first error.

I do not think that this is the best decision. Let me give you an example: you need to visit local authorities and lodge 10 documents. You come to the office and they tell you that the first document has a typo. You fix the document and come another day. They tell you that the first document is OK but the second has a typo. After ten visits to the office, you might ask: why haven't they checked all the documents on the first day? Same for GTFS validation and also for compile errors in software: you really want to know as more errors as possible during the first validation.

Running this branch in IntelliJ with the following config returns the output I described:
-u http://webapps.thebus.org/transitdata/Production/google_transit.zip -o output -t 4 -f us-hawai

That works perfectly for me. The validator exits with 0 code and the ERRORS are added to the JSON reports.

[barbeau]

@aababilov A related topic - what are your thoughts on combining multiple notice checks in the same validator class? For example, looks like StopTimeWithDepartureBeforeArrivalTimeNotice and StopTimeWithArrivalBeforePreviousDepartureTimeNotice are already consolidated within StopTimeArrivalAndDepartureTimeValidator. FastTravelBetweenStopsNotice (WIP in PR #710) depends on stop times to calculate travel speed, so times out-of-order could produce strange results.

Some options:

1. Add checks for FastTravelBetweenStopsNotice to StopTimeArrivalAndDepartureTimeValidator - Pros are that this avoids another loop through stop_times.txt and we are already checking for the preconditions (valid time order) for FastTravelBetweenStopsNotice to be calculated correctly. Con is an increased complexity of the single validator class for multiple notices.
2. Add checks within separate TripTravelSpeedValidator for valid times (but don't generate notice for invalid times) - Pros are that this ensures the output of FastTravelBetweenStopsNotice is valid and it doesn't add complexity to a single validator class for this and other notices. Cons are duplicating some code for the time checks and another loop through stop_times.txt for this separate validator.
3. Don't check within separate TripTravelSpeedValidator for valid times - Pros are simplicity of implementation of this new validator for FastTravelBetweenStopsNotice in it's own class without any duplicate code for time checks. Cons are potentially strange output when time values are wrong, plus another loop through stop_times.txt for this separate validator.

Thoughts? Options 1) and 2) accomplish the same goal as this PR.

[lionel-nj]

GTFS Validator is designed to run its validations in parallel - this allows to validate faster and benefit from modern multithreaded environments. Engineers should not rely on any particular order of invoking the FileValidators. All FileValidators must be independent from each other and they also should not rely on custom SingleEntityValidators. Remember that there will be more validations added in future. That would be hard to keep in mind relations between validation rules and to debug and maintain them.

So I suppose that solution 2 @barbeau suggested is better suited. We will have duplicate code, but each validator will be kept independant.

I do not think that this is the best decision. Let me give you an example: you need to visit local authorities and lodge 10 documents. You come to the office and they tell you that the first document has a typo. You fix the document and come another day. They tell you that the first document is OK but the second has a typo. After ten visits to the office, you might ask: why haven’t they checked all the documents on the first day? Same for GTFS validation and also for compile errors in software: you really want to know as more errors as possible during the first validation.

Would you suggest we continue the validation process after the first error is solution 2 is implemented?

That works perfectly for me. The validator exits with 0 code and the ERRORS are added to the JSON reports.

Hmm @barbeau what is the output that you get when you run this config in IntelliJ: -u http://webapps.thebus.org/transitdata/Production/google_transit.zip -o output -t 4 -f us-hawai?

system_errors.txt

report.json

execution log:

[barbeau]

Hmm @barbeau what is the output that you get when you run this config in IntelliJ: -u http://webapps.thebus.org/transitdata/Production/google_transit.zip -o output -t 4 -f us-hawai?

Using code in this PR (branch abort-on-error) I get the same output as @lionel-nj documented in #765 (comment) above.

[dbabramov]

@aababilov In theory, the goal is to cut down on noisy and potentially confusing/invalid output from the validator that may be unpredictable after the first semantic ERROR occurs. It's the same reason why the validator currently aborts if the feed isn't parsed successfully.

@barbeau I think that introducing abort-on-first-error is going to be problematic for a couple of reasons:

1. I am not sure how terminating on the first error is going to cut down on noisy and potentially confusing output.
   Chances are that it is actually going to achieve the opposite - for a feed has multiple different errors, a multithreaded validator may terminate on a different error every time. The output is going to be inconsistent and unreproducible.

2. I think it is useful for a data provider to see the entirety of errors in the feed that they need to fix (even though some of those error may be downstream from the previous ones).
   Seeing all errors can give data providers much better understanding of the amount of effort involved in fixing the output from their systems, than detecting and fixing these errors drop-by-drop.

The risk of some errors resulting in others and creating confusion exists indeed.
However in my mind it is dwarfed by the problems that data providers are going to face if they have to go walk in the darkness - fixing errors one-by-one, and not knowing whether a disappearance of the previous error is due to a successful fix, or a termination of a different thread that found a different error first.

In my opinion, the validator should endeavour to:
a) detect and report as many errors as it can, provided that it can continue functioning (while recognising that some errors, such as failed integrity checks, need to prevent further stages of validation).
b) produce an output as consistent and reproducible as possible.

Thanks!

[dbabramov]

A good analogy for GTFS Validator is that of a compiler.
GTFS Validator is effectively a compiler of GTFS feeds, just like a C++/Java compiler is a C++/Java code Validator.

Terminating a compiler on the first error is unlikely to be a winning feature of that compiler: the top error is not necessarily the one that a developer would want to fix first.
A good compiler will normally show reports for all errors it could detect before terminating.
These error reports are reproducible, and ideally shown in the same order.
That way a developer could decide which errors matter more, and which ones should be fixed first.

GTFS validator is a compiler of GTFS that a data producer uses to validate their GTFS data, and fix errors in them.

[barbeau]

@dbabramov @aababilov Thanks for the feedback, I agree with your concerns.

@lionel-nj I would suggest we close this PR and we can work on more targeted solutions within validators (see some thoughts in #765 (comment)) when we know that bad data could influence a validator's output.

[lionel-nj]

Thanks @aababilov, @dbabramov, and @barbeau for your feedback. Closing now.

[nackko]

An interesting read indeed!
My take out of it is that the validator is not deterministic indeed.