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

Refactor OTP to have new OTP classes to replace the OBA GTFS classes #2495

Merged
merged 35 commits into from
Aug 30, 2018

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Sep 12, 2017

This PR is discussed in the opentripplanner-dev group.

Issue 2494 describe the changes i detail.

There is a lot of commits here, feel free to squash them, but be aware that som of them is necessary to preserve the git history.

t2gran and others added 9 commits September 12, 2017 16:39
…ebusaway2'. (Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…g package is done in 2 commits to preserve git history. (Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
… best place to map between the to models. (Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
… serialization.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…resentation). This is probably a god idea, but it was not used.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…nd after refactoring.

Remove unused code, collapse DAO inheritance structure to get rid of unnecessary complexity.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
Fixed member formatting, removing prefix: '_', and changing constant names to upper case.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
@t2gran t2gran force-pushed the split_gtfs_and_internal_model branch from 4f944d7 to 3c4f5dd Compare September 12, 2017 23:04
*/
public class MapCollection {
static <S,T> Collection<T> mapCollection(Collection<S> entites, Function<S,T> mapper) {
return entites == null ? null : entites.stream().map(mapper).collect(Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "s/entites/entities/"

Thomas Gran added 3 commits September 19, 2017 14:39
  - Spelling errors fixed.
  - All new code formatted with the project profile.
  - Minor code style improvements, like Java 8 style generics.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
CalendarServiceDataFactoryImpl into one.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
onebusaway2 into package 'org.opentripplanner.calendar.impl'.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
@t2gran t2gran force-pushed the split_gtfs_and_internal_model branch from ef6d328 to 52c98c5 Compare September 20, 2017 19:58
Thomas Gran and others added 11 commits October 4, 2017 14:48
…ceInterval.java

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…P transit model.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…fy it.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…lanner.util'.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…ner.model'.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…ner.model'.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
… to 'org.opentripplanner.model.impl'.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
…P Tranist model - Note these objects do

not have ids in the GTFS model.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
@t2gran t2gran force-pushed the split_gtfs_and_internal_model branch from 0be366b to f6d27f1 Compare November 28, 2017 13:33
@t2gran t2gran changed the title Refactor OTP to have new OTP classes to replace the OBA GTFS classes (Issue #2494) Refactor OTP to have new OTP classes to replace the OBA GTFS classes Jan 18, 2018
@abyrd
Copy link
Member

abyrd commented Aug 16, 2018

A final comment on memory consumption (I don't believe the memory usage situation is a serious concern now, just something we need to understand and remember). In the mailing list message https://groups.google.com/d/msg/opentripplanner-dev/5ij1PJAVvs4/PommfsgOCQAJ you observed no difference between the pre- and post-split OTP. The code included in the message has the line that forces garbage collection at every measurement commented out. Was it commented out when the measurement CSV files were produced?

t2gran added 2 commits August 16, 2018 11:32
… copied from ..." to "This file is based on code copied from project X, see the LICENSE file for further information."
@t2gran
Copy link
Member Author

t2gran commented Aug 16, 2018

@abyrd I am not sure, probably tested with and without, but the submitted log files are mote likely without. Note! The gc() call is just a request to do gc - the VM may or may not perform one - the most common thing is not to do a full gc. I wish I had run the test with the JVM flagg -verbose:gc, then it would be clear.

  - Rename GTFSPatternHopFactory to PatternHopFactory
  - Rename method mapDao(...) to mapGtfsDaoToOTPTransitService(...)
  - Rename OtpTransitDaoMapper to GTFSToOtpTransitServiceMapper
  - Rename OtpTransitBuilder to OtpTransitServiceBuilder
  - Rename GTFSPatternHopFactory to PatternHopFactory
  - Add JavaDoc to all mapper classes and OtpTransitServiceBuilder
  - Cleanup JavaDoc in OtpTransitServiceImpl
  - Delete InterliningTrip (not used in PatternHopFactory)
@abyrd
Copy link
Member

abyrd commented Aug 17, 2018

Thanks for the comments and updates @t2gran. What do you think about renaming FeedId to FeedScopedId before the merge?

@t2gran
Copy link
Member Author

t2gran commented Aug 17, 2018

I will do that. What should i do with the FeedIdSerializer, it is not needed any more. We can now annotate the FeedScopedId so it is serializable, but we should probably think through this and do a proper cleanup. We can do this as part of #1671 or the best is probably to split the #1671 into to Issues/PRs. One for general cleanup of the API and one for the specific RouteMatcher problem. See also: line IndexGraphQLSchema@line 323 and Segment @line 40.

Copy link
Member

@abyrd abyrd left a comment

Choose a reason for hiding this comment

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

I think we have covered all the details, thanks for all the effort on this change! @fpurcell we need a second positive review before we can merge this - you have actually tested this code on Portland data and observed no ill effects. Can you give it one last review from that same point of view - either test it out or let us know if you can approve the changes based on your past testing?

@abyrd
Copy link
Member

abyrd commented Aug 23, 2018

As a matter of procedure @t2gran can you add a line to the changelog as part of this PR?

@fpurcell
Copy link
Member

Hey Andrew & Thomas ... I'll get it tested before the weekend, and add an approval if it passes my test suite.

BTW, there's quite a few files being changed, and I'm sorry I don't have the bandwidth to do anything more than build-graph, run-tests, report-back-on-results, approve-on-test-success. Thus, my approval is simply based on running a regression check. I'd like to note being slightly uncomfortable giving approval w/only a minimal scan of the actual code changes (but not uncomfortable enough to not submit a review :-)); and assume that other reviewers are comfortable with the actual code changes.

@fpurcell
Copy link
Member

fpurcell commented Aug 24, 2018

FYI: firstly, I am able to successfully build otp.jar (using jdk 1.8.0_181). But that said, I am seeing the following when I run 'mvn package' :

[INFO] Compiling 875 source files to /u01/home/otp/OpenTripPlanner/target/classes
[WARNING] /u01/home/otp/OpenTripPlanner/src/main/java/org/opentripplanner/analyst/cluster/JobSimulator.java: Some input files use or override a deprecated API.
[WARNING] /u01/home/otp/OpenTripPlanner/src/main/java/org/opentripplanner/analyst/cluster/JobSimulator.java: Recompile with -Xlint:deprecation for details.
[WARNING] /u01/home/otp/OpenTripPlanner/src/main/java/org/opentripplanner/routing/graph/Graph.java: Some input files use unchecked or unsafe operations.
[WARNING] /u01/home/otp/OpenTripPlanner/src/main/java/org/opentripplanner/routing/graph/Graph.java: Recompile with -Xlint:un
...
[INFO] Running org.opentripplanner.updater.bike_rental.TestBikeRentalStationSource
Bike rental station ZAC SAINT SULPICE at 48.132100, -1.635280
Bike rental station VILLEJEAN-UNIVERSITE at 48.121075, -1.704122
Bike rental station TURMEL at 48.122475, -1.651089
Bike rental station TNB at 48.107697, -1.673695
17:42:19.994 WARN (SmooveBikeRentalDataSource.java:63) Error parsing bike rental station B08
java.lang.NumberFormatException: empty String
	at sun.misc.FloatingDecimal.readJavaFormatString(FloatingDecimal.java:1842) ~[na:1.8.0_181]
	at sun.misc.FloatingDecimal.parseDouble(FloatingDecimal.java:110) ~[na:1.8.0_181]
	at java.lang.Double.parseDouble(Double.java:538) ~[na:1.8.0_181]
	at org.opentripplanner.updater.bike_rental.SmooveBikeRentalDataSource.makeStation(SmooveBikeRentalDataSource.java:59) ~[classes/:na]
	at org.opentripplanner.updater.bike_rental.GenericJsonBikeRentalDataSource.parseJSON(GenericJsonBikeRentalDataSource.java:132) [classes/:na]
	at org.opentripplanner.updater.bike_rental.GenericJsonBikeRentalDataSource.update(GenericJsonBikeRentalDataSource.java:90) [classes/:na]
	at org.opentripplanner.updater.bike_rental.TestBikeRentalStationSource.testSmoove(TestBikeRentalStationSource.java:35) [test-classes/:na]
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.8.0_181]
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:1.8.0_181]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.8.0_181]
	at java.lang.reflect.Method.invoke(Method.java:498) ~[na:1.8.0_181]
	at junit.framework.TestCase.runTest(TestCase.java:176) [junit-4.11.jar:na]
	at junit.framework.TestCase.runBare(TestCase.java:141) [junit-4.11.jar:na]
	at junit.framework.TestResult$1.protect(TestResult.java:122) [junit-4.11.jar:na]
	at junit.framework.TestResult.runProtected(TestResult.java:142) [junit-4.11.jar:na]
	at junit.framework.TestResult.run(TestResult.java:125) [junit-4.11.jar:na]
	at junit.framework.TestCase.run(TestCase.java:129) [junit-4.11.jar:na]
	at junit.framework.TestSuite.runTest(TestSuite.java:255) [junit-4.11.jar:na]
	at junit.framework.TestSuite.run(TestSuite.java:250) [junit-4.11.jar:na]
	at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:84) [junit-4.11.jar:na]
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:365) [surefire-junit4-2.21.0.jar:2.21.0]
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:273) [surefire-junit4-2.21.0.jar:2.21.0]
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238) [surefire-junit4-2.21.0.jar:2.21.0]
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159) [surefire-junit4-2.21.0.jar:2.21.0]
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:379) [surefire-booter-2.21.0.jar:2.21.0]
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:340) [surefire-booter-2.21.0.jar:2.21.0]
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:125) [surefire-booter-2.21.0.jar:2.21.0]
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:413) [surefire-booter-2.21.0.jar:2.21.0]
Bike rental station Hamn at 60.167913, 24.952269
Bike rental station Fake at 60.000000, 24.000000
Bike rental station Foo at 61.000000, 25.000000
...
[WARNING] Tests run: 485, Failures: 0, Errors: 0, Skipped: 2

Copy link
Member

@fpurcell fpurcell left a comment

Choose a reason for hiding this comment

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

This PR passed the TriMet test suite. The graph, data and config files are all located here: http://maps7.trimet.org/p/

@abyrd
Copy link
Member

abyrd commented Aug 24, 2018 via email

@abyrd
Copy link
Member

abyrd commented Aug 24, 2018

Also, thanks for pointing out that NumberFormat warning in the tests @fpurcell. I will see if I can fix that before I merge.

@abyrd
Copy link
Member

abyrd commented Aug 24, 2018

@fpurcell checking out the test org.opentripplanner.updater.bike_rental.TestBikeRentalStationSource#testSmoove. This uses a JSON data source containing the following bike station definition:

{
         "name" : "B08 Invalid",
         "operative" : true,
         "coordinates" : "",
         "style" : "",
         "avl_bikes" : 5,
         "free_slots" : 5,
         "total_slots" : 5
      }

And a comment in the test states // Invalid station without coordinates should be ignored so this test is intentionally checking that the failure you're seeing does not entirely prevent bike data parsing.

@abyrd
Copy link
Member

abyrd commented Aug 24, 2018

After catching this PR up to master, we get a build error. This is due to the fact that the OBA FeedInfo objects now have String IDs instead of Integer IDs. It is easy to update the OTP code to expect Strings (I've already done this), but in the process I also realize that Agencies are also identified by a single String, rather than an AgencyAndId / FeedScopedId. This discrepancy is due to the same difference of perspective that gives us OBA's AgencyAndId versus OTP's new FeedScopedId. OTP considers entities to be unique within a feed (and all feeds must have a unique ID), OBA considers them to be unique within an Agency (and all Agencies must have a unique ID or be remapped).

So unfortunately we'll need to somehow inject feed ID information into these IDs. But this leads me to realize that we haven't verified how this code behaves when loading multiple feeds containing the same Agency ID with colliding stop IDs (as is/was the case in New York and other places where multiple feeds defined agency 1, all with stop IDs 1, 2, 3, 4...).

Unfortunately some additional testing is necessary here, and to avoid a regression we may need some additional code to handle feed IDs, and some additional Javadoc explaining the process by which multiple feeds are loaded and merged. It is very hard to predict the effect of some existing code, e.g. the call to reader.setDefaultAgencyId(gtfsFeedId.getId()) in GtfsModule which seems to tell the reader to use the FeedId as the default AgencyId, but what about entities that have a declared AgencyId in the feed? And all the code around GTFSModule line 200 which performs mappings on the OBA side.

Now that we have our own internal model, I think we can skip these hacks and just provide the FeedId to the mapper classes, allowing them to blanket overwrite whatever OBA thinks the identifier scope string is.

We should also add automated tests that load multiple feeds with and without declared IDs, to ensure that it behaves as expected and entities do not collide. I can do that.

@abyrd
Copy link
Member

abyrd commented Aug 24, 2018

I've created an automated test which shows there are unexpected ID collisions (which may have existed even before these changes). The existing code is trying to make both agency IDs and feed IDs unique, and keys some internal OTP maps on Agency IDs as if they are expected to be unique. All this can be fixed with FeedScopedId now under our control. I'm going to see how the test behaves on the master branch code and propose a fix.

@t2gran
Copy link
Member Author

t2gran commented Aug 24, 2018

FeedId id changed from int to String in #2637 when switching org.onebusaway:onebusaway-gtfs from 1.3.5-conveyal-SNAPSHOT-2 to 1.3.41. @abyrd Should I fix this - I also had to do this to get the code running?

When I did this, I did my best to keep the exact same behavior - replicating both wanted and more suspicious behavior. I agree we should fix the agency id and inject feed id to it, but I think that is a separate issue - @abyrd I am not sure if you want to fix this issue before merging or as a separate issue later?

Unfortunately some additional testing is necessary here, and to avoid a regression we may need
some additional code to handle feed IDs, and some additional Javadoc explaining the process by
which multiple feeds are loaded and merged. It is very hard to predict the effect of some existing
code, e.g. the call to reader.setDefaultAgencyId(gtfsFeedId.getId()) in GtfsModule which seems
to tell the reader to use the FeedId as the default AgencyId, but what about entities that have a
declared AgencyId in the feed? And all the code around GTFSModule line 200 which performs
mappings on the OBA side.

This should work exactly the same as before - but I have to admit that it is a long time since I did this, and I do not remember how and what tested. I do know, that I tried to add UnitTests on all new code (not the code i imported, but mapping and so on), and that I did test the feed id loading.
@abyrd Do you want me to write an integration test with multiple feedIds - testing the different senario you mentioned?

@t2gran
Copy link
Member Author

t2gran commented Aug 24, 2018

@abyrd I did not see you last comment - let me know if I can do some of this.

@abyrd
Copy link
Member

abyrd commented Aug 30, 2018

@t2gran I suspect that the "incorrect" ID collision behavior I was seeing already exists in mainline OTP. I hope you didn't go to too much effort to replicate that behavior in this PR :) What I'll do is create a separate issue for the ID collision problem, create a branch adding a new unit test in commits referencing that issue, and see how it behaves when branching off master OTP. If it fails there too, we'll just make the Integer -> String ID change on this PR and merge, and fix the collisions as a separate issue.

@t2gran t2gran merged commit e9f2db1 into opentripplanner:master Aug 30, 2018
@abyrd
Copy link
Member

abyrd commented Aug 30, 2018

🎆 Success! Now we can move on to adding Netex...

@t2gran t2gran deleted the split_gtfs_and_internal_model branch September 24, 2019 14:14
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.

7 participants