Skip to content

feat(ingestion): allow cause_detail and effect_detail to be Translations#765

Merged
runkelcorey merged 5 commits into
mainfrom
feature-alerts-translated-strings
Jun 22, 2026
Merged

feat(ingestion): allow cause_detail and effect_detail to be Translations#765
runkelcorey merged 5 commits into
mainfrom
feature-alerts-translated-strings

Conversation

@runkelcorey

Copy link
Copy Markdown
Collaborator

🔔 handle new & old schemas when ingesting alerts

What changes does this PR propose?

Implements temporary logic until Alerts Manager changes schema that

  1. allows for cause_detail and effect_detail to either be strings or Translations
  2. regardless of the input, outputs a table with
    • cause_detail.translation
    • effect_detail.translation
    • cause_detail
    • effect_detail

Not in scope for this PR is updates to performance manager to actually read the English translations from Alerts in the new schema.

How were these changes validated?

  1. Updated PM alerts test to ensure that these additions wouldn't cause problems for PM
  2. Adds ingestion test to check what happens if data with the new schema is appended to data with the old schema
  3. Soaking in dev w/o errors
  4. No PM Alerts errors in dev

What questions should reviewers consider?

  1. Are you okay with this so long as I create a ticket to clean it up after migration?

@runkelcorey runkelcorey self-assigned this Jun 18, 2026
@runkelcorey runkelcorey requested a review from a team as a code owner June 18, 2026 21:28
@runkelcorey runkelcorey requested a review from huangh June 18, 2026 21:28
@github-actions

Copy link
Copy Markdown

LCOV of commit 05e19ae during Continuous Integration (Python) #1993

Summary coverage rate:
  lines......: 64.7% (3596 of 5558 lines)
  functions..: 29.9% (275 of 920 functions)
  branches...: no data found

Files changed coverage rate:
                                                                                     |Lines       |Functions  |Branches    
  Filename                                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================================
  src/lamp_py/ingestion/config_rt_alerts.py                                          |98.2%     56|41.7%    12|    -      0
  src/lamp_py/ingestion/convert_gtfs_rt.py                                           |88.4%    242|50.0%    26|    -      0

@huangh huangh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks reasonable - decide if any of my comments are valid or if you can live with it - i think the only consequential change would be to further flatten the translation.language out, but if you've already been testing and are okay with the ergonomics of it, it's not a dealbreaker for me.

i could see arguments against doing what i suggested..

Comment thread src/lamp_py/ingestion/config_rt_alerts.py
Comment thread src/lamp_py/ingestion/convert_gtfs_rt.py
Comment thread src/lamp_py/ingestion/config_rt_alerts.py Outdated
Comment thread src/lamp_py/ingestion/config_rt_alerts.py Outdated
Comment thread src/lamp_py/ingestion/config_rt_alerts.py
@github-actions

Copy link
Copy Markdown

LCOV of commit 78f2d38 during Continuous Integration (Python) #1994

Summary coverage rate:
  lines......: 64.7% (3596 of 5558 lines)
  functions..: 29.9% (275 of 920 functions)
  branches...: no data found

Files changed coverage rate:
                                                                                     |Lines       |Functions  |Branches    
  Filename                                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================================
  src/lamp_py/ingestion/config_rt_alerts.py                                          |98.2%     56|41.7%    12|    -      0
  src/lamp_py/ingestion/convert_gtfs_rt.py                                           |88.4%    242|50.0%    26|    -      0
  src/lamp_py/ingestion/gtfs_rt_structs.py                                           | 100%      6|    -     0|    -      0

@runkelcorey runkelcorey requested a review from huangh June 22, 2026 18:20

@huangh huangh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm. last thing non-blocking- should we have some sort of logging when the schema gets expanded or some new input comes in? we want to know that it's happened and perhaps adjust the default schema of record to incude these changes eventually? otherwise our documented dataframely definitions will get outdated.

@runkelcorey runkelcorey merged commit edee712 into main Jun 22, 2026
10 checks passed
@runkelcorey runkelcorey deleted the feature-alerts-translated-strings branch June 22, 2026 21:09
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.

2 participants