Cleanup CO₂ emissions #6558

t2gran · 2025-03-19T15:16:58Z

Summary

This PR refactor the GTFS GraphBuilder and clean up the code. If contains a few bug fixes, the most important is
to make the emission sandbox module work with other data sources than just the file system. At Entur we uses Google Storage.

Some of the main changes:

Support for data-sources like Google cloud storage
Use dependency injection to inject Emission itinerary decorator into the itinerary filter-chain, the main OTP code does not need to know about any of the emission types like EmissionService or the DecorateWithEmission.
Renamed types to follow naming conventions
Make the logic for resolving feedId public available, so the emission graph builder module can reuse it.
Refactor the Graph build parameters so we can enforce "not null" for source. This is done by splitting the GTFSFeedParameters in two: GtfsFeedParameters and GtfsDefaultParameters. I used inheritance here, but delegation might have beed equally good. The nice thing about inheritance is that the parameters align with the structure of the build-config.

Issue

🟥 No issue for this, this is mostly cleanup code and a few bug fixes

Closes #45

Unit tests

✅ Some new tests are added, while emission tests no focus on business - test-coverage should be higher.

Documentation

✅ Relevant JavaDoc is updated

Changelog

🟥 Not relevant.

Bumping the serialization version id

✅ Needed, the emission repository is changed.

not in GraphBuild only there should be a "OTP Global" scope name-space or FeedInfo.

- I used inheritance to do this, delegation would probably be at least as good. - We can now do proper validation of the required source parameter.

- This add support for using all data-sources, not just the file system

t2gran · 2025-03-19T15:23:14Z

I see that there is a problem with the data-fetchers for Grams - I will investigate.

optionsome

I tested this but the emissions building failed with the following stack trace:

16:46:00.173 ERROR [main]  (OTPMain.java:61) An uncaught error occurred inside OTP: Cannot invoke "java.util.zip.ZipFile.getInputStream(java.util.zip.ZipEntry)" because "this.zipFile" is null
java.lang.NullPointerException: Cannot invoke "java.util.zip.ZipFile.getInputStream(java.util.zip.ZipEntry)" because "this.zipFile" is null
	at org.opentripplanner.datastore.file.ZipFileDataSource.entryStream(ZipFileDataSource.java:75)
	at org.opentripplanner.datastore.file.ZipFileEntryDataSource.asInputStream(ZipFileEntryDataSource.java:56)
	at org.opentripplanner.gtfs.graphbuilder.GtfsBundle$1.getResource(GtfsBundle.java:63)
	at org.opentripplanner.gtfs.graphbuilder.GtfsFeedIdResolver.fromGtfsFeed(GtfsFeedIdResolver.java:39)
	at org.opentripplanner.gtfs.graphbuilder.GtfsBundle.getFeedId(GtfsBundle.java:95)
	at org.opentripplanner.ext.emissions.EmissionsGraphBuilder.buildGraph(EmissionsGraphBuilder.java:49)
	at org.opentripplanner.graph_builder.GraphBuilder.run(GraphBuilder.java:202)
	at org.opentripplanner.standalone.OTPMain.startOTPServer(OTPMain.java:144)
	at org.opentripplanner.standalone.OTPMain.main(OTPMain.java:56)

The GTFS data I used can be downloaded from here

application/src/ext/java/org/opentripplanner/ext/emissions/Co2EmissionsDataReader.java

optionsome · 2025-03-24T14:25:24Z

application/src/ext/java/org/opentripplanner/ext/emissions/EmissionsGraphBuilder.java

+/**
+ * This class allows updating the graph with emissions data from external emissions data files.
+ */
+public class EmissionsGraphBuilder implements GraphBuilderModule {


On this Gram vs Grams dicussion. Should we call all these classes EmissionSomething or EmissionsSomething?

I created an issue for this #6571. I think we should use EmissionSomeThing and not EmissionsSomeThing. I would like to do it in a separate PR.

application/src/main/java/org/opentripplanner/datastore/api/DataSource.java

...ion/src/main/java/org/opentripplanner/graph_builder/model/ConfiguredCompositeDataSource.java

application/src/main/java/org/opentripplanner/gtfs/graphbuilder/GtfsFeedIdResolver.java

.../src/main/java/org/opentripplanner/routing/algorithm/filterchain/ext/EmissionsDecorator.java

application/src/ext/java/org/opentripplanner/ext/emissions/Co2EmissionsDataReader.java

application/src/main/java/org/opentripplanner/routing/algorithm/filterchain/ext/package-info.md

...ation/src/main/java/org/opentripplanner/standalone/configure/ConstructApplicationModule.java

application/src/ext/java/org/opentripplanner/ext/emissions/Co2EmissionsDataReader.java

Co-authored-by: Joel Lappalainen <[email protected]>

t2gran · 2025-03-25T21:28:42Z

The failure (stacktrace) is caused by the zip-file being closed after the GTFS data is read, before the emissions data is loaded. I will look into how this should be resolved. The best solution is probably to clone and open DataSource, but we need to be careful - opening two connections to a connection based data-source (Google Cloud) might cause problems.

t2gran · 2025-03-25T22:02:27Z

The failure was a bug in the ZipFileDataSource the wrapper is perfect for keeping information about the open/closed connections - so if two graph modules uses the same CompositeDataSources then it should be reopened the second time it is accessed. I checked all the implementation we have, it was only the ZipFile version who had this error. It does not clean up its internal state when closed.

…odule The ZipFileDataSource did not cleanup its internal state when it was closed, so opening it a second time failed. The OTP DataSource framework is tailored to address this kind of features, so the fix is simple. None of the other composite data sources has the same bug.

application/src/ext/java/org/opentripplanner/ext/emissions/Co2EmissionsDataReader.java

optionsome · 2025-03-26T20:21:00Z

application/src/main/java/org/opentripplanner/graph_builder/GraphBuilderDataSources.java

+    return store.stopConsolidation();
+  }
+
+  private ConfiguredDataSource<OsmExtractParameters> mapOsmData(DataSource dataSource) {


This method should be lower in the file as it's private. The diff is already difficult to read so it shouldn't matter much if you move one more method a bit.

…EmissionsDataReader.java Co-authored-by: Joel Lappalainen <[email protected]>

optionsome

Looks good, just fix the formatting issue.

leonardehrenfried · 2025-03-31T11:27:10Z

@t2gran you need to merge dev-2.x because a test has moved and the merge of your branch and dev-2.x doesn't compile anymore.

vpaturet

Looks good, no additional comment from my side.

t2gran added 24 commits March 14, 2025 15:54

refactor: Rename Grams to Gram

4620d92

refactor: Simplify DefaultEmissionsService options

ed3a813

refactor: Move ConfiguredDataSource into model

0bec4ac

refactor: Make a ConfiguredCompositeDataSource to type-safety

6a62749

doc: Add doc to DataSource#uri() to avoid wrong usage

35d351c

refactor: Rename EmissionsGraphBuilder from EmissionsModule

43ac98c

refactor: Fix DI for Emission services

a0a3d49

refactor: Move DefaultEmissionsService to internal

ea49c7b

refactor: Move DecorateWithEmission to itinerary

58ce844

refactor: Inject EmissionsDecorator in itinerary filter chain

568e1b7

refactor: Rename to EmissionsRepository from EmissionsDataModel

9e0e6b9

refactor: Extract EmissionsRepository interface

a21ab45

refactor: Convert DecorateWithEmission to a class

2b777c8

refactor: EmissionsConfig feature envy

54208f9

refactor: Move GtfsFeedId to o.o.gtfs.graphbuilder package

ff8681f

refactor: Add factory method to GtfsFeedParameters#of()

f072157

refactor: Move config parameters from GTFS bundle to module

54973fb

refactor: Use GtfsFeedParameters in GtfsBundle as is, do not copy

e665c14

refactor: We should encapsulate feed id (namespace), but ...

22d7f43

not in GraphBuild only there should be a "OTP Global" scope name-space or FeedInfo.

refactor: Rename GtfsFeedId to GtfsFeedIdResolver

01c31b4

refactor: Move GtfsFeedIdResolverTest to correct package

ee451f1

refactor: Split default and feed config into two explicit types

9bc3a21

- I used inheritance to do this, delegation would probably be at least as good. - We can now do proper validation of the required source parameter.

refactor: Make test factory functions in GtfsModule & GtfsBundle

07db62a

feature: Use the data-sources to load emissions.txt

275dedb

- This add support for using all data-sources, not just the file system

t2gran added Technical Debt bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Skip Changelog labels Mar 19, 2025

t2gran added this to the 2.8 (next release) milestone Mar 19, 2025

t2gran requested a review from a team as a code owner March 19, 2025 15:16

review: Codegen tests are failing

559270a

optionsome requested changes Mar 24, 2025

View reviewed changes

t2gran commented Mar 25, 2025

View reviewed changes

application/src/ext/java/org/opentripplanner/ext/emissions/Co2EmissionsDataReader.java Outdated Show resolved Hide resolved

Apply suggestions from code review

e744914

Co-authored-by: Joel Lappalainen <[email protected]>

t2gran mentioned this pull request Mar 25, 2025

Singular vs plural in type names #6571

Closed

t2gran changed the title ~~Cleanup co2 emissions~~ Cleanup CO₂ emissions Mar 25, 2025

t2gran added 2 commits March 25, 2025 21:45

refactor: Avoid cast inside constructor

40353d3

refactor: Make GtfsFeedParameters#source required

b6ebaac

t2gran requested a review from optionsome March 25, 2025 22:19

t2gran added 2 commits March 25, 2025 23:21

refactor: Cleanup in GraphBuilderDataSources

daf119e

t2gran force-pushed the add_co2_emmistions branch from d63570a to 6c5d37a Compare March 25, 2025 22:22

optionsome reviewed Mar 26, 2025

View reviewed changes

t2gran and others added 3 commits March 28, 2025 16:16

Update application/src/ext/java/org/opentripplanner/ext/emissions/Co2…

5e9edc6

…EmissionsDataReader.java Co-authored-by: Joel Lappalainen <[email protected]>

Refactor: Reorder private methods in GraphBuilderDataSources

d2ef16e

Merge remote-tracking branch 'otp/dev-2.x' into add_co2_emmistions

b0621bd

optionsome previously approved these changes Mar 28, 2025

View reviewed changes

refactor: Reformat code

d1b9969

t2gran dismissed optionsome’s stale review via d1b9969 March 31, 2025 09:55

t2gran requested a review from optionsome March 31, 2025 09:55

Merge remote-tracking branch 'otp/dev-2.x' into add_co2_emmistions

87a359a

optionsome approved these changes Apr 4, 2025

View reviewed changes

vpaturet approved these changes Apr 10, 2025

View reviewed changes

t2gran merged commit 9d3ff48 into opentripplanner:dev-2.x Apr 10, 2025
6 checks passed

t2gran pushed a commit that referenced this pull request Apr 10, 2025

Bump serialization version id for #6558

d3239c2

leonardehrenfried mentioned this pull request Apr 11, 2025

Exception when using new refactored emissions code #6609

Closed

t2gran deleted the add_co2_emmistions branch April 11, 2025 20:21

Cleanup CO₂ emissions #6558

Cleanup CO₂ emissions #6558

Uh oh!

Conversation

t2gran commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Changelog

Bumping the serialization version id

Uh oh!

t2gran commented Mar 19, 2025

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

optionsome Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

t2gran Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t2gran commented Mar 25, 2025

Uh oh!

t2gran commented Mar 25, 2025

Uh oh!

Uh oh!

optionsome Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Mar 31, 2025

Uh oh!

vpaturet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

t2gran commented Mar 19, 2025 •

edited

Loading

t2gran Mar 25, 2025 •

edited

Loading