Skip to content

Conversation

@AlexAxthelm
Copy link
Contributor

@AlexAxthelm AlexAxthelm commented Dec 4, 2024

Comment action didn't trigger on this PR. See https://rmi-pacta.github.io/workflow.pacta.dashboard/pr-preview/pr-35/

Major changes:

  • (CI) Adds Docker RMI checks + build process (docker.yml)
  • (CI/Test) Adds "integration test" in running docker image (against outputs of workflow.pacta's LATEST CI run, and publishing to GH pages (gh-pages.yml)
  • Adds default dir: dashboard_output_dir for rendered dashboard files (dir can be served directly with nginx)
  • Moves default dev data directory to dashboard_output_dir/data
  • (CI) passes hadolint
  • (R) adds function to build contents of dashboard_output_dir, and calling script updated to use that

Depends on RMI-PACTA/pacta-dashboard-svelte#129

@codecov
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (750d539) to head (c5da0ac).
Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
R/build_dashboard.R 0.00% 18 Missing ⚠️
R/copy_dashboard_files.R 0.00% 11 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main     #35   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         21      23    +2     
  Lines       1021    1049   +28     
=====================================
- Misses      1021    1049   +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented Dec 6, 2024

Docker build status

commit_time git_sha image
2024-12-10T09:47:55Z c5da0ac ghcr.io/rmi-pacta/workflow.pacta.dashboard:main

@AlexAxthelm AlexAxthelm marked this pull request as ready for review December 9, 2024 15:33
@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

@AlexAxthelm can you please name this PR more precisely, and it's intended function (for posterity, as this is what will appear in the git log and "integrate dashboard" isn't very descriptive)

@AlexAxthelm AlexAxthelm changed the title Feat/33 integrate dashboard CI/33 Add dashboard skeleton and integration test Dec 9, 2024
Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

I have a few comments already!
Have a look and I'll do another review tomorrow 😊

@github-actions
Copy link

github-actions bot commented Dec 9, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-12-10 09:51 UTC

@cjyetman
Copy link
Member

cjyetman commented Dec 9, 2024

The tech mix plots on the portfolio-level and sector-level tabs of
https://rmi-pacta.github.io/workflow.pacta.dashboard/pr-preview/pr-35/

are not working as expected

though they still work as expected here currently
https://rmi-pacta.github.io/pacta-dashboard-svelte/

no idea what's causing that or if it matters, just pointing it out because it seems pretty weird

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

The tech mix plots on the portfolio-level and sector-level tabs of https://rmi-pacta.github.io/workflow.pacta.dashboard/pr-preview/pr-35/

are not working as expected

though they still work as expected here currently https://rmi-pacta.github.io/pacta-dashboard-svelte/

no idea what's causing that or if it matters, just pointing it out because it seems pretty weird

Indeed, totally weird!
However, it's important to consider that this PR no longer uses the typical sample-data found in the pacta-dashboard-svelte repo, but is pulling from Azure:

e.g.

          source: https://pactadatadev.blob.core.windows.net/ghactions-workflow-pacta-results/main/latest/full_params_2023Q4/analysis_output_dir
          source: https://pactadatadev.blob.core.windows.net/benchmarks-webapp/2023Q4/2023Q4_20240529T002355Z

etc.

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

I will try to build and run the dashboard locally using these data and see if I can reproduce the behaviour

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

This is totally related to: #27 (comment)

(and also the reason that the zip_output function had to be disabled), woohoo, CI is working!! 😊
The short explanation is that, using the Azure input datasets, the output datasets data_company_bubbles.json and data_techexposure.json are empty.

This results in those plots not being able to be rendered.

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

data_techexposure.json is empty because start_year <- 2022, however the earliest year in the input data is 2023, so filters are removing all data.

What's a bit bizarre however is that the start_year value is coming directly from the input data manifest:

start_year <- manifest$params$analysis$startYear

It's very surprising that those would be mis-aligned.

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

data_techexposure.json is empty because start_year <- 2022, however the earliest year in the input data is 2023, so filters are removing all data.

What's a bit bizarre however is that the start_year value is coming directly from the input data manifest:

start_year <- manifest$params$analysis$startYear

It's very surprising that those would be mis-aligned.

Turns out it's the same issue for the data_company_bubbles.json dataset. Also filtering out all data when we filter start_year == 2022.

Now to understand why the manifest.json file is giving a start_year that is less than the earliest year in the analyzed data... 🤔

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

This is so bizarre haha. Everything in the manifest.json file of the analysis outputs points to 2023 as expected, except for the startYear:

  "params": {
    "portfolio": {
      "holdingsDate": "2023-12-31",
      "files": ["default_portfolio.csv"],
      "name": "Default Portfolio"
    },
    "analysis": {
      "equityMarketList": ["GlobalMarket", "DevelopedMarket", "EmergingMarket"],
      "scenarioGeographiesList": ["Global", "GlobalAggregate", "NonOECD", "OECD"],
      "scenarioSourcesList": ["GECO2023", "ISF2023", "WEO2023"],
      "sectorList": ["Power", "Automotive", "Coal", "Steel", "Aviation", "Cement"],
      "startYear": 2022, -> this is funky for some reason
      "timeHorizon": 5
    }
  },

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

Probably addressed by RMI-PACTA/workflow.pacta#103

@AlexAxthelm
Copy link
Contributor Author

Tech mix plots appear to be working, following merge of RMI-PACTA/workflow.pacta#104 (fix start year in test params)

Screenshot_2024-12-10_09 28 28
Screenshot_2024-12-10_09 28 49

@AlexAxthelm AlexAxthelm requested a review from jdhoffa December 10, 2024 09:29
@AlexAxthelm AlexAxthelm merged commit b18c2bb into main Dec 10, 2024
26 checks passed
@AlexAxthelm AlexAxthelm deleted the feat/33-integrate-dashboard branch December 10, 2024 09:47
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.

4 participants