FEAT: S3 remote file paths in one location #423

rymarczy · 2024-08-16T15:11:39Z

This changes attempts to consolidate references to S3 paths all in one location.

Original version of remote_files.py was modified to reduce the verbosity of calling resources defined by the file. Also added all bucket references and lamp file prefix constants to the file.

Hopefully I found all the places where we reference S3 file locations....

Asana Task: https://app.asana.com/0/1205827492903547/1207771349226027

rymarczy · 2024-08-16T15:13:00Z

tests/test_resources.py

-    tm_stop_crossing = LocalS3Location(
-        bucket_name=springboard_bucket,
-        file_prefix=os.path.join(tm_prefix, "STOP_CROSSING"),
-    )
-    tm_geo_node_file = LocalS3Location(
-        bucket_name=springboard_bucket,
-        file_prefix=os.path.join(tm_prefix, "TMMAIN_GEO_NODE.parquet"),
-    )
-    tm_route_file = LocalS3Location(
-        bucket_name=springboard_bucket,
-        file_prefix=os.path.join(tm_prefix, "TMMAIN_ROUTE.parquet"),
-    )
-    tm_trip_file = LocalS3Location(
-        bucket_name=springboard_bucket,
-        file_prefix=os.path.join(tm_prefix, "TMMAIN_TRIP.parquet"),
-    )
-    tm_vehicle_file = LocalS3Location(
-        bucket_name=springboard_bucket,
-        file_prefix=os.path.join(tm_prefix, "TMMAIN_VEHICLE.parquet"),
-    )
-
-    # output of public bus events published by LAMP
-    bus_events = LocalS3Location(
-        bucket_name=public_bucket,
-        file_prefix="bus_vehicle_events",
-    )


Doesn't seem like this stuff was being used

rymarczy · 2024-08-16T15:13:48Z

src/lamp_py/runtime_utils/remote_files.py

+rt_vehicle_positions = S3Location(
+    bucket=S3_SPRINGBOARD,
+    prefix=os.path.join(LAMP, "RT_VEHICLE_POSITIONS"),
+)


Pulled all of these declarations out of the class to reduce the verbosity of referencing them in other parts of the application.

makes sense. it was too long and too gross already.

rymarczy · 2024-08-16T15:16:05Z

src/lamp_py/runtime_utils/remote_files.py

+)
+
+
+class GTFSArchive(S3Location):


sub class with additional function for compressed gtfs archive files

github-actions · 2024-08-16T17:16:50Z

Coverage of commit `c60cb65`

Summary coverage rate:
  lines......: 76.7% (2409 of 3140 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate:
                                                                                     |Lines       |Functions  |Branches    
  Filename                                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================================
  src/lamp_py/bus_performance_manager/gtfs.py                                        |90.9%     77|    -     0|    -      0
  src/lamp_py/bus_performance_manager/gtfs_utils.py                                  | 100%      9|    -     0|    -      0
  src/lamp_py/ingestion/compress_gtfs/gtfs_to_parquet.py                             |76.5%    102|    -     0|    -      0
  src/lamp_py/ingestion/compress_gtfs/schedule_details.py                            |85.4%     96|    -     0|    -      0
  src/lamp_py/ingestion/convert_gtfs.py                                              |79.6%     49|    -     0|    -      0
  src/lamp_py/ingestion/convert_gtfs_rt.py                                           |48.8%    211|    -     0|    -      0
  src/lamp_py/ingestion/utils.py                                                     |61.6%    112|    -     0|    -      0
  src/lamp_py/ingestion_tm/jobs/parition_table.py                                    |45.0%     80|    -     0|    -      0
  src/lamp_py/ingestion_tm/jobs/whole_table.py                                       |75.8%     91|    -     0|    -      0
  src/lamp_py/performance_manager/alerts.py                                          |53.3%    199|    -     0|    -      0
  src/lamp_py/performance_manager/flat_file.py                                       |82.7%    104|    -     0|    -      0
  src/lamp_py/performance_manager/l0_gtfs_static_load.py                             |88.1%    135|    -     0|    -      0
  src/lamp_py/runtime_utils/remote_files.py                                          | 100%     43|    -     0|    -      0

Download coverage report

rymarczy · 2024-08-16T17:17:46Z

pyproject.toml

@@ -54,6 +54,7 @@ pylint = "^3.2.2"
 pytest = "^8.2.1"
 pytest-cov = "^5.0.0"
 types-python-dateutil = "^2.9.0.20240316"
+pytest-env = "^1.1.3"


Use this package for env vars during testing to avoid any ENV VAR mocking requirements.

rymarczy · 2024-08-16T17:30:35Z

tests/ingestion/test_gtfs_converter.py

-@pytest.fixture(name="_set_env_vars")
-def fixture_set_env_vars() -> None:
-    """setup bucket names for this test"""
-    os.environ["SPRINGBOARD_BUCKET"] = "springboard"
-    os.environ["ERROR_BUCKET"] = "error"
-    os.environ["ARCHIVE_BUCKET"] = "archive"
-
-


shouldn't need this with pytest-env package

github-actions · 2024-08-16T17:39:14Z

Coverage of commit `2927301`

Summary coverage rate:
  lines......: 76.3% (2396 of 3140 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate:
                                                                                     |Lines       |Functions  |Branches    
  Filename                                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================================
  src/lamp_py/bus_performance_manager/gtfs.py                                        |90.9%     77|    -     0|    -      0
  src/lamp_py/bus_performance_manager/gtfs_utils.py                                  | 100%      9|    -     0|    -      0
  src/lamp_py/ingestion/compress_gtfs/gtfs_to_parquet.py                             |67.6%    102|    -     0|    -      0
  src/lamp_py/ingestion/compress_gtfs/schedule_details.py                            |81.2%     96|    -     0|    -      0
  src/lamp_py/ingestion/convert_gtfs.py                                              |79.6%     49|    -     0|    -      0
  src/lamp_py/ingestion/convert_gtfs_rt.py                                           |48.8%    211|    -     0|    -      0
  src/lamp_py/ingestion/utils.py                                                     |61.6%    112|    -     0|    -      0
  src/lamp_py/ingestion_tm/jobs/parition_table.py                                    |45.0%     80|    -     0|    -      0
  src/lamp_py/ingestion_tm/jobs/whole_table.py                                       |75.8%     91|    -     0|    -      0
  src/lamp_py/performance_manager/alerts.py                                          |53.3%    199|    -     0|    -      0
  src/lamp_py/performance_manager/flat_file.py                                       |82.7%    104|    -     0|    -      0
  src/lamp_py/performance_manager/l0_gtfs_static_load.py                             |88.1%    135|    -     0|    -      0
  src/lamp_py/runtime_utils/remote_files.py                                          | 100%     43|    -     0|    -      0

Download coverage report

mzappitello

i have a few comments, mostly around if we should be making more constants for remote locations in remote_files.py or generating those paths in the files using them based on the constants we have.

my intuition says everything should be defined there, but i would concede its unlikely we'd need some of these paths outside of the context they're defined in (like hyper file publication).

mzappitello · 2024-08-19T13:08:41Z

pyproject.toml

@@ -77,6 +78,13 @@ log_cli = true
 log_cli_level = "DEBUG"
 verbose = true

+[tool.pytest.ini_options]


thats a cool addition.

mzappitello · 2024-08-19T13:23:30Z

src/lamp_py/runtime_utils/remote_files.py

+S3_SPRINGBOARD: str = os.environ.get("SPRINGBOARD_BUCKET", "")
+S3_PUBLIC: str = os.environ.get("PUBLIC_ARCHIVE_BUCKET", "")
+S3_INCOMING: str = os.environ.get("INCOMING_BUCKET", "")
+S3_ARCHIVE: str = os.environ.get("ARCHIVE_BUCKET", "")
+S3_ERROR: str = os.environ.get("ERROR_BUCKET", "")


i would suggest changing the defaults from "" to "<bucket_name>_unset" to help with debugging?

there is the issue in publishing/performancedata.py around this though where we string match the bucket, but we could handle that differently with a block in the run_on_app_start similarly to how we run the alembic_upgrade_to_head using an env var directly.

mzappitello · 2024-08-19T13:31:20Z

src/lamp_py/ingestion/compress_gtfs/gtfs_to_parquet.py

-    gtfs_tmp_folder = GTFS_PATH.replace(
-        os.getenv("PUBLIC_ARCHIVE_BUCKET"), "/tmp"
-    )
+    gtfs_tmp_folder = compressed_gtfs.s3_uri.replace(S3_PUBLIC, "/tmp")


does it make sense to build in the /tmp local file locations into the S3Location class as well, since it comes up as a pattern somewhat often and the location info is always the same?

looking at it more that looks like a hairy mess thats outside the scope here.

mzappitello · 2024-08-19T13:42:10Z

src/lamp_py/performance_manager/alerts.py

+from lamp_py.runtime_utils.remote_files import (
+    S3_PUBLIC,
+    LAMP,
+)

 from .gtfs_utils import BOSTON_TZ


 class AlertsS3Info:
    """S3 Constant info for Alerts Parquet File"""

-    bucket_name: str = os.environ.get("PUBLIC_ARCHIVE_BUCKET", "")
    s3_path: str = "s3://" + os.path.join(
-        bucket_name, "lamp", "tableau", "alerts", "LAMP_RT_ALERTS.parquet"
+        S3_PUBLIC, LAMP, "tableau", "alerts", "LAMP_RT_ALERTS.parquet"


i think this should be another instance of the S3Location class rather than building up the path in this file.

mzappitello · 2024-08-19T13:51:17Z

src/lamp_py/runtime_utils/remote_files.py

+rt_vehicle_positions = S3Location(
+    bucket=S3_SPRINGBOARD,
+    prefix=os.path.join(LAMP, "RT_VEHICLE_POSITIONS"),
+)


makes sense. it was too long and too gross already.

mzappitello · 2024-08-19T13:53:08Z

src/lamp_py/tableau/jobs/rt_rail.py

-            remote_parquet_path=f"s3://{os.getenv('PUBLIC_ARCHIVE_BUCKET')}/lamp/tableau/rail/LAMP_ALL_RT_fields.parquet",
+            remote_parquet_path=f"s3://{S3_PUBLIC}/{LAMP}/tableau/rail/LAMP_ALL_RT_fields.parquet",


again it feels like something that could be a constant in remote_files.py rather than building it up here.

github-actions · 2024-08-20T16:42:48Z

Coverage of commit `6e32feb`

Summary coverage rate:
  lines......: 76.7% (2412 of 3143 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate:
                                                                                     |Lines       |Functions  |Branches    
  Filename                                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================================
  src/lamp_py/bus_performance_manager/gtfs.py                                        |90.9%     77|    -     0|    -      0
  src/lamp_py/bus_performance_manager/gtfs_utils.py                                  | 100%      9|    -     0|    -      0
  src/lamp_py/ingestion/compress_gtfs/gtfs_to_parquet.py                             |76.5%    102|    -     0|    -      0
  src/lamp_py/ingestion/compress_gtfs/schedule_details.py                            |85.4%     96|    -     0|    -      0
  src/lamp_py/ingestion/convert_gtfs.py                                              |79.6%     49|    -     0|    -      0
  src/lamp_py/ingestion/convert_gtfs_rt.py                                           |48.8%    211|    -     0|    -      0
  src/lamp_py/ingestion/utils.py                                                     |61.6%    112|    -     0|    -      0
  src/lamp_py/ingestion_tm/jobs/parition_table.py                                    |45.0%     80|    -     0|    -      0
  src/lamp_py/ingestion_tm/jobs/whole_table.py                                       |75.8%     91|    -     0|    -      0
  src/lamp_py/performance_manager/alerts.py                                          |53.3%    199|    -     0|    -      0
  src/lamp_py/performance_manager/flat_file.py                                       |82.7%    104|    -     0|    -      0
  src/lamp_py/performance_manager/l0_gtfs_static_load.py                             |88.1%    135|    -     0|    -      0
  src/lamp_py/runtime_utils/remote_files.py                                          | 100%     46|    -     0|    -      0

Download coverage report

mzappitello · 2024-08-20T18:49:30Z

LGTM. Merge away.

PR #423 re-factor broke compressed GTFS process by prefixing the gtfs_temp_folder declaration with s3://

remote_file bonanza

f90cd91

rymarczy commented Aug 16, 2024

View reviewed changes

rymarczy force-pushed the feat_remote_locations_everywhere branch from 9c8aa4a to 1f52ba3 Compare August 16, 2024 16:13

fix env test

c60cb65

rymarczy force-pushed the feat_remote_locations_everywhere branch from 1f52ba3 to c60cb65 Compare August 16, 2024 17:09

rymarczy commented Aug 16, 2024

View reviewed changes

rymarczy requested a review from mzappitello August 16, 2024 17:18

drop compressed_gtfs patch

8d729f5

rymarczy commented Aug 16, 2024

View reviewed changes

unused imports

2927301

mzappitello reviewed Aug 19, 2024

View reviewed changes

rymarczy added 2 commits August 19, 2024 11:31

pr comments #1

872d77e

fix bad test

6e32feb

rymarczy force-pushed the feat_remote_locations_everywhere branch from dcce7c1 to 6e32feb Compare August 20, 2024 16:37

rymarczy requested a review from mzappitello August 20, 2024 18:08

rymarczy merged commit b39c127 into main Aug 20, 2024
5 checks passed

rymarczy mentioned this pull request Aug 20, 2024

FIX: Bad Compress GTFS Local Folder #427

Merged

rymarczy added a commit that referenced this pull request Aug 20, 2024

FIX: Bad compress_gtfs Local Folder (#427)

37a016b

PR #423 re-factor broke compressed GTFS process by prefixing the gtfs_temp_folder declaration with s3://

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: S3 remote file paths in one location #423

FEAT: S3 remote file paths in one location #423

rymarczy commented Aug 16, 2024

rymarczy Aug 16, 2024

rymarczy Aug 16, 2024

mzappitello Aug 19, 2024

rymarczy Aug 16, 2024

github-actions bot commented Aug 16, 2024

rymarczy Aug 16, 2024

rymarczy Aug 16, 2024

github-actions bot commented Aug 16, 2024

mzappitello left a comment

mzappitello Aug 19, 2024

mzappitello Aug 19, 2024

mzappitello Aug 19, 2024

mzappitello Aug 19, 2024

mzappitello Aug 19, 2024

mzappitello Aug 19, 2024

mzappitello Aug 19, 2024

mzappitello Aug 19, 2024

github-actions bot commented Aug 20, 2024

mzappitello commented Aug 20, 2024

		remote_parquet_path=f"s3://{os.getenv('PUBLIC_ARCHIVE_BUCKET')}/lamp/tableau/rail/LAMP_ALL_RT_fields.parquet",
		remote_parquet_path=f"s3://{S3_PUBLIC}/{LAMP}/tableau/rail/LAMP_ALL_RT_fields.parquet",

FEAT: S3 remote file paths in one location #423

FEAT: S3 remote file paths in one location #423

Conversation

rymarczy commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2024

Coverage of commit c60cb65

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2024

Coverage of commit 2927301

mzappitello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 20, 2024

Coverage of commit 6e32feb

mzappitello commented Aug 20, 2024

Coverage of commit `c60cb65`

Coverage of commit `2927301`

Coverage of commit `6e32feb`